1519988 - Implement toggling on/off of a CSS declaration

Status: RESOLVED FIXED

(DevTools :: Inspector: Rules, enhancement, P3)

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: 1519988.patch [1.0]

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

Main logic https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/views/text-property-editor.js#753

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

Regarding the Math.random(), I was looking up a good way to generate a UUID for an unique identifier for the declarations since indexes won't work well when we add new declarations, and this method came up the most in my searches.

Comment 4

[Razvan Caliman [:rcaliman]]

Comment on attachment 9036447 [details] [diff] [review]
1519988.patch [1.0]

Review of attachment 9036447 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/inspector/rules/components/Declaration.js
@@ +38,5 @@
>      });
>    }
>  
> +  onToggleDeclarationEnabledClick(event) {
> +    event.stopPropagation();

Is this `event.stopPropagation();` necessary?

::: devtools/client/inspector/rules/components/Declarations.js
@@ +15,5 @@
>  class Declarations extends PureComponent {
>    static get propTypes() {
>      return {
>        declarations: PropTypes.arrayOf(PropTypes.shape(Types.declaration)).isRequired,
> +      onToggleDeclarationEnabled: PropTypes.object.isRequired,

Shouldn't this be `PropTypes.func.isRequired` ?

::: devtools/client/inspector/rules/models/element-style.js
@@ +332,5 @@
>      }
>    },
>  
>    /**
> +   * Toggles the enabled property of the given CSS declaration.

s/property/state

@@ +340,5 @@
> +   * @param {String} declarationId
> +   *        The TextProperty id for the CSS declaration.
> +   *
> +   */
> +  toggleDeclarationEnabled: function(ruleId, declarationId) {

`toggleDeclarationEnabled` is very wordy and appears in many places in this patch.

I suggest renaming it to `toggleDeclaration` which is adequately explicit. Plus, it follows the naming precedent set by `togglePseudoClass`.

@@ +341,5 @@
> +   *        The TextProperty id for the CSS declaration.
> +   *
> +   */
> +  toggleDeclarationEnabled: function(ruleId, declarationId) {
> +    const rule = this.getRule(ruleId);

If rule or declaration can be undefined, perhaps it's best to guard against those here.

::: devtools/client/inspector/rules/models/text-property.js
@@ +34,5 @@
>   *        coming from parseDeclarations.
>   */
>  function TextProperty(rule, name, value, priority, enabled = true,
>                        invisible = false) {
> +  this.id = name + "_" + Math.random().toString(36).substr(2, 5);

We seem to have a uuid generator avaialble: 
https://searchfox.org/mozilla-central/source/devtools/shared/generate-uuid.js

It does seem to cross the JS/C++ boundary so I don't know if using that incurs any substantial performance hit.

I wonder if this id is stable across the session (or perhaps that doesn't matter; the implications are a bit obscure to me right now). 

There is at least one place where the list of text properties is rebuilt and replaced during the same session in Rule.refresh():

https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/models/rule.js#518-529

This gets called on the element style model by _maybeAddRule():
https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/models/element-style.js#188

::: devtools/client/inspector/rules/new-rules.js
@@ +144,5 @@
> +   * @param  {String} declarationId
> +   *         The TextProperty id for the CSS declaration.
> +   */
> +  onToggleDeclarationEnabled(ruleId, declarationId) {
> +    this.elementStyle.toggleDeclarationEnabled(ruleId, declarationId);

`ElementStyle.toggleDeclarationEnabled()` ends up calling `Rule._applyProperties()` which makes a call to the server and returns a promise. Once that promise is resolved, we can be sure our computed object model with rules is up to date (for example, overwritten properties which become active as a result of toggling another property).

We should wait for that to finish before dispatching `updateRules(this.elementStyle.rules)` on the next line.

There's an `onChanged` callback that can be declared on `ElementStyle` to be notified when it has completed its computations:

https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/models/element-style.js#75-77

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Razvan Caliman [:rcaliman] from comment #4)

Comment on attachment 9036447 [details] [diff] [review]
1519988.patch [1.0]

Review of attachment 9036447 [details] [diff] [review]:

::: devtools/client/inspector/rules/components/Declaration.js
@@ +38,5 @@

 });

}

• onToggleDeclarationEnabledClick(event) {
• event.stopPropagation();

Is this event.stopPropagation(); necessary?

Yes. We actually end up calling the function twice if we don't stop the event from bubbling.

::: devtools/client/inspector/rules/components/Declarations.js
@@ +15,5 @@

class Declarations extends PureComponent {
static get propTypes() {
return {
declarations: PropTypes.arrayOf(PropTypes.shape(Types.declaration)).isRequired,

•  onToggleDeclarationEnabled: PropTypes.object.isRequired,
  

Shouldn't this be PropTypes.func.isRequired ?

Good catch and thanks!

::: devtools/client/inspector/rules/models/element-style.js
@@ +332,5 @@

 }

},

/**

• • Toggles the enabled property of the given CSS declaration.

s/property/state

@@ +340,5 @@

• • @param {String} declarationId

• •    The TextProperty id for the CSS declaration.
    

• */
• toggleDeclarationEnabled: function(ruleId, declarationId) {

toggleDeclarationEnabled is very wordy and appears in many places in this
patch.

I suggest renaming it to toggleDeclaration which is adequately explicit.
Plus, it follows the naming precedent set by togglePseudoClass.

Fixed.

@@ +341,5 @@

• •    The TextProperty id for the CSS declaration.
    

• */
• toggleDeclarationEnabled: function(ruleId, declarationId) {
• const rule = this.getRule(ruleId);

If rule or declaration can be undefined, perhaps it's best to guard against
those here.

Fixed.

::: devtools/client/inspector/rules/models/text-property.js
@@ +34,5 @@

•    coming from parseDeclarations.
  

*/
function TextProperty(rule, name, value, priority, enabled = true,
invisible = false) {

• this.id = name + "_" + Math.random().toString(36).substr(2, 5);

We seem to have a uuid generator avaialble:
https://searchfox.org/mozilla-central/source/devtools/shared/generate-uuid.js

It does seem to cross the JS/C++ boundary so I don't know if using that
incurs any substantial performance hit.

I wonder if this id is stable across the session (or perhaps that doesn't
matter; the implications are a bit obscure to me right now).

There is at least one place where the list of text properties is rebuilt and
replaced during the same session in Rule.refresh():

https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/
models/rule.js#518-529

This gets called on the element style model by _maybeAddRule():
https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/
models/element-style.js#188

::: devtools/client/inspector/rules/new-rules.js
@@ +144,5 @@

• • @param {String} declarationId

• •     The TextProperty id for the CSS declaration.
    

• */
• onToggleDeclarationEnabled(ruleId, declarationId) {
• this.elementStyle.toggleDeclarationEnabled(ruleId, declarationId);

ElementStyle.toggleDeclarationEnabled() ends up calling
Rule._applyProperties() which makes a call to the server and returns a
promise. Once that promise is resolved, we can be sure our computed object
model with rules is up to date (for example, overwritten properties which
become active as a result of toggling another property).

We should wait for that to finish before dispatching
updateRules(this.elementStyle.rules) on the next line.

There's an onChanged callback that can be declared on ElementStyle to be
notified when it has completed its computations:

https://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/
models/element-style.js#75-77

Fixed.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/83c3a433bd15
Implement toggling on/off of a CSS declaration. r=rcaliman

Comment 7

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/83c3a433bd15