stylo: Implement the class/id case-insensitive matching quirk




CSS Parsing and Computation
3 months ago
18 days ago


(Reporter: bz, Assigned: SimonSapin)


(Depends on: 1 bug, Blocks: 2 bugs)

53 Branch
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)




3 months ago
Spec: and both say that matching is ASCII-case-insensitive in quirks mode.


  <style>#foo { color: green; }</style>                                                
  <div id="FOO">Should be green</div>
Blocks: 1324348

Comment 1

3 months 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
Simon, you seem to be working on parts of this. How much overlap is there?
Flags: needinfo?(simon.sapin)
Priority: -- → P2
Oh nevermind, looks like Simon was working on regular case-insensitive matching.
Flags: needinfo?(simon.sapin)

Comment 4

2 months 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].

[2] is months old and didn’t get a response.

Comment 5

2 months 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.

Comment 6

2 months ago
We already have Atom::eq_ignore_ascii_case:

We probably want to add a fast path that tries `self == other` first.
(In reply to Simon Sapin (:SimonSapin) from comment #6)
> We already have Atom::eq_ignore_ascii_case:
> gecko_string_cache/


> We probably want to add a fast path that tries `self == other` first.

Pushed for this.


2 months ago
Assignee: nobody → simon.sapin

Comment 8

2 months ago is up for review, but it doesn’t seem to fix any test: . I don’t know if we don’t have relevant tests, or if I did something wrong.

Comment 9

2 months 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.

Comment 10

2 months ago
Ah that makes sense. Thanks.

Comment 11

a month ago
Last Resolved: a month ago
Resolution: --- → FIXED
Depends on: 1379299
You need to log in before you can comment on or make changes to this bug.