Closed Bug 888190 Opened 11 years ago Closed 8 years ago

Case-insensitive attribute selectors (from Selectors level 4)

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: glazou, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 2 obsolete files)

Attached patch fix (obsolete) — Splinter Review
Since I absolutely needed it myself in BlueGriffon, I wrote a trivial patch to implement the case-insensitive attribute selector proposal from Selectors 4 in section 6.3 [1]. Not sure you want to adopt this patch right now given the status of the document but hey, the patch is at least preserved here and waits for decision.

From my point of view and given how is designed that extension to attribute selectors, it's probably quite harmless to ship it. The impact on existing web sites should be close to zero.

I'll add a test when decision to adopt the patch is made.

[1] http://dev.w3.org/csswg/selectors4/#attribute-case
Exposing a feature to the web makes it harder to change later. Does the CSS WG consider this one mature enough?

If not, the feature could be behind a flag that defaults to off in Firefox and on in BlueGriffon, right?
(In reply to Simon Sapin (:SimonSapin) from comment #1)

> Exposing a feature to the web makes it harder to change later. Does the CSS
> WG consider this one mature enough?
> 
> If not, the feature could be behind a flag that defaults to off in Firefox
> and on in BlueGriffon, right?

Agreed, of course. But:

1. It's not exposed to BlueGriffon users. I am using it in BlueGriffon's JS code but BlueGriffon offers no UI at all for this.

2. I said the decision is in Mozilla's hands
Attachment #768829 - Flags: review?(dbaron)
Comment on attachment 768829 [details] [diff] [review]
fix

This patch does most but not all of what we need to implement the feature.

The problem is that the current isCaseSensitive boolean in attribute selectors has two values:
 * true means case sensitive
 * false means case insensitive for HTML and case sensitive for other languages

See the two occurrences of "mCaseSensitive" in nsCSSRuleProcessor.cpp and the context around them.

However, for "i" we need a third state that is case-insensitive for all languages.  So fixing this properly requires turning nsAttrSelector::mCaseSensitive into an enum with 3 states.


As far as whether to ship this:  I think it's pretty stable at this point, and likely to go to CR pretty soon given https://lists.w3.org/Archives/Public/www-style/2015Jul/0296.html .  http://css4-selectors.com/selector/css4/attribute-case-sensitivity/ suggests other browsers haven't shipped it yet, although I'm not sure how current that is.

Is one of you interested in updating the patch, or should I do it?
Flags: needinfo?(daniel)
Flags: needinfo?(Ms2ger)
Attachment #768829 - Flags: review?(dbaron) → review-
I'll see if I can carve out some time next week.
(In reply to :Ms2ger from comment #4)
> I'll see if I can carve out some time next week.

WebKit has achieved most of the CSS Selector 4 functions.
Flags: needinfo?(bzbarsky)
Attached patch RebasedSplinter Review
Attachment #768829 - Attachment is obsolete: true
The "i" modifier will ship in Safari 9.
Note to self: the "trivial" patch attached above totally doesn't work.  It fails to even parse the relevant selectors.
OK, so if I fix that, things are a bit better.  Next issue: the spec is light on detail in the usual way, so Chrome and Safari shipped non-interoperable implementations.  See https://lists.w3.org/Archives/Public/www-style/2016Jan/0166.html

I'm going to side with Chrome here, not least because the web platform tests do.  So accept 'i' but not 'I'.
OK, this actually works.  The web platform tests even have tests for the serialization bit, which was nice.  No new tests had to be written at all.  ;)
Assignee: daniel → bzbarsky
Comment on attachment 8710855 [details] [diff] [review]
Implement case-insensitive attribute value selectors from Selectors 4

I guess I'll keep the needinfo on myself until dbaron is accepting review requests again...
> I'm going to side with Chrome here

OK, spec editor says Chrome is wrong.  I guess I get the fun of updating the tests.
Attachment #8710855 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
Attachment #8710879 - Flags: review?(dbaron)
Comment on attachment 8710879 [details] [diff] [review]
Implement case-insensitive attribute value selectors from Selectors 4

Is there a Chromium bug filed?  And does somebody upstream that WPT change, or do you have to?  (If so, please do!)

r=dbaron

(I wonder if there are followups we should make for UA style sheet cleanups and/or conformance fixes.)
Attachment #8710879 - Flags: review?(dbaron) → review+
> Is there a Chromium bug filed?

Yep.  https://code.google.com/p/chromium/issues/detail?id=580446 was filed as part of the www-style thread on this.

> And does somebody upstream that WPT change, or do you have to? 

jgraham upstreams them.  But also, since I wrote the patch this got fixed in wpt in https://github.com/w3c/web-platform-tests/commit/b62f9facbc5d0e56e79b9dee7958bed1d05826e9

If that gets merged to our tree before I land, I'll just adjust my patch appropriately.  If not, I'll update my patch such that it matches the upstream one, so hopefully the merge conflict jgraham gets (if any) is simple to resolve.

> (I wonder if there are followups we should make for UA style sheet cleanups and/or conformance fixes.)

Good question.  I'll think on this a bit.
> Good question.  I'll think on this a bit.

I haven't thought of anything so far.  Form stuff uses @value, which is matched case-insensitively anyway in HTML, and that's the obvious thing for which I know we need case-insensitivity.
Flags: needinfo?(daniel)
Flags: needinfo?(Ms2ger)
https://hg.mozilla.org/mozilla-central/rev/53ea4b142e77
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: Future → mozilla47
Depends on: 1439798
See Also: → 1512386
You need to log in before you can comment on or make changes to this bug.