420245 - empty string in [attr^=""], [attr$=""], [attr*=""], and [attr~=""] should match nothing

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Per http://lists.w3.org/Archives/Public/www-style/2008Feb/0333.html we should treat attribute substring selectors with "" as the substring as parser errors.  This change needs to be made in CSSParserImpl::ParseAttributeSelector in layout/style/nsCSSParser.cpp.

Comment 1

[fantasai]

Attachment: testcase

Comment 2

[fantasai]

Attachment: testcase

Comment 3

[fantasai]

Attachment: patch

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The message doesn't say anything about =~.  Is that changing too?

Could you add a mochitest in layout/style/test to test for this?

Comment 5

[fantasai]

=~ suffers from the same ambiguity, so I'd assume so. Peter, is that correct?

Can't I just add a reftest?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Well, I don't see how =$ and =^ suffered from any ambiguity.  But I'm fine with the change, although I'd prefer not to change anything if the WG hasn't agreed to and announced it.

You could add a reftest, but mochitests are a good bit faster, especially once we start testing a lot of different selectors (which I think we should eventually).

Comment 7

[Peter Linss]

Yes, ~= has the same problem and is changing as well.

The ambiguity is what should [foo^=""] select? All elements with a foo attribute, only foo="", or no foo attribute? ie: What strings "begin" with an empty string, all of them, none of them, or only empty strings?

The WG has decided to treat these as invalid selectors in CSS3 since they have no well defined behavior and have no purpose anyhow. The change will be updated in the Selecots draft shortly.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

For mochitests, you could use the framework I put in the patch in bug 420814.

Comment 9

[Jeff Walden [:Waldo]]

http://disruptive-innovations.com/zoo/css3tests/selectorTest.html#target

Daniel: a number of your tests at the above URL will be broken by this change.  (I discovered this when investigating the solitary red block in eighth row for [attr*=""].)  I also note you don't have a test for |="", although what's supposed to happen there is unclear from the URL in comment 0.

Comment 10

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

I'll update the tests, thanks Jeff.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It ([attr|=""]) is clear in http://lists.w3.org/Archives/Public/www-style/2008Mar/0007.html

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 306476 [details] [diff] [review]
patch

Could you fix the |= per comments above, and add tests?

For tests, you can steal test_parseable and test_balanced_unparseable from attachment 308231 [details] [diff] [review] (this will probably land first).  It might not be a bad idea to run a few test_selector_in_html tests too, though.

Comment 13

[fantasai]

Attachment: patch with tests

Added tests. Don't know what you mean by "fix [attr|='']", though; it's working fine afaict. Let me know if there's some other tests you want me to add here.

BTW, why is the function named test_balanced_unparseable instead of just test_unparseable?

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 308577 [details] [diff] [review]
patch with tests

r+sr=dbaron, given that you've actually run the mochitests you're adding and they all pass.

It would be good if the WG made some public announcement about ~=, though.  That actually requires errata to CSS 2.1 as well.  (And, actually, that makes me a bit hesitant about the whole thing given how long that selector has been around -- and that the current behavior is probably interoperable.  In other words, I'm not particularly happy with this resolution since I don't see any good reason for deviating from the mostly-already-interoperable implementations, but I can live with it.)

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Though I think I'd prefer that this not land until CSS 2.1 errata are published (and given a chance for public feedback).

Comment 16

[Michael Ventnor]

http://lists.w3.org/Archives/Public/www-style/2008Apr/0038.html

Now apparently we should accept the empty string but make it match nothing.

Comment 17

[Michael Ventnor]

Attachment: Patch

(I hope fantasai doesn't mind)

I'll run this through tests shortly and attach my own soon, this makes the last of Glazman's CSS3 selectors tests pass.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm really not sure what the case is for changing interoperably-implemented behavior here.

Comment 19

[Michael Ventnor]

(In reply to comment #18)
> I'm really not sure what the case is for changing interoperably-implemented
> behavior here.
> 

What do you mean?

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Are the other browsers, which all implement $="" and ^="" the same way we do, planning to change to match this as well?  Are they waiting until the spec is published or in CR again or doing it now?

Frankly, I'm hesitant to make the change because I wouldn't be surprised if the spec changes again when the other implementers notice and complain.

Comment 21

[Michael Ventnor]

(In reply to comment #20)
> Are the other browsers, which all implement $="" and ^="" the same way we do,
> planning to change to match this as well?  Are they waiting until the spec is
> published or in CR again or doing it now?
> 
> Frankly, I'm hesitant to make the change because I wouldn't be surprised if the
> spec changes again when the other implementers notice and complain.
> 

Have there been any complaints about this decision in the W3 list? This resolution was made 2 months ago according to the date so there should have been plenty of time for complaints to arise.

If not, either they didn't take any notice and are just following us in the hopes of compatibility, or they support that but still follow us in the hopes of compatibility.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #21)
> (In reply to comment #20)
> If not, either they didn't take any notice and are just following us in the
> hopes of compatibility, or they support that but still follow us in the hopes
> of compatibility.

No idea what you mean by following us.  That said, I don't particularly care.

How about some tests with the patch?  (See, for example, the tests in the previous patch?)

Comment 23

[Michael Ventnor]

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)
> > If not, either they didn't take any notice and are just following us in the
> > hopes of compatibility, or they support that but still follow us in the hopes
> > of compatibility.
> 
> No idea what you mean by following us.  That said, I don't particularly care.

I mean that in the same way they (IIRC) stopped enforcing WRONG_DOCUMENT_ERR due to web developers coding by Gecko's standards rather than web standards. Gecko provides a huge leverage in the standards community due to its popularity.

I will be attaching tests shortly.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No, this is in the category of something that nobody ever tried or cared about until somebody wrote a selectors test suite that tested it, not something that Web pages would actually depend on.  (It's also a pretty new feature compared to that.)

Comment 25

[Michael Ventnor]

Attachment: Tests

Comment 26

[Michael Ventnor]

Attachment: Patch 2

It makes more sense to do the checking this way.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 324119 [details] [diff] [review]
Patch 2

r+sr=dbaron

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 324118 [details] [diff] [review]
Tests

r=dbaron, if all four of these are also preceded by a test_parseable for the same selector.

We should also update and land fantasai's tests.

Comment 29

[Michael Ventnor]

Attachment: More tests

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm preparing these for landing in my tree, but I might not actually land until later this evening.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And some of fantasai's tests were still omitted in your "more tests" patch, but I think I've added them all back now.

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Landed:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/41b1e18d4eb2
http://hg.mozilla.org/mozilla-central/index.cgi/rev/7c4bcbed53f1

Thanks for the patches.

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Looks good with the following builds. So marking it verified.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a1pre) Gecko/2008071002 Minefield/3.1a1pre ID:2008071002

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a1pre) Gecko/2008070903 Minefield/3.1a1pre ID:2008070903