986201 - Computed style trace can't be shown for properties when they have multiple inherited inline styles

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Sebastian Zartner [:sebo]]

Steps to reproduce the problem:
1. Inspect the "List item" at https://getfirebug.com/tests/manual/issues/7269/issue7269.html
2. Switch to the Computed side panel
3. Click the arrow besides the 'color' property

=> Nothing happens. You would expect to see the two values 'blue' and 'green' defined by its <ul> and <div> ancestor.

See the related Firebug issue http://code.google.com/p/fbug/issues/detail?id=7269.
There this error was thrown:

Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [inIDOMUtils.getSpecificity]
resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/styleinspector/css-logic.js:1443

Sebastian

Comment 1

[Paul Silaghi, QA [:pauly]]

Confirmed 31.0a1 (2014-03-21), win 7 x64

Comment 2

[Sebastian Zartner [:sebo]]

That's still happening on 33.0a1 (2014-07-15), just the line has changed to resource://gre/modules/devtools/styleinspector/css-logic.js:1486:24.

Sebastian

Comment 3

[Sami Jaktholm]

Attachment: fix-bug-986201.patch

DOMUtils.getSpecificity expects an object with CSSStyleRule interface which element.style doesn't implement.

As specificity of element style is constant[1], this patch just returns the specificity right away without calling DOMUtils.getSpecificity. The patch also includes an unit test to ensure specificity is correctly handled for element styles.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=2170adb746f6

[1] http://www.w3.org/TR/CSS2/cascade.html#specificity

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8459129 [details] [diff] [review]
fix-bug-986201.patch

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

Thanks for the patch!  There is an existing test for css-logic specificity: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/test/browser_styleinspector_csslogic-specificity.js.  I think it would make sense to add the test into that file instead of adding a new one.

Comment 5

[Joe Walker [:jwalker]]

Comment on attachment 8459129 [details] [diff] [review]
fix-bug-986201.patch

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

I wish we'd called this.elementStyle, this._isElementStyle to make it clearer that it's a boolean.
Nit: In addition to Brian's comments, this patch adds some trailing whitespace that we should remove before we commit it.
Thanks, and sorry for the slow review.

Comment 6

[Sami Jaktholm]

Attachment: fix-bug-986201.patch

Thanks for the review. Here's a patch without the trailing whitespace and the test is merged into browser_styleinspector_csslogic-specificity.js.

Will push to try once it stops timing out.

Comment 7

[Sami Jaktholm]

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

Comment 8

[Carsten Book [:Tomcat]]

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

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/a9c0471594e5

Comment 10

[Sebastian Zartner [:sebo]]

I can confirm that it's working now. Thanks!

Sebastian