979574 - [rule-view] important CSS property inherited from parent causes property with the same name on the element to incorrectly be marked as overridden

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[efpies]

Attachment: Снимок экрана 2014-03-05 в 1.56.42.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20140212131424

Steps to reproduce:

Container have the line-height property set with the !important modifier.
The child have the line-height property set without modifiers.

http://jsfiddle.net/cWLF7/


Actual results:

Firebug strikes child's line-height property as overriden. Parent's property is not striked.
However, child's line-height is actually used, and changing parent's property in the Firebug don't affect on text positioning.


Expected results:

Parent's !important property is used as having the highest priority.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Unfortunately, your expected result doesn't match what the CSS spec says to do, or the behavior of any browser.  !important only affects what the specified value is, and hence the computed value.  That's what gets inherited if the child inherits at all, but if the child has a different specified value it doesn't matter what was specified on the parent.

> Firebug strikes child's line-height property as overriden.

Sounds like a bug in Firebug...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Though I just checked, and Firebug does not in fact strike out the "line-height: 3px;" bit, or in fact anything in its style view that I can see...

Comment 3

[Brian Grinstead [:bgrins]]

This is an issue in which the built in developer tools (not Firebug) are showing the wrong property as being active in the rule view.

Comment 4

[Brian Grinstead [:bgrins]]

I believe this is happening due to the markOverridden function being too aggressive: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/rule-view.js#328.  It should be checking to make sure the property is not inherited before marking the earlier as overridden.  Even nicer would be if the getApplied() call from the style actor knew which properties would be overridden.

Comment 6

[Brian Grinstead [:bgrins]]

Easy test case - inspect the span on this page:

data:text/html,<style type="text/css">div{line-height: 1.25!important; } span {line-height: 1}</style><div><span>hi</span></div>

Comment 7

[Tom Tromey :tromey]

Attachment: don't let inherited properties override with !important

Comment 8

[Tom Tromey :tromey]

The test likely depends on the as-authored series.

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8667533 [details] [diff] [review]
don't let inherited properties override with !important

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

Took me a while to step through all the conditions in markOverridden.. Feel like that's some information that we should be able to fetch from the platform  more reliably than computing it all ourselves.  r+ though assuming all other tests are green, and I'd say let's ship it ASAP

::: devtools/client/styleinspector/test/browser_ruleview_mark_overridden_07.js
@@ +9,5 @@
> +
> +const TEST_URI = `
> +  <style type='text/css'>
> +    #testid {
> +      background-color: #f0c;

If you use margin instead of background-color this patch could be landed now, independently from the as-authored series

Comment 10

[Tom Tromey :tromey]

(In reply to Brian Grinstead [:bgrins] from comment #9)

> If you use margin instead of background-color this patch could be landed
> now, independently from the as-authored series

Excellent idea.

Comment 11

[Tom Tromey :tromey]

Attachment: don't let inherited properties override with !important

Updated to use margin properties instead.

Comment 12

[Tom Tromey :tromey]

Attachment: don't let inherited properties override with !important

This time, a clean patch against fx-team.

Comment 13

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=39d373393550

Comment 14

[Tom Tromey :tromey]

Attachment: don't let inherited properties override with !important

The expected results are different before as-authored.

Comment 15

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9eceba31808a

Comment 16

[Tom Tromey :tromey]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d80a181488a6

Comment 17

[Pulsebot]

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

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/178f1ddd2f6a