1017115 - [rule view] Showing inline styles as inherited when opening toolbox via 'inspect element'

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Brian Grinstead [:bgrins]]

Attachment: inspectinherited.gif

Saw some weird behavior when working on Bug 935803.

STR:

Open this page:
data:text/html,<blockquote style='color:red;' type=cite><pre _moz_quote=true>fun <a href='foo' style='color:orange'>to</a> inspect</code></blockquote>

Right click on the orange link and select Inspect Element

See that the color is showing up as "inherited from blockquote" - it should be inline (which you can see when reselecting the element).

See attached gif

Comment 1

[Brian Grinstead [:bgrins]]

Seems to be related to the highlight() function in ruleview which gets called twice on initial load (once for body, once for the a tag).  Could be a timing problem.

Comment 2

[Brian Grinstead [:bgrins]]

This is a pretty important bug to fix in that it shows confusing / invalid data about the node styles, and right click -> inspect element is a pretty common way to access the tools.

Comment 3

[Patrick Brosset <:pbro>]

It looks like this bug might be a duplicate of bug 1021040 which also occurs when using the right-click/inspect element.
It definitely looks like a race condition of some sorts.

Comment 4

[Patrick Brosset <:pbro>]

Attachment: bug1017115-too-many-rule-headers.patch

Brian, this fix seems to work (also for the STR described in bug 1021040) but I haven't spent too much time looking at the code.
What do you think of this approach?

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8436883 [details] [diff] [review]
bug1017115-too-many-rule-headers.patch

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

I think this is the right check - can you think of a way to add a test for this?

::: browser/devtools/styleinspector/rule-view.js
@@ +1449,1 @@
>        this._showEmpty();

There is a check a few lines above that says

    if (this._elementStyle) {
      delete this._elementStyle;
    }

However, this line is immediately after calling clear() which sets this._elementStyle = null.  So that if statement can be removed

@@ +1460,2 @@
>      }).then(() => {
>        // A new node may already be selected, in which this._elementStyle will

Seems like maybe this code has changed over time since this comment was/is misleading. _elementStyle will never really be null (it is cleared, but only right before it is set again inside of this function).

Comment 6

[Patrick Brosset <:pbro>]

Thanks for the review, I'm working on a test now, and have removed the delete this._elementStyle if statement.

(In reply to Brian Grinstead [:bgrins] from comment #5)
> Seems like maybe this code has changed over time since this comment was/is
> misleading. _elementStyle will never really be null (it is cleared, but only
> right before it is set again inside of this function).
this._elementStyle is set to null in clear() and set again in highlight, but if the highlighted element is null, then the function returns before it sets this._elementStyle again, so there is a chance it will be null, but the comment was misleading anyway. What I've done is remove it altogether and only kept the check for this._viewedElement === aElement.

Comment 8

[Patrick Brosset <:pbro>]

Attachment: bug1017115-too-many-rule-headers v2.patch

v2 - Cleaned up the code a little bit, as suggested. And added a test.
Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=87f8a02b2030

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8437596 [details] [diff] [review]
bug1017115-too-many-rule-headers v2.patch

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

Everything looks good, r=me with the localized strings replaced

::: browser/devtools/styleinspector/rule-view.js
@@ +1456,2 @@
>      }).then(() => {
> +      if (this._viewedElement === aElement) {

So is there a case where this._elementStyle will be null here?  I guess aElement will never be null at this point, so if this._viewedElement === aElement then that will not be null either, which means that this._elementStyle would have been set earlier in this function.

There is such a strong tie between _viewedElement and _elementStyle that I think there should probably be a refactor in which _viewedElement is tuend into a getter that fetches it from _elementStyle.  Given how closely the two are used in relation to each other, refactoring seems more likely to cause a regression, so I'd prefer we fix this bug as it and deal with that separately.

::: browser/devtools/styleinspector/test/browser_ruleview_content_02.js
@@ +47,5 @@
> +
> +  let headers = [...doc.querySelectorAll(".ruleview-header")];
> +  is(headers.length, 3, "There are 3 headers for inherited rules");
> +
> +  is(headers[0].textContent, "Inherited from p", "The first header is correct");

This is a localized string (rule.inheritedFrom) in styleinspector.properties.  Can you grab the string from there instead of hardcoding it (see browser_ruleview_media-queries.js)?

Comment 10

[Patrick Brosset <:pbro>]

Attachment: bug1017115-too-many-rule-headers v3.patch

Thanks for the review

(In reply to Brian Grinstead [:bgrins] from comment #9)
> Comment on attachment 8437596 [details] [diff] [review]
> bug1017115-too-many-rule-headers v2.patch
> 
> Review of attachment 8437596 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Everything looks good, r=me with the localized strings replaced
> 
> ::: browser/devtools/styleinspector/rule-view.js
> @@ +1456,2 @@
> >      }).then(() => {
> > +      if (this._viewedElement === aElement) {
> 
> So is there a case where this._elementStyle will be null here?  I guess
> aElement will never be null at this point, so if this._viewedElement ===
> aElement then that will not be null either, which means that
> this._elementStyle would have been set earlier in this function.
> 
> There is such a strong tie between _viewedElement and _elementStyle that I
> think there should probably be a refactor in which _viewedElement is tuend
> into a getter that fetches it from _elementStyle.  Given how closely the two
> are used in relation to each other, refactoring seems more likely to cause a
> regression, so I'd prefer we fix this bug as it and deal with that
> separately.
I agree, let's fix this separately. The function isn't making it easy to follow and could use a refactor.
> ::: browser/devtools/styleinspector/test/browser_ruleview_content_02.js
> @@ +47,5 @@
> > +
> > +  let headers = [...doc.querySelectorAll(".ruleview-header")];
> > +  is(headers.length, 3, "There are 3 headers for inherited rules");
> > +
> > +  is(headers[0].textContent, "Inherited from p", "The first header is correct");
> 
> This is a localized string (rule.inheritedFrom) in
> styleinspector.properties.  Can you grab the string from there instead of
> hardcoding it (see browser_ruleview_media-queries.js)?
Done.

Comment 11

[Patrick Brosset <:pbro>]

https://hg.mozilla.org/integration/fx-team/rev/2114c3579d37

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2114c3579d37

Comment 13

[Catalin Suciu [:csuciu]]

Verified as fixed on Nightly 34.0a1 (2014-08-28)
The color isn't showing up as "inherited from blockquote"