Closed Bug 1363531 Opened 5 years ago Closed 5 years ago

stylo: Support case insensitive HTML attributes

Categories

(Core :: CSS Parsing and Computation, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: manishearth, Unassigned)

References

(Blocks 1 open bug)

Details

A bunch of HTML attributes are case insensitive wrt style, which means that in an HTML document, the selector `[dir=rtl]` must match `<span dir="RTL">` too.


https://searchfox.org/mozilla-central/source/layout/style/nsCSSParser.cpp#5853

When parsing selectors, Gecko will check if the selector is an attr selector for one of these, and store an extra bit alongside the selector that it is case sensitive. It will then check this[1][2] whilst matching attributes and matches appropriately.

We need something similar. AFAICT this will require rust-selectors changes; while we could make SelectorImpl::Attribute into an (Atom, bool), this kind of gets awkward since the attributes we match against will have to get a dummy bool (computing the bool requires a slew of string comparisons and is expensive; so we can't do this during traversal); and it makes == stop being reflexive.

I guess the simple thing to do here would be to store a bool in attr selectors, have SelectorImpl have a function for determining if an attr is case insensitive, and pass down an extra bool (which is this bool + whether or not the doc is HTML) to the match_attr_foo functions on selectors::MatchAttr.


Thoughts?

This unblocks a lot of tests using `dir`.


 [1]: https://searchfox.org/mozilla-central/source/layout/style/nsCSSRuleProcessor.cpp#1400
 [2]: https://searchfox.org/mozilla-central/source/layout/style/nsCSSRuleProcessor.cpp#2236
https://html.spec.whatwg.org/multipage/#case-sensitivity-of-selectors

This could probably be filed as a E-mentored issue on servo/servo.
I think we should definitely not smuggle an extra bool through SelectorImpl::Attribute. We can add it to AttrSelector (possibly packed with NamespaceConstraint if it helps avoid growing size_of::<selector::Component>()). We moved rust-selectors in-tree to make this kind of change easier :)
Don't we already have CaseSensitivity on AttrEqual?

Also filed bug 1364148 about making Component smaller.
Also, does the case-sensitivity here apply only to AttrEqual, or to all the attribute selectors?
> Also, does the case-sensitivity here apply only to AttrEqual, or to all the attribute selectors?

All of them.  Looks like Servo generally screws this up for things other than AttrEqual (e.g. it supports [foo="bar"i] but not [foo^="bar"i]).  I filed bug 1364162 on that.

We should probably have tests in test_selectors.html for this if we don't have them already.

Note that what Gecko has right now is a case-sensitivity tristate, with three values:

1)  Case-sensitive. ([foo="bar"])
2)  Case-insensitive  ([foo="bar"i])
3)  Case-insensitive only if the element is HTML ([dir="RTL"]).
[1] says that the value of `type` attributes (among others) is case-insensitive, except in the bit of user-agent stylesheet [2] that sets list-style-type based on the type attribute of ul, ol, and li. However I can’t find these rules in layout/style/res/*.css. Are they implemented in C++ without selector matching? If so, we don’t need to worry about this exception to the exception.

[1] https://html.spec.whatwg.org/multipage/scripting.html#case-sensitivity-of-selectors
[2] https://html.spec.whatwg.org/multipage/rendering.html#attribute-selector-case-sensitive
Flags: needinfo?(bzbarsky)
Those list-style-type mappings are implemented via attribute mapping. We are not using ua sheet for that.
I have a patch that goes on top of https://github.com/servo/servo/pull/16915. I’ll open a new PR after that one lands.

(Canceling needinfo since Xidorn responded.)
Flags: needinfo?(bzbarsky)
> Those list-style-type mappings are implemented via attribute mapping

Indeed.  Note that in the spec these are defined as presentational hints, not UA-level rules.  For Gecko that means implementation as mapped attributes in C++, for the moment.  So no need to worry about these for selector matching purposes.
Fixed in https://github.com/servo/servo/pull/16915. It uses the same mechanism as [foo=bar i], so bug 1364162 applies.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.