Bug 1325145 - [devtools] Wait for setProperty/preview to be settled before updating TextPropertyEditor on value inplace editor destroy. r=devtools-reviewers,bomsy.

This patch updates InplaceEditor so it passes the result of the options.done callback
to the options.destroy one, so we can orchestrate things in callsites.

For the declaration value, this allows us to properly wait for the value to be
previewed/set before updating the view, so all underlying data are up to date.

A test case is added to check that this fixes the original issue (warning icon
being displayed on declaration after clearing the value but dismissing the edit).
We also add an option to the `openRuleView` helper to avoid overriding the `debounce`
method, so test can replicate more accurately what actually happens.

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

devtools/client/inspector/rules/test/browser_rules_edit-property-cancel.js

@@ -17,7 +17,7 @@ const TEST_URI = `
 
 add_task(async function () {
   await addTab("data:text/html;charset=utf-8," + encodeURIComponent(TEST_URI));
-  const { inspector, view } = await openRuleView();
+  const { inspector, view } = await openRuleView({ overrideDebounce: false });
   await selectNode("#testid", inspector);
 
   const ruleEditor = getRuleViewRuleEditor(view, 1);
@@ -38,11 +38,28 @@ add_task(async function () {
     "#00F background color is set."
   );
 
-  await focusEditableField(view, propEditor.valueSpan);
+  const editor = await focusEditableField(view, propEditor.valueSpan);
+  info("Delete input value");
   const onValueDeleted = view.once("ruleview-changed");
-  await sendKeysAndWaitForFocus(view, ruleEditor.element, ["DELETE", "ESCAPE"]);
+  EventUtils.sendKey("DELETE", view.styleWindow);
   await onValueDeleted;
 
+  is(editor.input.value, "", "value input is empty");
+
+  await waitFor(() => view.popup?.isOpen);
+  ok(true, "autocomplete popup opened");
+
+  info("Hide autocomplete popup");
+  const onPopupClosed = once(view.popup, "popup-closed");
+  EventUtils.sendKey("ESCAPE", view.styleWindow);
+  await onPopupClosed;
+  ok(true, "popup was closed");
+
+  info("Cancel edit with escape key");
+  const onRuleViewChanged = view.once("ruleview-changed");
+  EventUtils.sendKey("ESCAPE", view.styleWindow);
+  await onRuleViewChanged;
+
   is(
     propEditor.valueSpan.textContent,
     "#00F",
@@ -53,4 +70,10 @@ add_task(async function () {
     "rgb(0, 0, 255)",
     "#00F background color is set."
   );
+
+  is(
+    propEditor.warning.hidden,
+    true,
+    "warning icon is hidden after cancelling the edit"
+  );
 });

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

@@ -423,7 +423,19 @@ TextPropertyEditor.prototype = {
         start: this._onStartEditing,
         element: this.valueSpan,
         done: this._onValueDone,
-        destroy: this.update,
+        destroy: onValueDonePromise => {
+          const cb = this.update;
+          // The `done` callback is called before this `destroy` callback is.
+          // In _onValueDone, we might preview/set the property and we want to wait for
+          // that to be resolved before updating the view so all data are up to date (see Bug 1325145).
+          if (
+            onValueDonePromise &&
+            typeof onValueDonePromise.then === "function"
+          ) {
+            return onValueDonePromise.then(cb);
+          }
+          return cb();
+        },
         validate: this._onValidate,
         advanceChars: advanceValidate,
         contentType: InplaceEditor.CONTENT_TYPES.CSS_VALUE,
@@ -1274,13 +1286,13 @@ TextPropertyEditor.prototype = {
     // If the value is not empty (or is an empty variable) and unchanged,
     // revert the property back to its original value and enabled or disabled state
     if ((value.trim() || isVariable) && isValueUnchanged) {
-      this.ruleEditor.rule.previewPropertyValue(
+      const onPropertySet = this.ruleEditor.rule.previewPropertyValue(
         this.prop,
         val.value,
         val.priority
       );
       this.rule.setPropertyEnabled(this.prop, this.prop.enabled);
-      return;
+      return onPropertySet;
     }
 
     // Check if unit of value changed to add dragging feature
@@ -1293,7 +1305,7 @@ TextPropertyEditor.prototype = {
     this.telemetry.recordEvent("edit_rule", "ruleview");
 
     // First, set this property value (common case, only modified a property)
-    this.prop.setValue(val.value, val.priority);
+    const onPropertySet = this.prop.setValue(val.value, val.priority);
 
     if (!this.prop.enabled) {
       this.prop.setEnabled(true);
@@ -1322,6 +1334,8 @@ TextPropertyEditor.prototype = {
         }
       }, 0);
     }
+
+    return onPropertySet;
   },
 
   /**

devtools/client/inspector/test/shared-head.js

@@ -81,19 +81,25 @@ var openInspectorSidebarTab = async function (id) {
  * Open the toolbox, with the inspector tool visible, and the rule-view
  * sidebar tab selected.
  *
+ * @param {Object} options
+ * @param {Boolean} options.overrideDebounce: Whether to replace the rule view debounce
+ *        method with manual debounce (requires explicit calls to trigger the debounced calls).
+ *        Defaults to true.
  * @return a promise that resolves when the inspector is ready and the rule view
  * is visible and ready
  */
-async function openRuleView() {
+async function openRuleView({ overrideDebounce = true } = {}) {
   const { inspector, toolbox, highlighterTestFront } = await openInspector();
 
   const ruleViewPanel = inspector.getPanel("ruleview");
   await ruleViewPanel.readyPromise;
   const view = ruleViewPanel.view;
 
-  // Replace the view to use a custom debounce function that can be triggered manually
-  // through an additional ".flush()" property.
-  view.debounce = manualDebounce();
+  if (overrideDebounce) {
+    // Replace the view to use a custom debounce function that can be triggered manually
+    // through an additional ".flush()" property.
+    view.debounce = manualDebounce();
+  }
 
   return {
     toolbox,

devtools/client/shared/inplace-editor.js

@@ -122,6 +122,7 @@ function isKeyIn(key, ...keys) {
  *        This function is called before the editor has been torn down.
  * @param {Function} options.destroy:
  *        Called when the editor is destroyed and has been torn down.
+ *        This may be called with the return value of the options.done callback (if it is passed).
  * @param {Function} options.contextMenu:
  *        Called when the user triggers a contextmenu event on the input.
  * @param {Object} options.advanceChars:
@@ -476,8 +477,12 @@ class InplaceEditor extends EventEmitter {
 
   /**
    * Get rid of the editor.
+   *
+   * @param {*|null} doneCallResult: When #clear is called after calling #apply, this will
+   *        be the returned value of the call to options.done that is done there.
+   *        Will be null when options.done is undefined.
    */
-  #clear() {
+  #clear(doneCallResult) {
     if (!this.input) {
       // Already cleared.
       return;
@@ -499,7 +504,7 @@ class InplaceEditor extends EventEmitter {
     delete this.elt;
 
     if (this.destroy) {
-      this.destroy();
+      this.destroy(doneCallResult);
     }
   }
 
@@ -1157,8 +1162,8 @@ class InplaceEditor extends EventEmitter {
     ) {
       this.#acceptPopupSuggestion();
     } else {
-      this.#apply();
-      this.#clear();
+      const onApplied = this.#apply();
+      this.#clear(onApplied);
     }
   };
 
@@ -1363,7 +1368,7 @@ class InplaceEditor extends EventEmitter {
         }
       }
 
-      this.#apply(direction, key);
+      const onApplied = this.#apply(direction, key);
 
       // Close the popup if open
       if (this.popup && this.popup.isOpen) {
@@ -1389,7 +1394,7 @@ class InplaceEditor extends EventEmitter {
         }
       }
 
-      this.#clear();
+      this.#clear(onApplied);
     } else if (isKeyIn(key, "ESCAPE")) {
       // Cancel and blur ourselves.
       // Now we don't want to suggest anything as we are moving out.
@@ -1399,8 +1404,8 @@ class InplaceEditor extends EventEmitter {
         this.#hideAutocompletePopup();
       } else {
         this.cancelled = true;
-        this.#apply();
-        this.#clear();
+        const onApplied = this.#apply();
+        this.#clear(onApplied);
       }
       prevent = true;
       event.stopPropagation();