1040701 - Rule view no longer marks changed properties

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: rule-view-not-marking-changes-1040701.patch

Really simple patch as that moves the updating of the collection of changed rules to the TextProperty where it should have been all along.

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8458714 [details] [diff] [review]
rule-view-not-marking-changes-1040701.patch

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

A couple of notes from using it (not sure how much of this it used to do before regressing):

1) If I just tab through the props without changing anything, they turn green.
2) Also when I reload the page some of them stay green

Can you also add a test (and/or update browser_ruleview_edit-property_02.js so that it fails without the patch applied)?

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: rule-view-not-marking-changes-1040701.patch

Works fine now. The test also now fails without the patch applied.

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Try:
https://tbpl.mozilla.org/?tree=Try&rev=a016eb8fa8ca

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #4)
> Try:
> https://tbpl.mozilla.org/?tree=Try&rev=a016eb8fa8ca

Looks like there is a test failure in browser/devtools/styleinspector/test/browser_ruleview_colorpicker-and-image-tooltip_02.js (I see it locally, too).

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: rule-view-not-marking-changes-1040701.patch

So, it seems that a couple of tests were "expecting" the wrong values and receiving them.

When a value is typed in the rule view we keep the value that was typed, even if it is a color in a different format than the default color type.

The color was being coerced into the default format when chosen by the color picker. The color picker now enters rgba colors as we are keeping the "user typed" value.

If you would like us to coerce the value we can easily do this from either the rule-view or the tooltip... log a bug and I would be happy to do it.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=89d20f35385c

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8459336 [details] [diff] [review]
rule-view-not-marking-changes-1040701.patch

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

I still see an issue where it shows up as changed when simply tabbing through a background rule (you can see this on the body element at https://www.mozilla.org/ for instance), but it is much better than the current behavior.

I'd like a some feedback from Patrick on this patch, specifically about moving the store.userProperties.setProperty call out of applyProperties and into the TextProperty object.  If you have an opinion on the colors as authored thing in Comment 6 feel free to chime in also - IMO it is nicer for a user to see "red" right after typing red instead of seeing #f00, so I like that change.

::: browser/devtools/styleinspector/rule-view.js
@@ +1034,5 @@
> +  setValue: function(aValue, aPriority, aCommit=false) {
> +    let store = this.rule.elementStyle.store;
> +
> +    if (aCommit) {
> +      store.userProperties.setProperty(this.rule.style, this.name, aValue);

Shouldn't this also happen in setName?

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Brian Grinstead [:bgrins] from comment #7)
> Comment on attachment 8459336 [details] [diff] [review]
> rule-view-not-marking-changes-1040701.patch
> 
> Review of attachment 8459336 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I still see an issue where it shows up as changed when simply tabbing
> through a background rule (you can see this on the body element at
> https://www.mozilla.org/ for instance), but it is much better than the
> current behavior.
> 

Hmm, I will look into that.

> I'd like a some feedback from Patrick on this patch, specifically about
> moving the store.userProperties.setProperty call out of applyProperties and
> into the TextProperty object.


When I created the store, putting it in applyProperties made sense but this happens to be where we are most likely to make changes and break this behavior. I think that having this [almost] as a property of TextProperty makes sense to me.

Patrick is away for two weeks but we can revisit this another time.

> If you have an opinion on the colors as
> authored thing in Comment 6 feel free to chime in also - IMO it is nicer for
> a user to see "red" right after typing red instead of seeing #f00, so I like
> that change.
> 

When I was over the rule view we had a lot of users complained that they wanted to see what they had typed. The fact that it was coercing all properties was a regression.

> ::: browser/devtools/styleinspector/rule-view.js
> @@ +1034,5 @@
> > +  setValue: function(aValue, aPriority, aCommit=false) {
> > +    let store = this.rule.elementStyle.store;
> > +
> > +    if (aCommit) {
> > +      store.userProperties.setProperty(this.rule.style, this.name, aValue);
> 
> Shouldn't this also happen in setName?

Yes, it should... I will change that.

To me this is a big issue as I tend to change a bunch of properties and then use the green highlighting to know which properties I need to update in the source.

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: rule-view-not-marking-changes-1040701.patch

(In reply to Brian Grinstead [:bgrins] from comment #7)
> Comment on attachment 8459336 [details] [diff] [review]
> rule-view-not-marking-changes-1040701.patch
> 
> Review of attachment 8459336 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I still see an issue where it shows up as changed when simply tabbing
> through a background rule (you can see this on the body element at
> https://www.mozilla.org/ for instance), but it is much better than the
> current behavior.
> 

Turns out this was an issue with any declaration containing a color.

On property creation we were saving the actual CSS text as the base value. To check for changes we were comparing the original text with the parsed text. Because the color is coerced to the default color type in the rule view we assumed that the user had changed the value so we marked it as changed.

The fix was to parse the property names when saving the original CSS text and use the parsed value.

> I'd like a some feedback from Patrick on this patch, specifically about
> moving the store.userProperties.setProperty call out of applyProperties and
> into the TextProperty object.

When I created the store, putting it in applyProperties made sense but this
happens to be where we are most likely to make changes and break this
behavior. I think that having this [almost] as a property of TextProperty
makes sense to me.

Patrick is away for two weeks but we can revisit this another time.

> If you have an opinion on the colors as
> authored thing in Comment 6 feel free to chime in also - IMO it is nicer for
> a user to see "red" right after typing red instead of seeing #f00, so I like
> that change.
> 

When I was over the rule view we had a lot of users complained that they
wanted to see what they had typed so we implemented it this way. The fact that it was coercing user entered properties was a regression.

> ::: browser/devtools/styleinspector/rule-view.js
> @@ +1034,5 @@
> > +  setValue: function(aValue, aPriority, aCommit=false) {
> > +    let store = this.rule.elementStyle.store;
> > +
> > +    if (aCommit) {
> > +      store.userProperties.setProperty(this.rule.style, this.name, aValue);
> 
> Shouldn't this also happen in setName?

TextPropertyEditor._onNameDone() calls this.prop.setValue() so we don't need to save to the store from setName().

Everything is working perfectly now.

Comment 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: rule-view-not-marking-changes-1040701.patch

Added a comment explaining why we parse the declaration:
    // 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
    // the user.

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8459476 [details] [diff] [review]
rule-view-not-marking-changes-1040701.patch

In the absence of pbrosset why don't we ask jwalker for feedback?

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #9)
> > ::: browser/devtools/styleinspector/rule-view.js
> > @@ +1034,5 @@
> > > +  setValue: function(aValue, aPriority, aCommit=false) {
> > > +    let store = this.rule.elementStyle.store;
> > > +
> > > +    if (aCommit) {
> > > +      store.userProperties.setProperty(this.rule.style, this.name, aValue);
> > 
> > Shouldn't this also happen in setName?
> 
> TextPropertyEditor._onNameDone() calls this.prop.setValue() so we don't need
> to save to the store from setName().
> 

When I open up a page with the patch applied and change margin: 0px to padding: 0px (or to any other property name), the green changed color doesn't show up on the left.

Comment 13

[Brian Grinstead [:bgrins]]

Also, changing a color value with the colorpicker does not cause the changed border to show up.  If I click into the field and change the value to the same thing, the changed border does show up.

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8459476 [details] [diff] [review]
rule-view-not-marking-changes-1040701.patch

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

Clearing review (see Comments 12 and 13)

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 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

Comment 16

[Patrick Brosset <:pbro>]

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.

Comment 17

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: 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

Comment 18

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8470823 [details] [diff] [review]
rule-view-not-marking-changes-1040701.patch

https://tbpl.mozilla.org/?tree=Fx-Team&rev=35800e6b8a7b

Comment 19

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://hg.mozilla.org/integration/fx-team/rev/35800e6b8a7b

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/35800e6b8a7b