Closed Bug 949651 Opened 6 years ago Closed 5 years ago

Pseudo-elements should always be separated by two colons in selectorText

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: sebo, Assigned: brunomperes1, Mentored)

Details

(Whiteboard: [language=c++][good first bug])

Attachments

(2 files, 3 obsolete files)

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
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?
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
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
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]
Mentor: dbaron
Whiteboard: [mentor=dbaron][language=c++][good first bug] → [language=c++][good first bug]
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.
Attached file standalone testcase
Here's a simple standalone testcase using document.write.
Just to clarify David's test case. The output should be q::before, while it currently is q:before.

Sebastian
Attached patch css.patch (obsolete) — Splinter Review
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.
Attached patch css3.patch (obsolete) — Splinter Review
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+
Attached patch cssextratests.patch (obsolete) — Splinter Review
: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+
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?
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
Are you able to fix the failures in layout/inspector/tests/test_bug557726.html ?
Flags: needinfo?(brunomperes1)
Attached patch fixes-1.patchSplinter Review
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+
https://hg.mozilla.org/mozilla-central/rev/b79232598a90
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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.