Bug 1871806 - [devtools] Add notice for Enter behavior in Rules view. r=devtools-reviewers,ochameau.

A link points to a blog post explaining why we made the change.
Once the notice is closed by the user, it won't be opened again.
The notice is not localized so it's easier to uplift for now; it will
be localized in a follow up.

Depends on D197897

Differential Revision: https://phabricator.services.mozilla.com/D197924

browser/app/profile/firefox.js

@@ -2522,6 +2522,8 @@ pref("devtools.gridinspector.maxHighlighters", 3);
 // Whether or not simplified highlighters should be used when
 // prefers-reduced-motion is enabled.
 pref("devtools.inspector.simple-highlighters-reduced-motion", false);
+// Display notice about Enter key behavior in Rules view.
+pref("devtools.inspector.showRulesViewEnterKeyNotice", true);
 
 // Whether or not the box model panel is opened in the layout view
 pref("devtools.layout.boxmodel.opened", true);

devtools/client/inspector/index.xhtml

@@ -235,6 +235,23 @@
           <div id="ruleview-container" class="ruleview" role="document">
             <div id="ruleview-container-focusable" tabindex="-1"></div>
           </div>
+
+          <aside id="ruleview-kbd-enter-notice" hidden="">
+            <img src="chrome://devtools/skin/images/fox-smiling.svg" />
+            <p>
+              <kbd>Enter</kbd> no longer moves focus to the next input. Use
+              <kbd id="ruleview-kbd-enter-notice-ctrl-cmd"></kbd> +
+              <kbd>Enter</kbd> or <kbd>Tab</kbd>.
+              <a href="https://fxdx.dev/rules-view-enter-key/" target="_blank"
+                >Read why</a
+              >
+            </p>
+            <button
+              id="ruleview-kbd-enter-notice-dismiss-button"
+              class="devtools-button"
+              title="Hide"
+            ></button>
+          </aside>
         </div>
 
         <div

devtools/client/inspector/rules/rules.js

@@ -144,6 +144,7 @@ function CssRuleView(inspector, document, store) {
   this.childHasDragged = false;
 
   this._outputParser = new OutputParser(document, this.cssProperties);
+  this._abortController = new this.styleWindow.AbortController();
 
   this._onAddRule = this._onAddRule.bind(this);
   this._onContextMenu = this._onContextMenu.bind(this);
@@ -675,6 +676,48 @@ CssRuleView.prototype = {
     this.pageStyle.addNewRule(element, pseudoClasses);
   },
 
+  maybeShowEnterKeyNotice() {
+    const SHOW_RULES_VIEW_ENTER_KEY_NOTICE_PREF =
+      "devtools.inspector.showRulesViewEnterKeyNotice";
+    // Make the Enter key notice visible
+    // if it wasn't dismissed by the user yet.
+    if (
+      !Services.prefs.getBoolPref(SHOW_RULES_VIEW_ENTER_KEY_NOTICE_PREF, false)
+    ) {
+      return;
+    }
+
+    const enterKeyNoticeEl = this.styleDocument.getElementById(
+      "ruleview-kbd-enter-notice"
+    );
+
+    if (!enterKeyNoticeEl.hasAttribute("hidden")) {
+      return;
+    }
+
+    // Compute the right key (Cmd / Ctrl) depending on the OS
+    enterKeyNoticeEl.querySelector(
+      "#ruleview-kbd-enter-notice-ctrl-cmd"
+    ).textContent = Services.appinfo.OS === "Darwin" ? "Cmd" : "Ctrl";
+    enterKeyNoticeEl.removeAttribute("hidden");
+
+    enterKeyNoticeEl
+      .querySelector("#ruleview-kbd-enter-notice-dismiss-button")
+      .addEventListener(
+        "click",
+        () => {
+          // Hide the notice
+          enterKeyNoticeEl.setAttribute("hidden", "");
+          // And set the pref to false so we don't show the notice on next startup.
+          Services.prefs.setBoolPref(
+            SHOW_RULES_VIEW_ENTER_KEY_NOTICE_PREF,
+            false
+          );
+        },
+        { once: true, signal: this._abortController.signal }
+      );
+  },
+
   /**
    * Disables add rule button when needed
    */
@@ -878,6 +921,8 @@ CssRuleView.prototype = {
     this.tooltips.destroy();
 
     // Remove bound listeners
+    this._abortController.abort();
+    this._abortController = null;
     this.shortcuts.destroy();
     this.styleDocument.removeEventListener("click", this, { capture: true });
     this.element.removeEventListener("copy", this._onCopy);

devtools/client/inspector/rules/test/browser_rules_container-queries.js

@@ -244,7 +244,17 @@ async function assertJumpToContainerButton(
   );
 
   await onNodeUnhighlight;
-  ok("Highlighter was hidden when clicking on icon");
+  ok(true, "Highlighter was hidden when clicking on icon");
+
+  // Move mouse so it does stay in a position where it could hover something impacting
+  // the test.
+  EventUtils.synthesizeMouse(
+    selectContainerButton.closest("body"),
+    0,
+    0,
+    { type: "mouseover" },
+    selectContainerButton.ownerDocument.defaultView
+  );
 }
 
 async function assertQueryContainerTooltip({

devtools/client/inspector/rules/test/browser_rules_keybindings.js

@@ -6,12 +6,18 @@
 // Test keyboard navigation in the rule view
 
 add_task(async function () {
-  await addTab(`data:text/html;charset=utf-8,
+  await pushPref("devtools.inspector.showRulesViewEnterKeyNotice", true);
+  const tab = await addTab(`data:text/html;charset=utf-8,
     <style>h1 {}</style>
     <h1>Some header text</h1>`);
-  const { inspector, view } = await openRuleView();
+  let { inspector, view } = await openRuleView();
   await selectNode("h1", inspector);
 
+  let kbdNoticeEl = view.styleDocument.getElementById(
+    "ruleview-kbd-enter-notice"
+  );
+  ok(kbdNoticeEl.hasAttribute("hidden"), "Notice is not displayed by default");
+
   info("Getting the ruleclose brace element for the `h1` rule");
   const brace = view.styleDocument.querySelectorAll(".ruleview-ruleclose")[1];
 
@@ -68,6 +74,18 @@ add_task(async function () {
     )?.valueSpan?.querySelector(".ruleview-colorswatch")
   );
 
+  ok(
+    !kbdNoticeEl.hasAttribute("hidden"),
+    "Notice is displayed after hitting Enter"
+  );
+
+  info("Click on dismiss button");
+  kbdNoticeEl.querySelector("button").click();
+  ok(
+    kbdNoticeEl.hasAttribute("hidden"),
+    "Notice was hidden after clicking on dismiss button"
+  );
+
   is(
     view.styleDocument.activeElement.textContent,
     "gold",
@@ -125,6 +143,26 @@ add_task(async function () {
   EventUtils.sendKey("Space");
   await onRuleViewRefreshed;
   ok(toggleEl.checked, "Checkbox is checked again");
+
+  info("Re-start the toolbox");
+  await gDevTools.closeToolboxForTab(tab);
+  ({ view } = await openRuleView());
+
+  kbdNoticeEl = view.styleDocument.getElementById("ruleview-kbd-enter-notice");
+  ok(
+    kbdNoticeEl.hasAttribute("hidden"),
+    "Notice isn't displayed on init when it was dismissed before"
+  );
+  is(
+    Services.prefs.getBoolPref(
+      "devtools.inspector.showRulesViewEnterKeyNotice"
+    ),
+    false,
+    "The preference driving the UI is set to false, as expected"
+  );
+  Services.prefs.clearUserPref(
+    "devtools.inspector.showRulesViewEnterKeyNotice"
+  );
 });
 
 // The `element` have specific behavior, so we want to test that keyboard navigation

devtools/client/inspector/rules/test/browser_rules_large_base64_background_image.js

@@ -40,6 +40,7 @@ const TEST_URL =
 
 // Check that long URLs are rendered correctly in the rule view.
 add_task(async function () {
+  await pushPref("devtools.inspector.showRulesViewEnterKeyNotice", false);
   const { inspector } = await openInspectorForURL(TEST_URL);
   const view = selectRuleView(inspector);
 

devtools/client/inspector/rules/views/rule-editor.js

@@ -39,6 +39,12 @@ loader.lazyRequireGetter(
   "resource://devtools/client/definitions.js",
   true
 );
+loader.lazyRequireGetter(
+  this,
+  "KeyCodes",
+  "resource://devtools/client/shared/keycodes.js",
+  true
+);
 
 const STYLE_INSPECTOR_PROPERTIES =
   "devtools/shared/locales/styleinspector.properties";
@@ -901,8 +907,14 @@ RuleEditor.prototype = {
    *        True if the change should be applied.
    * @param {Number} direction
    *        The move focus direction number.
+   * @param {Number} key
+   *        The event keyCode that trigger the editor to close
    */
-  async _onSelectorDone(value, commit, direction) {
+  async _onSelectorDone(value, commit, direction, key) {
+    if (value && commit && !direction && key === KeyCodes.DOM_VK_RETURN) {
+      this.ruleView.maybeShowEnterKeyNotice();
+    }
+
     if (
       !commit ||
       this.isEditing ||

devtools/client/inspector/rules/views/text-property-editor.js

@@ -41,6 +41,12 @@ loader.lazyRequireGetter(
   "resource://devtools/shared/inspector/css-logic.js",
   true
 );
+loader.lazyRequireGetter(
+  this,
+  "KeyCodes",
+  "resource://devtools/client/shared/keycodes.js",
+  true
+);
 loader.lazyGetter(this, "PROPERTY_NAME_INPUT_LABEL", function () {
   return l10n("rule.propertyName.label");
 });
@@ -1141,8 +1147,14 @@ TextPropertyEditor.prototype = {
    *        True if the change should be applied.
    * @param {Number} direction
    *        The move focus direction number.
+   * @param {Number} key
+   *        The event keyCode that trigger the editor to close
    */
-  _onNameDone(value, commit, direction) {
+  _onNameDone(value, commit, direction, key) {
+    if (value && commit && !direction && key === KeyCodes.DOM_VK_RETURN) {
+      this.ruleView.maybeShowEnterKeyNotice();
+    }
+
     const isNameUnchanged =
       (!commit && !this.ruleEditor.isEditing) || this.committed.name === value;
     if (this.prop.value && isNameUnchanged) {
@@ -1226,8 +1238,14 @@ TextPropertyEditor.prototype = {
    *        True if the change should be applied.
    * @param {Number} direction
    *        The move focus direction number.
+   * @param {Number} key
+   *        The event keyCode that trigger the editor to close
    */
-  _onValueDone(value = "", commit, direction) {
+  _onValueDone(value = "", commit, direction, key) {
+    if (value && commit && !direction && key === KeyCodes.DOM_VK_RETURN) {
+      this.ruleView.maybeShowEnterKeyNotice();
+    }
+
     const parsedProperties = this._getValueAndExtraProperties(value);
     const val = parseSingleValue(
       this.cssProperties.isKnown,

devtools/client/inspector/test/browser_inspector_search-sidebar.js

@@ -15,7 +15,7 @@ add_task(async function () {
   isInInspectorSearchBox(inspector);
 
   info("Click somewhere in the rule-view");
-  clickInRuleView(inspector);
+  moveFocusInRuleView(inspector);
 
   info("Check that the rule-view search field gets focused");
   pressCtrlF();
@@ -52,9 +52,13 @@ function pressCtrlF() {
   EventUtils.synthesizeKey("f", { accelKey: true });
 }
 
-function clickInRuleView(inspector) {
-  const el = inspector.panelDoc.querySelector("#sidebar-panel-ruleview");
-  EventUtils.synthesizeMouseAtCenter(el, {}, inspector.panelDoc.defaultView);
+function moveFocusInRuleView(inspector) {
+  // Only focus an element in the view so this has no unintended effects.
+  // Put the focus on the `element` closing bracket, which should always be visible
+  // and is focusable as it's a button.
+  inspector.panelDoc
+    .querySelector("#sidebar-panel-ruleview .ruleview-ruleclose")
+    .focus();
 }
 
 function clickInComputedView(inspector) {

devtools/client/shared/inplace-editor.js

@@ -8,7 +8,7 @@
  *
  * editableField({
  *   element: spanToEdit,
- *   done: function(value, commit, direction) {
+ *   done: function(value, commit, direction, key) {
  *     if (commit) {
  *       spanToEdit.textContent = value;
  *     }
@@ -107,8 +107,8 @@ function isKeyIn(key, ...keys) {
  * @param {Function} options.done:
  *        Called when input is committed or blurred.  Called with
  *        current value, a boolean telling the caller whether to
- *        commit the change, and the direction of the next element to be
- *        selected. Direction may be one of Services.focus.MOVEFOCUS_FORWARD,
+ *        commit the change, the direction of the next element to be
+ *        selected and the event keybode. Direction may be one of Services.focus.MOVEFOCUS_FORWARD,
  *        Services.focus.MOVEFOCUS_BACKWARD, or null (no movement).
  *        This function is called before the editor has been torn down.
  * @param {Function} options.destroy:
@@ -1095,7 +1095,7 @@ class InplaceEditor extends EventEmitter {
   /**
    * Call the client's done handler and clear out.
    */
-  #apply(event, direction) {
+  #apply(direction, key) {
     if (this.#applied) {
       return null;
     }
@@ -1104,7 +1104,7 @@ class InplaceEditor extends EventEmitter {
 
     if (this.done) {
       const val = this.cancelled ? this.initial : this.currentInputValue;
-      return this.done(val, !this.cancelled, direction);
+      return this.done(val, !this.cancelled, direction, key);
     }
 
     return null;
@@ -1335,7 +1335,7 @@ class InplaceEditor extends EventEmitter {
         }
       }
 
-      this.#apply(event, direction);
+      this.#apply(direction, key);
 
       // Close the popup if open
       if (this.popup && this.popup.isOpen) {

devtools/client/themes/rules.css

@@ -963,3 +963,38 @@
 .ruleview-propertyvalue-break-spaces {
   white-space: break-spaces;
 }
+
+/* Rule view notice about "Enter" key behavior */
+#ruleview-kbd-enter-notice {
+  display: grid;
+  grid-template-columns: 22px 1fr auto;
+  align-items: start;
+  border-top: 1px solid var(--theme-splitter-color);
+  padding: 8px;
+  background-color: var(--theme-body-alternate-emphasized-background);
+  column-gap: 1em;
+  font-family: system-ui, -apple-system, sans-serif;
+  font-size: 12px;
+
+  &[hidden] {
+    display: none;
+  }
+
+  /* fox icon */
+  img {
+    max-width: 100%;
+  }
+
+  p {
+    margin: 0;
+    align-self: center;
+  }
+
+  kbd {
+    font-weight: bold;
+  }
+
+  #ruleview-kbd-enter-notice-dismiss-button::before {
+    background-image: url("chrome://devtools/skin/images/close.svg");
+  }
+}