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

VERIFIED FIXED in mozilla1.9.1a1

Status

()

Core
CSS Parsing and Computation
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: dbaron, Assigned: Michael Ventnor)

Tracking

({css3})

Trunk
mozilla1.9.1a1
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

10 years ago
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

10 years ago
Created attachment 306470 [details]
testcase

Comment 2

10 years ago
Created attachment 306473 [details]
testcase
Attachment #306470 - Attachment is obsolete: true

Comment 3

10 years ago
Created attachment 306476 [details] [diff] [review]
patch
Assignee: nobody → fantasai.bugs
Status: NEW → ASSIGNED
Attachment #306476 - Flags: superreview?(dbaron)
Attachment #306476 - Flags: review?(dbaron)
(Reporter)

Updated

10 years ago
Attachment #306473 - Attachment mime type: application/octet-stream → text/html
(Reporter)

Updated

10 years ago
Attachment #306470 - Attachment mime type: application/octet-stream → text/html
(Reporter)

Comment 4

10 years ago
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

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

Can't I just add a reftest?
(Reporter)

Comment 6

10 years ago
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

10 years ago
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.
(Reporter)

Comment 8

10 years ago
For mochitests, you could use the framework I put in the patch in bug 420814.
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.
I'll update the tests, thanks Jeff.
(Reporter)

Comment 11

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

Comment 12

10 years ago
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.
Attachment #306476 - Flags: superreview?(dbaron)
Attachment #306476 - Flags: superreview-
Attachment #306476 - Flags: review?(dbaron)
Attachment #306476 - Flags: review-

Updated

10 years ago
Summary: empty string in [attr^=""], [attr$=""], and [attr*=""] should be parser error → empty string in [attr^=""], [attr$=""], [attr*=""], and [attr~=""] should be parser error

Comment 13

10 years ago
Created attachment 308577 [details] [diff] [review]
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?
Attachment #306476 - Attachment is obsolete: true
Attachment #308577 - Flags: superreview?(dbaron)
Attachment #308577 - Flags: review?(dbaron)
(Reporter)

Comment 14

10 years ago
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.)
Attachment #308577 - Flags: superreview?(dbaron)
Attachment #308577 - Flags: superreview+
Attachment #308577 - Flags: review?(dbaron)
Attachment #308577 - Flags: review+
(Reporter)

Comment 15

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

Comment 16

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

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

Comment 17

9 years ago
Created attachment 324111 [details] [diff] [review]
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.
Attachment #324111 - Flags: superreview?(dbaron)
Attachment #324111 - Flags: review?(dbaron)
(Assignee)

Updated

9 years ago
Summary: empty string in [attr^=""], [attr$=""], [attr*=""], and [attr~=""] should be parser error → empty string in [attr^=""], [attr$=""], [attr*=""], and [attr~=""] should match nothing
(Reporter)

Comment 18

9 years ago
I'm really not sure what the case is for changing interoperably-implemented behavior here.
(Assignee)

Comment 19

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

What do you mean?
(Reporter)

Comment 20

9 years ago
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.
(Assignee)

Comment 21

9 years ago
(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.
(Reporter)

Comment 22

9 years ago
(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?)
(Assignee)

Comment 23

9 years ago
(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.
(Reporter)

Comment 24

9 years ago
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.)
(Assignee)

Comment 25

9 years ago
Created attachment 324118 [details] [diff] [review]
Tests
Attachment #324118 - Flags: review?(dbaron)
(Assignee)

Comment 26

9 years ago
Created attachment 324119 [details] [diff] [review]
Patch 2

It makes more sense to do the checking this way.
Attachment #324111 - Attachment is obsolete: true
Attachment #324119 - Flags: superreview?(dbaron)
Attachment #324119 - Flags: review?(dbaron)
Attachment #324111 - Flags: superreview?(dbaron)
Attachment #324111 - Flags: review?(dbaron)
(Reporter)

Comment 27

9 years ago
Comment on attachment 324119 [details] [diff] [review]
Patch 2

r+sr=dbaron
Attachment #324119 - Flags: superreview?(dbaron)
Attachment #324119 - Flags: superreview+
Attachment #324119 - Flags: review?(dbaron)
Attachment #324119 - Flags: review+
(Reporter)

Comment 28

9 years ago
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.
Attachment #324118 - Flags: review?(dbaron) → review+
(Assignee)

Comment 29

9 years ago
Created attachment 324124 [details] [diff] [review]
More tests
Attachment #324118 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Keywords: checkin-needed

Updated

9 years ago
Assignee: fantasai.bugs → ventnor.bugzilla
Status: ASSIGNED → NEW
(Reporter)

Comment 30

9 years ago
I'm preparing these for landing in my tree, but I might not actually land until later this evening.
(Reporter)

Comment 31

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

Comment 32

9 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.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
Status: RESOLVED → VERIFIED
Target Milestone: mozilla1.9.1 → mozilla1.9.1a1
You need to log in before you can comment on or make changes to this bug.