Closed
Bug 1363778
Opened 7 years ago
Closed 7 years ago
stylo: Implement the class/id case-insensitive matching quirk
Categories
(Core :: CSS Parsing and Computation, enhancement, P2)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bzbarsky, Assigned: SimonSapin)
References
(Blocks 1 open bug)
Details
Spec: https://drafts.csswg.org/selectors/#id-selectors and https://drafts.csswg.org/selectors/#class-html both say that matching is ASCII-case-insensitive in quirks mode. Testcase: <style>#foo { color: green; }</style> <div id="FOO">Should be green</div>
Updated•7 years ago
|
Blocks: stylo-reftest
![]() |
Reporter | |
Comment 1•7 years ago
|
||
Just to scope out the work, there are two main parts here: 1) Fixing selector matching to do case-insensitive matching for ids and classes in quirks mode. 2) Fixing the rulehashes in stylist to do case-insensitive lookups.
Depends on: 1364746
Comment 2•7 years ago
|
||
Simon, you seem to be working on parts of this. How much overlap is there?
Flags: needinfo?(simon.sapin)
Priority: -- → P2
Comment 3•7 years ago
|
||
Oh nevermind, looks like Simon was working on regular case-insensitive matching.
Flags: needinfo?(simon.sapin)
Assignee | ||
Comment 4•7 years ago
|
||
In current code there’s an asymmetry between handling of IDs and classes. In the selectors::Element trait, the impl return the optional ID of an element and the selectors crate does the string (or atom) comparison. For classes, selectors provides the string/atom form the class selector and the impl returns a boolean. I think these should be "unified", picking one of: * The impl does the string comparisons, replacing Element::get_id with Element::has_id. However Servo + Stylo has a number of these impls (I think 5 in total?), so it’s as many places that need to not forget to check for quirks mode. * The selectors crate does the string comparisons, deciding which kind based on a new Element::in_quirks_mode_document method. The selectors crate needs some way to iterate on an element’s classes. An external iterator with the std::iter::Iterator trait is verbose to implement, especially in a generic context. So we could have an internal iterator: an FnMut(&ClassName) callback called for each class. But we need to stop iteration when a match is found so the callback needs to return at least a boolean, but I’d prefer a new `enum Next { Continue, Stop }`. [1] has a trait for internal iterators, but it seems to be unmaintained [2]. [1] https://docs.rs/traverse/ [2] https://github.com/reem/rust-traverse/pull/26 is months old and didn’t get a response.
![]() |
Reporter | |
Comment 5•7 years ago
|
||
One note on the above: We want to minimize the number of FFI calls as we go, and we do NOT want to be doing UTF16-to-UTF8-and-back conversions and allocating string copies. That means we'll probably want a Rust-side no-copy no-conversion ASCII-case-insensitive comparison on atoms.
Assignee | ||
Comment 6•7 years ago
|
||
We already have Atom::eq_ignore_ascii_case: https://github.com/servo/servo/blob/7f80d9cb45/components/style/gecko_string_cache/mod.rs#L241 We probably want to add a fast path that tries `self == other` first.
Comment 7•7 years ago
|
||
(In reply to Simon Sapin (:SimonSapin) from comment #6) > We already have Atom::eq_ignore_ascii_case: > https://github.com/servo/servo/blob/7f80d9cb45/components/style/ > gecko_string_cache/mod.rs#L241 Nice! > We probably want to add a fast path that tries `self == other` first. Pushed https://github.com/servo/servo/pull/17022 for this.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → simon.sapin
Assignee | ||
Comment 8•7 years ago
|
||
https://github.com/servo/servo/pull/17213 is up for review, but it doesn’t seem to fix any test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bd8e7f69cf56046c8fd685a57d0f8db8f43c4e8 . I don’t know if we don’t have relevant tests, or if I did something wrong.
![]() |
Reporter | |
Comment 9•7 years ago
|
||
We have relevant tests. See layout/reftests/bugs/571347-2c.html for example. I don't see changes to stylist or SelectorMap in the PR. See comment 1 item 2. I suspect that's why that test still fails with the chagnes in the PR.
Assignee | ||
Comment 10•7 years ago
|
||
Ah that makes sense. Thanks.
Assignee | ||
Comment 11•7 years ago
|
||
https://hg.mozilla.org/integration/autoland/rev/5eca9cc75c3f https://hg.mozilla.org/integration/autoland/rev/ad90f0a5b56c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 1379299
You need to log in
before you can comment on or make changes to this bug.
Description
•