Bug 1593944 - Emit event with StyleRuleActor as argument when its underlying CSS rule is updated. r=pbro
authorRazvan Caliman <rcaliman@mozilla.com>
Wed, 13 Nov 2019 14:04:37 +0000
changeset 501738 47a58584b81390485c7530b1eef7d59b2d6c32b2
parent 501737 4d16c3d62cfc0503075206bccfbd3b32ad396e64
child 501739 b8a3793ecceb864e30b0c7d060dc9397770b1d9e
push id114172
push userdluca@mozilla.com
push dateTue, 19 Nov 2019 11:31:10 +0000
treeherdermozilla-inbound@b5c5ba07d3db [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspbro
bugs1593944, 1557689
milestone72.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1593944 - Emit event with StyleRuleActor as argument when its underlying CSS rule is updated. r=pbro The fix for [Bug 1557689](https://bugzilla.mozilla.org/show_bug.cgi?id=1557689) created a situation in the [`Rule.onDeclarationsChanged()`](https://searchfox.org/mozilla-central/rev/6566d92dd46417a2f57e75c515135ebe84c9cef5/devtools/client/inspector/rules/models/rule.js#869-887) whereby the `isUsed` state of client-side declarations was made to point to a fixed `isUsed` state received from the server, thus losing sync with the latest state of the `StyleRuleActor`. Until another "declarations-update" event was fired from the `StyleRuleActor`, the rule's declarations' `isUsed` flag would point to the state with which they were overwritten on the last event handler call. As a reminder, the root cause of [Bug 1557689](https://bugzilla.mozilla.org/show_bug.cgi?id=1557689) was the inability force a "refresh" of the `StyleRuleFront` so it picked-up the latest `isUsed` state for its declarations when they depend on other declarations from other rules (ex: `justify-content: center` depends on `display:flex`). Therefore, the "declarations-updated" event was introduced with a payload of changed declarations to overwrite the client-side ones. It was convoluted, but it worked. However, while investigating the cause of this newer bug [Bug 1593944](https://bugzilla.mozilla.org/show_bug.cgi?id=1593944), I discovered an unusual but perhaps expected side-effect: when dispatching an event over the protocol with a payload of the `StyleRuleActor`, its corresponding `StyleRuleFront` on the client would "refresh" automatically (meaning that, looking up state on the previous front reference would point to the latest state from the actor) . The client doesn't even need to use this payload to replace its previous front reference. Surprisingly, the client doesn't even need to explicitly listen to the event which carries the `StyleRuleActor`/`StyleRuleFront` reference. So long as a previous reference of the front exists on the client, it will point to the updated state of the actor. I haven't been able to identify whether this is a known and expected behavior, so I'm pinging @jdescottes and @ochameau to weigh in on the validity of these findings. Relying on this behavior, the fix for both [Bug 1557689](https://bugzilla.mozilla.org/show_bug.cgi?id=1557689) and [Bug 1557689](https://bugzilla.mozilla.org/show_bug.cgi?id=1557689) involves simply emitting an event, "rule-updated", with the `StyleRuleActor` instance as payload whenever its internal state changes meaningfully so the corresponding front updates. The client will pick-up the latest `isUsed` state of declarations from its reference to the `StyleRuleFront`. --- Another way to check this behavior and hypothesis is to do the following: - In [`StyleRuleActor.setRuleText()`](https://searchfox.org/mozilla-central/rev/6566d92dd46417a2f57e75c515135ebe84c9cef5/devtools/server/actors/styles.js#1704) replace `return this` with `return null`; (this will no longer return the `StyleRuleActor` over the protocol; it's not explicitly used on the client anyway). - In the spec, replace the [`setRuleText()`](https://searchfox.org/mozilla-central/rev/6566d92dd46417a2f57e75c515135ebe84c9cef5/devtools/shared/specs/styles.js#222) return value with `RetVal("nullable:domstylerule")` so the protocol doesn't throw an error when getting the `null` from the actor. - Make a build. - Then, open the Inspector -> Rules View and change the value of a valid declaration, say: `display: block`, to something invalid, like `display: booo`. Notice that the declaration is no longer marked invalid with a warning sign. That's because the declaration's [`isValid`](https://searchfox.org/mozilla-central/rev/6566d92dd46417a2f57e75c515135ebe84c9cef5/devtools/server/actors/styles.js#1447) flag is set on the actor but it no longer syncs with the client which uses the corresponding front to render the declaration after the change. Not returning the `StyleRuleActor` over the protocol breaks this sync actor-front sync. Differential Revision: https://phabricator.services.mozilla.com/D52560
devtools/client/inspector/rules/models/rule.js
devtools/server/actors/styles.js
devtools/shared/fronts/styles.js
devtools/shared/specs/styles.js
--- a/devtools/client/inspector/rules/models/rule.js
+++ b/devtools/client/inspector/rules/models/rule.js
@@ -72,27 +72,40 @@ class Rule {
     // Populate the text properties with the style's current authoredText
     // value, and add in any disabled properties from the store.
     this.textProps = this._getTextProperties();
     this.textProps = this.textProps.concat(this._getDisabledProperties());
 
     this.getUniqueSelector = this.getUniqueSelector.bind(this);
     this.onDeclarationsUpdated = this.onDeclarationsUpdated.bind(this);
     this.onLocationChanged = this.onLocationChanged.bind(this);
+    this.onStyleRuleFrontUpdated = this.onStyleRuleFrontUpdated.bind(this);
     this.updateSourceLocation = this.updateSourceLocation.bind(this);
 
-    this.domRule.on("declarations-updated", this.onDeclarationsUpdated);
+    // Added in Firefox 72 for backwards compatibility of initial fix for Bug 1557689.
+    // See follow-up fix in Bug 1593944.
+    if (this.domRule.traits.emitsRuleUpdatedEvent) {
+      this.domRule.on("rule-updated", this.onStyleRuleFrontUpdated);
+    } else {
+      this.domRule.on("declarations-updated", this.onDeclarationsUpdated);
+    }
   }
 
   destroy() {
     if (this.unsubscribeSourceMap) {
       this.unsubscribeSourceMap();
     }
 
-    this.domRule.off("declarations-updated", this.onDeclarationsUpdated);
+    // Added in Firefox 72
+    if (this.domRule.traits.emitsRuleUpdatedEvent) {
+      this.domRule.off("rule-updated", this.onStyleRuleFrontUpdated);
+    } else {
+      this.domRule.off("declarations-updated", this.onDeclarationsUpdated);
+    }
+
     this.domRule.off("location-changed", this.onLocationChanged);
   }
 
   get declarations() {
     return this.textProps;
   }
 
   get inheritance() {
@@ -572,16 +585,32 @@ class Rule {
     // Need to re-apply properties in case removing this TextProperty
     // exposes another one.
     this.applyProperties(modifications => {
       modifications.removeProperty(index, property.name);
     });
   }
 
   /**
+   * Event handler for "rule-updated" event fired by StyleRuleActor.
+   *
+   * @param {StyleRuleFront} front
+   */
+  onStyleRuleFrontUpdated(front) {
+    // Overwritting this reference is not required, but it's here to avoid confusion.
+    // Whenever an actor is passed over the protocol, either as a return value or as
+    // payload on an event, the `form` of its corresponding front will be automatically
+    // updated. No action required.
+    // Even if this `domRule` reference here is not explicitly updated, lookups of
+    // `this.domRule.declarations` will point to the latest state of declarations set
+    // on the actor. Everything on `StyleRuleForm.form` will point to the latest state.
+    this.domRule = front;
+  }
+
+  /**
    * Get the list of TextProperties from the style. Needs
    * to parse the style's authoredText.
    */
   _getTextProperties() {
     const textProps = [];
     const store = this.elementStyle.store;
 
     // Starting with FF49, StyleRuleActors provide parsed declarations.
@@ -851,16 +880,18 @@ class Rule {
       if (!prop.invisible) {
         return true;
       }
     }
     return false;
   }
 
   /**
+   * TODO: Remove after Firefox 75. Keep until then for backwards-compatibility for Bug
+   * 1557689 which has an updated fix from Bug 1593944.
    * Handler for "declarations-updated" events fired from the StyleRuleActor for a
    * CSS rule when the status of any of its CSS declarations change.
    *
    * Check whether the used/unused status of any declaration has changed and update the
    * inactive CSS indicator in the UI accordingly.
    *
    * @param {Array} declarations
    *        List of objects describing all CSS declarations of this CSS rule.
--- a/devtools/server/actors/styles.js
+++ b/devtools/server/actors/styles.js
@@ -1115,17 +1115,17 @@ var PageStyleActor = protocol.ActorClass
   /**
    * Cause all StyleRuleActor instances of observed CSS rules to check whether the
    * states of their declarations have changed.
    *
    * Observed rules are the latest rules returned by a call to PageStyleActor.getApplied()
    *
    * This is necessary because changes in one rule can cause the declarations in another
    * to not be applicable (inactive CSS). The observers of those rules should be notified.
-   * Rules will fire a "declarations-updated" event if their declarations changed states.
+   * Rules will fire a "rule-updated" event if any of their declarations changed state.
    *
    * Call this method whenever a CSS rule is mutated:
    * - a CSS declaration is added/changed/disabled/removed
    * - a selector is added/changed/removed
    */
   refreshObservedRules() {
     for (const rule of this._observedRules) {
       rule.refresh();
@@ -1351,19 +1351,22 @@ var StyleRuleActor = protocol.ActorClass
 
   form: function() {
     const form = {
       actor: this.actorID,
       type: this.type,
       line: this.line || undefined,
       column: this.column,
       traits: {
-        // Whether the style rule actor implements the setRuleText
-        // method.
+        // Indicates whether StyleRuleActor implements and can use the setRuleText method.
+        // It cannot use it if the stylesheet was programmatically mutated via the CSSOM.
         canSetRuleText: this.canSetRuleText,
+        // Indicates that StyleRuleActor emits the "rule-updated" event.
+        // Added in Firefox 72.
+        emitsRuleUpdatedEvent: true,
       },
     };
 
     if (this.rawRule.parentRule) {
       form.parentRule = this.pageStyle._styleRef(
         this.rawRule.parentRule
       ).actorID;
 
@@ -1696,16 +1699,18 @@ var StyleRuleActor = protocol.ActorClass
         cssText.substring(offset + text.length);
 
       await parentStyleSheet.update(cssText, false, UPDATE_PRESERVING_RULES);
     }
 
     this.authoredText = newText;
     this.pageStyle.refreshObservedRules();
 
+    // Returning this updated actor over the protocol will update its corresponding front
+    // and any references to it.
     return this;
   },
 
   /**
    * Modify a rule's properties. Passed an array of modifications:
    * {
    *   type: "set",
    *   index: <number>,
@@ -2001,18 +2006,17 @@ var StyleRuleActor = protocol.ActorClass
     });
   },
 
   /**
    * Using the latest computed style applicable to the selected element,
    * check the states of declarations in this CSS rule.
    *
    * If any have changed their used/unused state, potentially as a result of changes in
-   * another rule, fire a "declarations-updated" event with all declarations and their
-   * updated states.
+   * another rule, fire a "rule-updated" event with this rule actor in its latest state.
    */
   refresh() {
     let hasChanged = false;
     const el = this.pageStyle.selectedElement;
     const style = CssLogic.getComputedStyle(el);
 
     for (const decl of this._declarations) {
       // TODO: convert from Object to Boolean. See Bug 1574471
@@ -2025,17 +2029,26 @@ var StyleRuleActor = protocol.ActorClass
 
       if (decl.isUsed.used !== isUsed.used) {
         decl.isUsed = isUsed;
         hasChanged = true;
       }
     }
 
     if (hasChanged) {
-      this.emit("declarations-updated", this._declarations);
+      // ⚠️ IMPORTANT ⚠️
+      // When an event is emitted via the protocol with the StyleRuleActor as payload, the
+      // corresponding StyleRuleFront will be automatically updated under the hood.
+      // Therefore, when the client looks up properties on the front reference it already
+      // has, it will get the latest values set on the actor, not the ones it originally
+      // had when the front was created. The client is not required to explicitly replace
+      // its previous front reference to the one it receives as this event's payload.
+      // The client doesn't even need to explicitly listen for this event.
+      // The update of the front happens automatically.
+      this.emit("rule-updated", this);
     }
   },
 });
 
 /**
  * Helper function for getting an image preview of the given font.
  *
  * @param font {string}
--- a/devtools/shared/fronts/styles.js
+++ b/devtools/shared/fronts/styles.js
@@ -89,16 +89,17 @@ class StyleRuleFront extends FrontClassW
     super(client, targetFront, parentFront);
 
     this.before("location-changed", this._locationChangedPre.bind(this));
   }
 
   form(form) {
     this.actorID = form.actor;
     this._form = form;
+    this.traits = form.traits || {};
     if (this._mediaText) {
       this._mediaText = null;
     }
   }
 
   /**
    * Ensure _form is updated when location-changed is emitted.
    */
--- a/devtools/shared/specs/styles.js
+++ b/devtools/shared/specs/styles.js
@@ -197,19 +197,19 @@ const styleRuleSpec = generateActorSpec(
   typeName: "domstylerule",
 
   events: {
     "location-changed": {
       type: "locationChanged",
       line: Arg(0, "number"),
       column: Arg(1, "number"),
     },
-    "declarations-updated": {
-      type: "declarationsUpdated",
-      declarations: Arg(0, "array:json"),
+    "rule-updated": {
+      type: "ruleUpdated",
+      rule: Arg(0, "domstylerule"),
     },
   },
 
   methods: {
     getRuleText: {
       response: {
         text: RetVal("string"),
       },