Last Comment Bug 420245 - empty string in [attr^=""], [attr$=""], [attr*=""], and [attr~=""] should match nothing
: empty string in [attr^=""], [attr$=""], [attr*=""], and [attr~=""] should mat...
Status: VERIFIED FIXED
: css3
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla1.9.1a1
Assigned To: Michael Ventnor
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-28 23:56 PST by David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch)
Modified: 2008-07-10 15:15 PDT (History)
11 users (show)
dbaron: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (394 bytes, text/html)
2008-02-29 00:58 PST, fantasai
no flags Details
testcase (436 bytes, text/html)
2008-02-29 01:20 PST, fantasai
no flags Details
patch (1.82 KB, patch)
2008-02-29 01:43 PST, fantasai
dbaron: review-
dbaron: superreview-
Details | Diff | Splinter Review
patch with tests (8.18 KB, patch)
2008-03-11 01:34 PDT, fantasai
dbaron: review+
dbaron: superreview+
Details | Diff | Splinter Review
Patch (1004 bytes, patch)
2008-06-06 18:34 PDT, Michael Ventnor
no flags Details | Diff | Splinter Review
Tests (1.39 KB, patch)
2008-06-06 19:47 PDT, Michael Ventnor
dbaron: review+
Details | Diff | Splinter Review
Patch 2 (1.07 KB, patch)
2008-06-06 20:06 PDT, Michael Ventnor
dbaron: review+
dbaron: superreview+
Details | Diff | Splinter Review
More tests (3.03 KB, patch)
2008-06-06 22:56 PDT, Michael Ventnor
no flags Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2008-02-28 23:56:24 PST
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 2008-02-29 00:58:51 PST
Created attachment 306470 [details]
testcase
Comment 2 fantasai 2008-02-29 01:20:19 PST
Created attachment 306473 [details]
testcase
Comment 3 fantasai 2008-02-29 01:43:25 PST
Created attachment 306476 [details] [diff] [review]
patch
Comment 4 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2008-02-29 11:56:23 PST
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 2008-02-29 12:42:21 PST
=~ 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+2 (review requests must explain patch) 2008-02-29 14:51:17 PST
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 2008-02-29 15:00:34 PST
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+2 (review requests must explain patch) 2008-03-03 21:49:48 PST
For mochitests, you could use the framework I put in the patch in bug 420814.
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2008-03-06 04:17:40 PST
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) 2008-03-06 07:47:19 PST
I'll update the tests, thanks Jeff.
Comment 11 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2008-03-06 08:16:04 PST
It ([attr|=""]) is clear in http://lists.w3.org/Archives/Public/www-style/2008Mar/0007.html
Comment 12 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2008-03-10 12:22:53 PDT
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 2008-03-11 01:34:30 PDT
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?
Comment 14 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2008-03-11 09:32:34 PDT
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+2 (review requests must explain patch) 2008-03-11 09:35:37 PDT
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 2008-06-06 18:31:42 PDT
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 2008-06-06 18:34:23 PDT
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.
Comment 18 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2008-06-06 18:38:19 PDT
I'm really not sure what the case is for changing interoperably-implemented behavior here.
Comment 19 Michael Ventnor 2008-06-06 18:43:53 PDT
(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+2 (review requests must explain patch) 2008-06-06 18:46:11 PDT
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 2008-06-06 18:51:27 PDT
(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+2 (review requests must explain patch) 2008-06-06 19:22:45 PDT
(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 2008-06-06 19:26:59 PDT
(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+2 (review requests must explain patch) 2008-06-06 19:33:22 PDT
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 2008-06-06 19:47:06 PDT
Created attachment 324118 [details] [diff] [review]
Tests
Comment 26 Michael Ventnor 2008-06-06 20:06:01 PDT
Created attachment 324119 [details] [diff] [review]
Patch 2

It makes more sense to do the checking this way.
Comment 27 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2008-06-06 21:55:47 PDT
Comment on attachment 324119 [details] [diff] [review]
Patch 2

r+sr=dbaron
Comment 28 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2008-06-06 21:57:16 PDT
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 2008-06-06 22:56:59 PDT
Created attachment 324124 [details] [diff] [review]
More tests
Comment 30 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2008-06-10 16:48:50 PDT
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+2 (review requests must explain patch) 2008-06-10 17:08:39 PDT
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+2 (review requests must explain patch) 2008-06-10 18:53:40 PDT
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) 2008-07-10 15:15:19 PDT
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

Note You need to log in before you can comment on or make changes to this bug.