Closed Bug 1420946 Opened 2 years ago Closed 2 years ago

stylo: CSS styling inheritance

Categories

(Core :: CSS Parsing and Computation, defect)

57 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- wontfix
firefox58 --- fixed
firefox59 --- fixed

People

(Reporter: halbur_josh, Assigned: xidorn)

Details

(Keywords: regression)

Attachments

(6 files, 1 obsolete file)

Attached file issue example.html (obsolete) —
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)
happens only with layout.css.servo.enabled==true
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Summary: CSS styling inheritance → stylo: CSS styling inheritance
Flags: needinfo?(miket)
Attached file 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.
Attachment #8932123 - Attachment is obsolete: true
Flags: needinfo?(miket)
Attached file tc-class.html
I get expected behavior using class selectors, rather than ID.
I get unexpected behavior combining both class and id selectors, if the ID one comes first in the tree.
(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?
I think I know where the problem is.
Assignee: nobody → xidorn+moz
Flags: needinfo?(xidorn+moz)
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!
Attachment #8933119 - Flags: review?(emilio) → review+
Attached file Servo PR
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)
Pushed by xquan@mozilla.com:
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 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
Attachment #8933119 - Flags: approval-mozilla-release?
Attachment #8933119 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/a0e6a7519ed1
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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+.
Attachment #8933119 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
(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
Flags: qe-verify-
I think it's safe to say that we aren't going to be taking this in 57 at this point.
Attachment #8933119 - Flags: approval-mozilla-release?
You need to log in before you can comment on or make changes to this bug.