Closed Bug 1017115 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 33

People

(Reporter: bgrins, Assigned: pbro)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached image 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
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.
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.
OS: Mac OS X → All
Priority: -- → P2
Hardware: x86 → All
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.
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?
Flags: needinfo?(bgrinstead)
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).
Attachment #8436883 - Flags: feedback+
Flags: needinfo?(bgrinstead)
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.
Duplicate of this bug: 1021040
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
Assignee: nobody → pbrosset
Attachment #8436883 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8437596 - Flags: review?(bgrinstead)
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)?
Attachment #8437596 - Flags: review?(bgrinstead) → review+
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.
Attachment #8437596 - Attachment is obsolete: true
Attachment #8438425 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2114c3579d37
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
QA Whiteboard: [good first verify]
Verified as fixed on Nightly 34.0a1 (2014-08-28)
The color isn't showing up as "inherited from blockquote"
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.