Rule view no longer marks changed properties

RESOLVED FIXED in Firefox 34

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: miker, Assigned: miker)

Tracking

({regression})

unspecified
Firefox 34
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

STR:
1. Change any property value in the rule view.

A changed value should have a green border to the left.

----------

In rule-view.js we have a store of changed properties. We use this in update() to set an attribute, "dirty", on the property. The dirty attribute should trigger the .ruleview-property[dirty] CSS selector.
Keywords: regression
Assignee: nobody → mratcliffe
Comment hidden (obsolete)
Status: NEW → ASSIGNED
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Created attachment 8468345 [details] [diff] [review]
rule-view-not-marking-changes-1040701.patch

bgrins really wanted you to take a look over this. It turns out that I needed to make more changes than expected to fix the highlighting of changed rules.

So, a quick summary of all previous comments:
1. The storing of user-entered values looks like it has been broken for some time so e.g. tooltips didn't mark things changed properly. I have fixed tests so that they can detect this and also fixed the store, which had numerous issues.

2. User entered properties are now cleared on navigate, as they should be.

3. The rule view was coercing user typed colors into html. We should be keeping user entered values and not co-ercing them. A few tests were expecting the non-user entered values when they should have expected user-entered values.

4. Because applyProperties is often changed it was very easy to break the marking of changed properties. moving this to the TextProperty object made sense as it is rarely changed and it simplifies the code.

5. Tabbing through anything with url=("...") would show changes as the computed version of this property is url("..."). This meant changing the rendering of urls in the output parser to use quotes.

6. Tabbing through any property containing a color was marking things as changed because we were comparing non default color types with default color types.

7. Changing names from color to e.g. background-color didn't flag changes.

8. Changes were not marked from pickers. We now call commit() when the picker is hidden unless it was hidden as a result of pressing escape. The color picker now returns the default color type where possible. We also call commit instead of just preview when selecting colors using the eye dropper.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=ad8a1e420438
Attachment #8459476 - Attachment is obsolete: true
Attachment #8468345 - Flags: review?(pbrosset)
Comment on attachment 8468345 [details] [diff] [review]
rule-view-not-marking-changes-1040701.patch

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

LGTM. We might want to uplift this to Aurora if it doesn't have huge dependencies.

::: browser/devtools/shared/widgets/Tooltip.js
@@ +891,5 @@
>          if (!this.eyedropperOpen) {
>            this.activeSwatch = null;
>          }
>        });
> +      this.tooltip.once("hiding", () => {

Can you move this block before the this.tooltip.once("hidden", ...) block? Since it occurs before, it would make more sense.

Also, I think both hiding and hidden event listeners would require some comments. It's not immediately straightforward why we listen for both these events when the tooltip is shown. Can you add a comment line for both?

@@ +892,5 @@
>            this.activeSwatch = null;
>          }
>        });
> +      this.tooltip.once("hiding", () => {
> +        if (!this._reverted && !this.eyedropperOpen) {

I'm a little bit uncomfortable accessing eyedropperOpen here, since this is a generic parent class that shouldn't know about the internal implementation of sub-classes. Having said this, this was here before your changes, and I don't have a better idea right now.

@@ +896,5 @@
> +        if (!this._reverted && !this.eyedropperOpen) {
> +          this.commit();
> +        }
> +        this._reverted = false;
> +        return true;

Why returning true is needed here?

::: browser/devtools/styleinspector/rule-view.js
@@ +2298,5 @@
>      this.nameSpan.textProperty = this.prop;
>  
> +    // If the value is a color property we need to put it through the parser
> +    // so that colors can be coerced into the default color type. This prevents
> +    // us from thinking that when colors are coerced they has been changed by

s/has/have

@@ +2767,5 @@
>        firstValue: firstValue
>      };
>    },
>  
> +  _applyNewValue: function(aValue, markChanged=true) {

Can you add a jsdoc comment block here to explain what markChanged does and in which rare cases is it not true.
Attachment #8468345 - Flags: review?(pbrosset) → review+
Created attachment 8470823 [details] [diff] [review]
rule-view-not-marking-changes-1040701.patch

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #16)
> Comment on attachment 8468345 [details] [diff] [review]
> rule-view-not-marking-changes-1040701.patch
> 
> Review of attachment 8468345 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM. We might want to uplift this to Aurora if it doesn't have huge
> dependencies.
> 
> ::: browser/devtools/shared/widgets/Tooltip.js
> @@ +891,5 @@
> >          if (!this.eyedropperOpen) {
> >            this.activeSwatch = null;
> >          }
> >        });
> > +      this.tooltip.once("hiding", () => {
> 
> Can you move this block before the this.tooltip.once("hidden", ...) block?
> Since it occurs before, it would make more sense.
> 

Agreed, done.

> Also, I think both hiding and hidden event listeners would require some
> comments. It's not immediately straightforward why we listen for both these
> events when the tooltip is shown. Can you add a comment line for both?
> 

Added comments.

> @@ +892,5 @@
> >            this.activeSwatch = null;
> >          }
> >        });
> > +      this.tooltip.once("hiding", () => {
> > +        if (!this._reverted && !this.eyedropperOpen) {
> 
> I'm a little bit uncomfortable accessing eyedropperOpen here, since this is
> a generic parent class that shouldn't know about the internal implementation
> of sub-classes. Having said this, this was here before your changes, and I
> don't have a better idea right now.
> 

Yeah, me too... seems like the only simple option at the moment though.

> @@ +896,5 @@
> > +        if (!this._reverted && !this.eyedropperOpen) {
> > +          this.commit();
> > +        }
> > +        this._reverted = false;
> > +        return true;
> 
> Why returning true is needed here?
> 

It isn't, removed.

> ::: browser/devtools/styleinspector/rule-view.js
> @@ +2298,5 @@
> >      this.nameSpan.textProperty = this.prop;
> >  
> > +    // If the value is a color property we need to put it through the parser
> > +    // so that colors can be coerced into the default color type. This prevents
> > +    // us from thinking that when colors are coerced they has been changed by
> 
> s/has/have
> 

Changed.

> @@ +2767,5 @@
> >        firstValue: firstValue
> >      };
> >    },
> >  
> > +  _applyNewValue: function(aValue, markChanged=true) {
> 
> Can you add a jsdoc comment block here to explain what markChanged does and
> in which rare cases is it not true.

Done
Attachment #8468345 - Attachment is obsolete: true
Attachment #8470823 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/35800e6b8a7b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34

Updated

4 years ago
Blocks: 940507
You need to log in before you can comment on or make changes to this bug.