1420946 - stylo: CSS styling inheritance

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[halbur_josh]

Attachment: issue example.html

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171112125346

Steps to reproduce:

view page where style of LI element is applied by ID where casing between ID attribute and the CSS definition do not match


Actual results:

style defined in CSS file was applied to all LI elements


Expected results:

style should only be applied to the element with the matching ID (case insensitive) or to no elements (since name & ID are supposed to be case sensitive)

Comment 1

[Tooru Fujisawa [:arai]]

happens only with layout.css.servo.enabled==true

Comment 2

[Mike Taylor [:miketaylr]]

Attachment: tc-id.html

Yeah, it looks like selector matching is busted in quirks mode, in certain cases. In this TC (tc-id.html), the 3rd paragraph should not be red.

Comment 3

[Mike Taylor [:miketaylr]]

Attachment: tc-class.html

I get expected behavior using class selectors, rather than ID.

Comment 4

[Mike Taylor [:miketaylr]]

Attachment: tc-class-plus-id-class-first.html

I get unexpected behavior combining both class and id selectors, if the ID one comes first in the tree.

Comment 5

[Mike Taylor [:miketaylr]]

Attachment: tc-class-plus-id-class-second.html

Comment 6

[Mike Taylor [:miketaylr]]

(In reply to Mike Taylor [:miketaylr] (58 Regression Engineering Owner) from comment #2)
> Yeah, it looks like selector matching is busted in quirks mode, in certain
> cases. In this TC (tc-id.html), the 3rd paragraph should not be red.

It seems like when doing case insensitive matching on an ID selector, things get screwed up.

Xidorn, do you know who would be a good person to look at this?

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

I think I know where the problem is.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1420946 - Don't try to share style on quirks mode whenever two elements have different id.

Review commit: https://reviewboard.mozilla.org/r/204092/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204092/

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8933119 [details]
Bug 1420946 - Don't try to share style on quirks mode whenever two elements have different id.

https://reviewboard.mozilla.org/r/204092/#review209594

I think a slighly more robust fix would be to move the check into `may_have_rules_for_id`, and return true if it'd need to be a case-sensitive lookup.


r=me with that (or without if you don't agree, though in that case I'd like to know why). Thanks for fixing this!

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8933119 [details]
Bug 1420946 - Don't try to share style on quirks mode whenever two elements have different id.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204092/diff/1-2/

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Servo PR

Comment 12

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s fe2818678545 -d 43e6105e56e6: rebasing 436751:fe2818678545 "Bug 1420946 - Don't try to share style on quirks mode whenever two elements have different id. r=emilio" (tip)
merging layout/reftests/bugs/reftest.list
merging servo/components/style/stylist.rs
warning: conflicts while merging layout/reftests/bugs/reftest.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a0e6a7519ed1
Don't try to share style on quirks mode whenever two elements have different id. r=emilio

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8933119 [details]
Bug 1420946 - Don't try to share style on quirks mode whenever two elements have different id.

Approval Request Comment
[Feature/Bug causing the regression]: stylo
[User impact if declined]: see some weird behavior on pages with quirks mode
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it just makes an optimization more conservative so that it wouldn't be applied incorrectly for pages on quirks mode
[String changes made/needed]: n/a

Comment 15

[Cristian Brindusan [:cbrindusan]]

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

Comment 16

[Gerry Chang [:gchang]]

Comment on attachment 8933119 [details]
Bug 1420946 - Don't try to share style on quirks mode whenever two elements have different id.

Fix a stylo issue. Beta58+.

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/9b41fc04fab1

Comment 18

[Ryan VanderMeulen [:RyanVM]]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #17)
> https://hg.mozilla.org/releases/mozilla-beta/rev/9b41fc04fab1

https://hg.mozilla.org/releases/mozilla-beta/rev/306f3de7c409 was also uplifted for this bug.

Comment 19

[Cornel Ionce [:noni] [Hubs QA]]

(In reply to Xidorn Quan [:xidorn] UTC-6 (less responsive Nov 5 ~ Dec 16) from comment #14)
> [Is this code covered by automated tests?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no

Marking as qe-verify- based on the automated coverage

Comment 20

[Ryan VanderMeulen [:RyanVM]]

I think it's safe to say that we aren't going to be taking this in 57 at this point.