1509620 - Computed style inspector 'best match' is wrong

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[temhawk]

Attachment: inspector-bug.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:62.0) Gecko/20100101 Firefox/62.0

Steps to reproduce:

Select the <span> node in the DOM inspector in the example HTML file attached.


Actual results:

1. The computed styles inspector shows the wrong 'best match'.

The computed color is correct but the 'best match' shows the header styling rule instead of the applied anchor rule ('parent match').

2. The rules inspector shows the header rule's color style as applied while the anchor rule's style is incorrectly shown as crossed out (superseded).

Even if you remove the anchor rule from the stylesheet and remove the !important keyword from the header style, the rules and computed style inspector will show wrong values.


Expected results:

The computed styles inspector should show a>rgb(0, 255, 0) as the best match, and the rules inspector should show the header rule's color property with strikethrough applied and the anchor rule's color property without.

Comment 1

[temhawk]

Attachment: inspector-bug.png

Screenshot.

Comment 2

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

The first question we need to ask here is how the weighting is calculated... the hierarchy looks like this:

<header style="color: #FF0000 !important">
  <a href="#" style="color: #00FF00;">
    <span>foo</span>
  </a>
</header>

The computed view assumes that the !important color overrides it's children but the browser doesn't.

Comment 3

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

So it is clear that <a> tags do not inherit colors so even though the header style contains !important the color is not inherited.

Comment 4

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

A full list of CSS properties... properties that are inherited have yes in the inherited column:

https://www.w3.org/TR/CSS22/propidx.html

To understand how we need to deal with this we need to find the special cases where CSS inheritance is blocked e.g. the <a> tag blocks color from being inherited by it's children.

Comment 5

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

@emilio You appear to be the person to ask about this as your name is all over our CSS cascade code.

Don't worry about our DevTools CSS Cascade calculation being completely wrong... there is an issue about that (bug 1535281).

My question is that surely the !important declaration inside <header> should be inherited by the <span> but the <a> tag's style somehow trumps it.

Chrome does the same thing so I am guessing we are missing something here?

Also, are there specific tags that block !important properties like this and, if so, how do we know which ones?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

That's just not how !important works. !important overrides other declarations for a given element, but doesn't affect inheritance.

So in this case the !important will only mean that the <header> will get that color over any other more specific declarations.

Once the <header>s computed color is #ff0000, it inherits to children, but its children can override it with any other declaration that applies to them, regardless of importance.

Comment 7

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

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

That's just not how !important works. !important overrides other declarations for a given element, but doesn't affect inheritance.

So in this case the !important will only mean that the <header> will get that color over any other more specific declarations.

Once the <header>s computed color is #ff0000, it inherits to children, but its children can override it with any other declaration that applies to them, regardless of importance.

Of course that is the case... it's been a long day ;)

Comment 8

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

The Problem

In devtools/server/actors/inspector/css-logic.js the CSS cascade is currently calculated like this (descending order):

1. User or author // normal user rules are level with author rules... wrong.
2. Inline style, normal weight // inline style is part of the specificity calculation... this shouldn't be here.
3. Inline style, !important // inline style is part of the specificity calculation... this shouldn't be here.
4. Non-inline style !important // From which stylesheet? They all have different priorities.
5. Important rules // From which stylesheet? They all have different priorities.
6. Non-important rules // From which stylesheet? They all have different priorities.
7. Specificity
8. Sheet Index
9. Rule Line
10. Rule Column

It should be clear from my comments above that our cascade algorithm is not correct.

What we should have been doing

According to https://www.w3.org/TR/CSS2/cascade.html#cascading-order we should have been doing it like this (descending order):

1. User & !important
2. Author & !important
3. Author, normal weight
4. User, normal weight
5. User-Agent, normal weight
6. specificity
7. Sheet Index
8. Rule Line
9. Rule Column

What we should do now

Now according to https://www.w3.org/TR/css-cascade-3/#cascading (which platform follows now) and https://www.w3.org/TR/css-cascade-4/#cascading we should now be doing this (descending order):

1. Transition declarations [css-transitions-1]
2. User-Agent & !important
3. User & !important
4. Author & !important
5. CSS Animations, @keyframes
6. Author, normal weight
7. User, normal weight
8. User-Agent, normal weight
9. specificity
10. Sheet Index - Declarations from imported style sheets are ordered as if their style sheets were substituted in place of the @import rule.
11. Rule Line
12. Rule Column

CSS Animations Example

h2 {
  animation: a 3s;
}
@keyframes a {
  0% {
    color:red;
  }
  100% {
    color:lime;
  }
}

CSS Transition Example

h2 {
  transition: color 0.5s ease;
}

h2:hover {
  color: purple;
}

Comment 10

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

We also pay no attention to the distance an inherited style is away from a node. This means that all inherited styles are classed as equal so the rule closest to the end of the stylesheet wins.

The closest parent is what we need when there are only inherited styles.

Comment 11

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

Attachment: Bug 1509620 - Computed style inspector CSS cascade calculation is wrong r?ladybenko!

Try

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

Summary

!!Comparing numerous complex websites such as github, facebook, cnn etc. the cascade now matchers that of Chrome so we are in a much better place.!!

According to https://www.w3.org/TR/css-cascade-3/#cascading (which platform follows now) and https://www.w3.org/TR/css-cascade-4/#cascading we should now be doing this (descending order):

• Transition declarations
• User-Agent & !important
• User & !important
• Author & !important
• CSS Animations, @keyframes
• Author, normal weight
• User, normal weight
• User-Agent, normal weight
• specificity
• Sheet Index
• Rule Line
• Rule Column

We are only dealing with CSS selectors here so we can safely drop Transition declarations and CSS Animations because their presence here is irrelevant when it comes to the CSS cascade information we display in the computed view.

This leaves us with:

• User-Agent & !important
• User & !important
• Author & !important
• Author, normal weight
• User, normal weight
• User-Agent, normal weight
• specificity
• Sheet Index
• Rule Line
• Rule Column

Changes

• References to content stylesheets have been changed to author stylesheet to closely match the technical terms author, user and agent stylesheets.
• Simplified and modernized a bunch of for loops to make the code easier to understand.
• Previous to these changes all matching parent rules were classed as equal e.g. color on the body tag was equal to color on a node's immediate container. We now use the distance variable to tell how close a rule is to the current node. This is the highest qualifier in our cascade calculation.
• The _agentSheet, _authorSheet and _userSheet properties are now used to obtain a sheets origin.
• elementStyle was renamed to inlineStyle in order to correctly identify the rule's origin.
• We used to sort the matchedSelectors to move rules with STATUS.MATCHED above STATUS.PARENT_MATCH but this is unnecessary now that we have the distance property so we no longer do this.
• The compareTo() method has been updated to match https://www.w3.org/TR/css-cascade-3/#cascading (which platform follows now) and https://www.w3.org/TR/css-cascade-4/#cascading. It has also been simplified and made far less prone to error.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b0704b4bf50
Computed style inspector CSS cascade calculation is wrong r=ladybenko

Comment 13

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/5b0704b4bf50