Closed
Bug 420245
Opened 17 years ago
Closed 17 years ago
empty string in [attr^=""], [attr$=""], [attr*=""], and [attr~=""] should match nothing
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
VERIFIED
FIXED
mozilla1.9.1a1
People
(Reporter: dbaron, Assigned: ventnor.bugzilla)
Details
(Keywords: css3)
Attachments
(4 files, 4 obsolete files)
436 bytes,
text/html
|
Details | |
8.18 KB,
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
1.07 KB,
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
3.03 KB,
patch
|
Details | Diff | Splinter Review |
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.
Attachment #306470 -
Attachment is obsolete: true
Assignee: nobody → fantasai.bugs
Status: NEW → ASSIGNED
Attachment #306476 -
Flags: superreview?(dbaron)
Attachment #306476 -
Flags: review?(dbaron)
Reporter | ||
Updated•17 years ago
|
Attachment #306473 -
Attachment mime type: application/octet-stream → text/html
Reporter | ||
Updated•17 years ago
|
Attachment #306470 -
Attachment mime type: application/octet-stream → text/html
Reporter | ||
Comment 4•17 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?
=~ suffers from the same ambiguity, so I'd assume so. Peter, is that correct?
Can't I just add a reftest?
Reporter | ||
Comment 6•17 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•17 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•17 years ago
|
||
For mochitests, you could use the framework I put in the patch in bug 420814.
Comment 9•17 years ago
|
||
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•17 years ago
|
||
It ([attr|=""]) is clear in http://lists.w3.org/Archives/Public/www-style/2008Mar/0007.html
Reporter | ||
Comment 12•17 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-
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•17 years ago
|
||
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•17 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•17 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•17 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•17 years ago
|
||
(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•17 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•17 years ago
|
||
I'm really not sure what the case is for changing interoperably-implemented behavior here.
Assignee | ||
Comment 19•17 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•17 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•17 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•17 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•17 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•17 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•17 years ago
|
||
Attachment #324118 -
Flags: review?(dbaron)
Assignee | ||
Comment 26•17 years ago
|
||
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•17 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•17 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•17 years ago
|
||
Attachment #324118 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Keywords: checkin-needed
Reporter | ||
Comment 30•17 years ago
|
||
I'm preparing these for landing in my tree, but I might not actually land until later this evening.
Reporter | ||
Comment 31•17 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•17 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
Closed: 17 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1
Comment 33•17 years ago
|
||
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.
Description
•