Closed
Bug 949651
Opened 10 years ago
Closed 10 years ago
Pseudo-elements should always be separated by two colons in selectorText
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: sebo, Assigned: brunomperes1, Mentored)
Details
(Whiteboard: [language=c++][good first bug])
Attachments
(2 files, 3 obsolete files)
151 bytes,
text/html
|
Details | |
6.43 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
The CSS2.1 pseudo-elements ::before, ::after, ::first-letter and ::first-line should be returned with double-colon when accessing the 'selectorText' property of a CSS rule like the CSS3 pseudo-elements. Test case: 1. Go to http://fiddle.jshell.net/JU4hz/show/ 2. Open the Web Console via Ctrl+Shift+K 3. Type "document.styleSheets[1].cssRules[0].selectorText" (without quotes) into the command line and hit Enter => "div:before" is returned instead of "div::before". Sebastian
Comment 1•10 years ago
|
||
I believe we do this very purposefully, ever since we supported the :: notation (see bug 62843): it allowed serialized values to be maximally compatible with other UAs and web sites. Do we have any indication that this change is web-compatible (or not)? Why is it desirable?
Reporter | ||
Comment 2•10 years ago
|
||
Actually the other main browser engines (Blink, Trident, Presto, Nitro) do normalize it to "div::before". So Gecko is the only one that does it different. Sebastian
Comment 3•10 years ago
|
||
Note that the compatibility constraint at the time was that the string we produce be parseable by other UAs.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Comment 4•10 years ago
|
||
Sure, though it obviously has changed now. Sebastian
I still want to stick to the general principle that serialization should always be to the maximally-compatible (and thus generally oldest) form. I'm not convinced that we should break it even if other browsers have done so, although in this case perhaps it's ok given that the single-colon syntax is informally deprecated.
The code to fix is around the nsCSSPseudoElements::IsCSS2PseudoElement test in nsCSSSelector::AppendToStringWithoutCombinatorsOrNegations in layout/style/StyleRule.cpp, and serialization tests should be added to layout/style/test/test_selectors.html with additional calls to should_serialize_to().
Whiteboard: [mentor=dbaron][language=c++]
Whiteboard: [mentor=dbaron][language=c++] → [mentor=dbaron][language=c++][good first bug]
Updated•10 years ago
|
Mentor: dbaron
Whiteboard: [mentor=dbaron][language=c++][good first bug] → [language=c++][good first bug]
Comment hidden (obsolete) |
Comment hidden (obsolete) |
I would like to work on this, though I can't reproduce the error. The output I get from the console for the is: "*, body, button, input, textarea, select" Can someone please confirm? I'm using the latest code from mozilla-central and that would be my first bug.
Here's a simple standalone testcase using document.write.
Reporter | ||
Updated•10 years ago
|
Reporter | ||
Comment 11•10 years ago
|
||
Just to clarify David's test case. The output should be q::before, while it currently is q:before. Sebastian
Assignee | ||
Comment 12•10 years ago
|
||
Perfect, it works now. Thank you! Though I am not entirely sure if my solution is appropriate, as it changes a logical statement. There was a !not operator on the nsCSSPseudoElements::IsCSS2PseudoElement for the tag. Can someone please explain it to me? Why would it append ':' only to non-CSS2 pseudoelements? Or was it a logical bug? I am moving to the writing the tests for the change. I am bit confused on how to run the tests on the layout/style/test/test_selectors.html, should I run mochitest-chrome on the container folder?
Attachment #8504469 -
Flags: feedback+
Comment on attachment 8504469 [details] [diff] [review] css.patch You actually want to remove the condition entirely, and always append a ":". The reason is that through CSS2, pseudo-elements were written using the same single-: syntax as pseudo-classes, but in selectors level 3, pseudo-elements started to be distinguished with two colons. Thus the code you're modifiying serialized the old pseudo-elements the old way. Also, if you want feedback or review on patches, you should set the flag to "?" and then put a person's email address or name in the textbox following.
Attachment #8504469 -
Flags: feedback+ → review-
Oh, and to run the tests, you can do: ./mach mochitest-plain layout/style/test/test_selectors.html from the root of the source tree.
Assignee | ||
Comment 15•10 years ago
|
||
Thank you :dbaron ! I've removed the condition and added a comment on the appending step. I also reviewed the test cases for serialization of pseudo elements and added one extra test to cover the case where the CSS already complies with the selectors level 3. Could you please review the patch?
Attachment #8504469 -
Attachment is obsolete: true
Attachment #8505171 -
Flags: review?(dbaron)
Comment on attachment 8505171 [details] [diff] [review] css3.patch You should also have a commit message in the patch. It should probably be: Bug 949651 - Serialize all pseudo-elements with the two-colon syntax, even those that allow one colon. r=dbaron >+ // Appends colon to all pseudo-elements, complying with specification >+ // for level 3 selectors Instead of this comment, how about: While our atoms use one colon, most pseudo-elements require two colons (those not in CSS level 2) and all pseudo-elements allow two colons. So serialize to the non-deprecated two colon syntax. >+ should_serialize_to("p::first-letter", "p::first-letter"); >+ should_serialize_to("p:first-letter", "p::first-letter"); >+ should_serialize_to("div>p:first-letter", "div > p::first-letter"); >+ should_serialize_to("span +div:first-line", "span + div::first-line"); Could you also add a test for a non-CSS2 pseudo-element? Probably ::-moz-placeholder? Also, did you check that the tests pass with the patch applied? Once you've addressed those comments, post a revised patch with them addressed and with the commit message above, and somebody can check it in for you.
Attachment #8505171 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 17•10 years ago
|
||
:dbaron, all tests on layout/style/test/ passed. I've: - Added an extra test case that verifies if non CSS2 pseudo elements aren't affected by the code change; - Changed to a clearer comment on the code; - Added a commit message. Could you please verify the patch again?
Attachment #8505171 -
Attachment is obsolete: true
Attachment #8505669 -
Flags: review?(dbaron)
Attachment #8505669 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 18•10 years ago
|
||
Thanks for your patience and help :dbaron! I don't see any checkin-needed flag option, should I just wait for now for it to be checked in?
Updated•10 years ago
|
Keywords: checkin-needed
There was actually something funny about the patch: two of the removed lines had different indentation from what they had in your source tree. It seemed like you'd made some other commits underneath this patch and not removed those. In any case, I fixed the patch and landed it: https://hg.mozilla.org/integration/mozilla-inbound/rev/f7e1426249f2 Thanks.
Assignee: nobody → brunomperes1
Keywords: checkin-needed
Comment 20•10 years ago
|
||
Backed out for mochitest-5 failures. https://hg.mozilla.org/integration/mozilla-inbound/rev/21caa5dfb494 https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3030788&repo=mozilla-inbound
Are you able to fix the failures in layout/inspector/tests/test_bug557726.html ?
Flags: needinfo?(brunomperes1)
Comment 22•10 years ago
|
||
Also devtools: https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3031988&repo=mozilla-inbound
Assignee | ||
Comment 23•10 years ago
|
||
I think so, please check? The test cases broke due to expectation on a single -: Should I add another validation in a second div with -:: too?
Attachment #8505669 -
Attachment is obsolete: true
Attachment #8505860 -
Flags: review?(dbaron)
Flags: needinfo?(brunomperes1)
Comment on attachment 8505860 [details] [diff] [review] fixes-1.patch >+ // While our atoms use one colon, most pseudo-elements require two >+ // colons (those not in CSS level 2) and all pseudo-elements allow >+ // two colons. So serialize to the non-deprecated two colon syntax. > aString.Append(char16_t(':')); >- if (!nsCSSPseudoElements::IsCSS2PseudoElement(mLowercaseTag)) { >- aString.Append(char16_t(':')); >- } This is again a patch against a modified tree with some of your other attempts in it, which isn't a patch that we can cleanly apply. The above line without a + or - should have a + before it, since it should be an added line. This means you have other changesets in your tree that you should have cleaned out before generating this patch. (You might want to ask for help in #mercurial on irc.mozilla.org, or perhaps other channels. I think there's one for new contributors but I've forgotten the name.) In any case, I did a try push of this patch: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a80be6e8bef0 https://tbpl.mozilla.org/?tree=Try&rev=a80be6e8bef0
Comment on attachment 8505860 [details] [diff] [review] fixes-1.patch r=dbaron with that diff fixed up and the commit message restored (which I have fixed locally). (I noticed the commit message also disappeared between the previous patch and this one.) I'll also land this, given that the try run was green.
Attachment #8505860 -
Flags: review?(dbaron) → review+
Comment 27•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b79232598a90
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Reporter | ||
Comment 28•10 years ago
|
||
Quickly tested it and it seems to work fine. Thanks! Sebastian
You need to log in
before you can comment on or make changes to this bug.
Description
•