942297 - [ruleview] Styles do not refresh after being updated from the rule view, then from an inline style

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Brian Grinstead [:bgrins]]

We stumbled on a bug with the rule view when looking at Bug 726427.  Turns out it is not related to that bug, and is easily reproducible on any style attribute.

STR:
* Open a page with an element that has a style attribute, like this: http://jsbin.com/OfuGIfOt/1
* Change the value of this style from the rule view (set it to height: 500px, for instance)
* Change the value of the style from the style attribute in the markup view

Notice that the properties on the rule view no longer change.  Even when reselecting the element, or even reloading the page, the rule view properties remain stuck.

This is not a bug on Aurora (27).

Comment 1

[Paul Rouget [:paul]]

(In reply to Brian Grinstead [:bgrins] from comment #0)
> the rule view properties remain stuck.

Not if you change it from the rule view, right? Only from the markup view.

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to Paul Rouget [:paul] from comment #1)
> (In reply to Brian Grinstead [:bgrins] from comment #0)
> > the rule view properties remain stuck.
> 
> Not if you change it from the rule view, right? Only from the markup view.

Correct - changing it from the rule view *does* update the style attribute in the markup view, but changing it in the markup view *does not* update it in the rule view.

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> (In reply to Paul Rouget [:paul] from comment #1)
> > (In reply to Brian Grinstead [:bgrins] from comment #0)
> > > the rule view properties remain stuck.
> > 
> > Not if you change it from the rule view, right? Only from the markup view.
> 
> Correct - changing it from the rule view *does* update the style attribute
> in the markup view, but changing it in the markup view *does not* update it
> in the rule view.

I should also point out that changing the style attribute from JS behaves the same as if you change it from the markup view - the text in the markup view updates to the new style, but the rule view does not.

Comment 4

[Patrick Brosset <:pbro>]

Aurora (28):
* Open http://jsbin.com/OfuGIfOt/1
* Inspect the element
* Change its height in the rule view to 500px
* Reload the page

==> The rule view still shows 500px for the element, but the markup-view is back to the correct value.

Comment 5

[Patrick Brosset <:pbro>]

After testing on Release (26) and noticing that it worked correctly there, I went looking for the change that introduced this bug.
The lucky winner is bug 929384 which dealt with color swatches in the rule view. Incidentally, I reviewed this bug's patch.
In any case, what it shows is that this scenario isn't tested automatically, so it was easy to overlook it.

Mike: You replaced this:

  getProperty: function(aStyle, aName, aDefault) {
    let key = this.getKey(aStyle);
    let entry = this.map.get(key, null);

    if (entry && aName in entry) {
      let item = entry[aName];
      if (item != aDefault) {
        delete entry[aName];
        return aDefault;
      }
      return item;
    }
    return aDefault;
  },

with this:

  getProperty: function(aStyle, aName, aDefault) {
    let key = this.getKey(aStyle);
    let entry = this.map.get(key, null);

    if (entry && aName in entry) {
      let item = entry[aName];
      return item || aDefault;
    }
    return aDefault;
  },

in rule-view.js / UserProperties class
Was it linked to bug 929384? Or an unrelated simplification that was made while working on this bug?
We test if aName is in entry first, so item can never be null I believe.

Comment 7

[Patrick Brosset <:pbro>]

Attachment: bug942297-ruleview-styles-not-updating V1.patch

This patch simply reverts a couple of lines in rule-view.js that were changed as explained in comment 5.
This seems to have been an unrelated change and the fix done in bug 929384 continues to work after reverting the lines.

The patch also contains a new test.

Ongoing try build : https://tbpl.mozilla.org/?tree=Try&rev=6e2331f4e3a2

Comment 8

[Heather Arthur [:harth]]

Comment on attachment 8359234 [details] [diff] [review]
bug942297-ruleview-styles-not-updating V1.patch

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

I can't say I understand the logic, but given it's just a revert, looks good.

Comment 9

[Patrick Brosset <:pbro>]

Fixed in fx-team
https://hg.mozilla.org/integration/fx-team/rev/6f3f8a900a0b

Comment 10

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

(In reply to Patrick Brosset [:pbrosset] from comment #5)
> After testing on Release (26) and noticing that it worked correctly there, I
> went looking for the change that introduced this bug.
> The lucky winner is bug 929384 which dealt with color swatches in the rule
> view. Incidentally, I reviewed this bug's patch.
> In any case, what it shows is that this scenario isn't tested automatically,
> so it was easy to overlook it.
> 
> Mike: You replaced this:
> 
>   getProperty: function(aStyle, aName, aDefault) {
>     let key = this.getKey(aStyle);
>     let entry = this.map.get(key, null);
> 
>     if (entry && aName in entry) {
>       let item = entry[aName];
>       if (item != aDefault) {
>         delete entry[aName];
>         return aDefault;
>       }
>       return item;
>     }
>     return aDefault;
>   },
> 
> with this:
> 
>   getProperty: function(aStyle, aName, aDefault) {
>     let key = this.getKey(aStyle);
>     let entry = this.map.get(key, null);
> 
>     if (entry && aName in entry) {
>       let item = entry[aName];
>       return item || aDefault;
>     }
>     return aDefault;
>   },
> 
> in rule-view.js / UserProperties class
> Was it linked to bug 929384? Or an unrelated simplification that was made
> while working on this bug?
> We test if aName is in entry first, so item can never be null I believe.

If the following in the rule view still works then it is fine:
- Change a property
- The left border should be marked in green to show that it was changed
- Inspect a second element that uses the same style rule
- The left border should still be marked in green

We should have a test for this if we don't already.

Comment 11

[Patrick Brosset <:pbro>]

> If the following in the rule view still works then it is fine:
> - Change a property
> - The left border should be marked in green to show that it was changed
> - Inspect a second element that uses the same style rule
> - The left border should still be marked in green
Yep, that scenario still works, so we're good.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6f3f8a900a0b