Closed Bug 1364009 Opened 6 years ago Closed 6 years ago

stylo: Decide what to do with difference parsing of an+b syntax

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Version:
53 Branch
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: xidorn, Assigned: manishearth)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1364009 - Don't allow comments/spaces between signs,numbers,and `n` in an+b syntax for nth-child;
59 bytes, text/x-review-board-request
dbaron
: review+
Details 
According to servo/rust-cssparser#138, Gecko implements the syntax from selectors-3 [1], but per Simon's comment, that syntax is weird for CSS tokenization, so there is a new definition of this syntax defined in CSS Syntax Module [2] which is used by selectors-4 [3].

We have tests layout/style/test/test_selectors.html contains check of the behavior of Gecko, and thus fails for Stylo.

It's not clear to me what is the easiest way towards closing the failures. I think there are two options:
1. implement the new syntax parsing rule in Gecko and update the test
2. change the test to support testing both syntax


[1] https://drafts.csswg.org/selectors-3/#nth-child-pseudo
[2] https://drafts.csswg.org/css-syntax-3/#the-anb-type
[3] https://drafts.csswg.org/selectors-4/#the-nth-child-pseudo
This patch disallows both +/*comment*/3n+4 and +3/*comment*/n+4. This also means that spaces will no longer work in those positions.
Assignee: nobody → manishearth
This doesn't parse nth-child(+n) or nth-child(+/**/n), but it should. Will look into tomorrow.
Alright, now it should match exactly what chrome does, and what the spec says we should do.

https://github.com/servo/rust-cssparser/pull/154 handles the last discrepancy with servo (servo doesn't parse `-/**/n`, even though it parses `+/**/n`)
Comment on attachment 8873636 [details]
Bug 1364009 - Don't allow comments/spaces between signs,numbers,and `n` in an+b syntax for nth-child;

https://reviewboard.mozilla.org/r/145026/#review149442

::: layout/reftests/css-selectors/nth-child-1.html
(Diff revision 7)
>  <html><head>
>      <meta charset="utf-8">
>      <title>Tests :nth-child(An+B) matching</title>
>      <style type="text/css">
>  
> -    div :nth-child(+/**/3n-2)  { color:white; }

Isn't this test still valid?  Shouldn't it stay?  (Same for -2 and the corresponding -ref changes?)

::: layout/reftests/css-selectors/nth-child-1.html
(Diff revision 7)
> -    div :nth-child(+/**/3n-2)  { color:white; }
>      div :nth-child(+3n/**/-2)  { background-color:black; }
>      div :nth-child(+3n/**/-2)  { font-size:12px; }
>      div :nth-child(+3n-/**/2)  { text-decoration: underline; }
>      div :nth-child(+3n-2/**/)  { border-left-width: 1px; }
> -    div :nth-child(+3/**/n-2) { border-right-width: 1px; }

shouldn't there now be a test that this doesn't match?

::: layout/reftests/css-selectors/nth-child-1.html:13
(Diff revision 7)
> -    div :nth-child(+3n-/**/2) { border-style: solid; }
> -    div :nth-child(+3n-2/**/) { border-color: blue; }
> +    div :nth-child(+3n-2/**/) { border-right-width: 1px }
> +    div :nth-child(+3n-/**/2) { border-style: solid; border-color: blue;}

maybe a little less confusing not to swap the order of these tests?

::: layout/reftests/css-selectors/nth-child-1.html
(Diff revision 7)
>      div :nth-child(+3n/**/-2) { border-bottom-width: 1px; }
> -    div :nth-child(+3n-/**/2) { border-style: solid; }
> -    div :nth-child(+3n-2/**/) { border-color: blue; }
> +    div :nth-child(+3n-2/**/) { border-right-width: 1px }
> +    div :nth-child(+3n-/**/2) { border-style: solid; border-color: blue;}
>  
>      /* valid but will not match anything */
> -    div :nth-child(-/**/n-2)  { color:red; }

shouldn't there now be a test that this (and the following test) do match... and can't that fill in for the removed tests above?

::: layout/reftests/css-selectors/nth-child-1.html:27
(Diff revision 7)
>      div :nth-child(-/**/ n-2) { color:red; }
>      div :nth-child(- /**/n-2) { color:red; }
>      div :nth-child(+/**/ n-2) { color:red; }
>      div :nth-child(+ /**/n-2) { color:red; }

could you also add tests for the same thing with 3n instead of n?

But don't bother if test_selectors tests cover it...

::: layout/reftests/css-selectors/nth-child-2.html:6
(Diff revision 7)
>  
> -    div :nth-child(+/**/3N-2)  { color:white; }
>      div :nth-child(+3N/**/-2)  { background-color:black; }
>      div :nth-child(+3N/**/-2)  { font-size:12px; }
>      div :nth-child(+3N-/**/2)  { text-decoration: underline; }
>      div :nth-child(+3N-2/**/)  { border-left-width: 1px; }
> -    div :nth-child(+3/**/N-2) { border-right-width: 1px; }
>      div :nth-child(+3N/**/-2) { border-top-width: 1px; }
>      div :nth-child(+3N/**/-2) { border-bottom-width: 1px; }
> -    div :nth-child(+3N-/**/2) { border-style: solid; }
> -    div :nth-child(+3N-2/**/) { border-color: blue; }
>  
> +    div :nth-child(+3N-2/**/) { border-right-width: 1px; }
> +    div :nth-child(+3N-/**/2) { border-style: solid; border-color: blue;}
> +
>      /* valid but will not match anything */
> -    div :nth-child(-/**/N-2)  { color:red; }
>      div :nth-child(-N/**/-2)  { color:red; }
>      div :nth-child(-N/**/-2)  { color:red; }
>      div :nth-child(-N-/**/2)  { color:red; }
>      div :nth-child(-N-2/**/)  { color:red; }
> -    div :nth-child(-1/**/N-2) { color:red; }

all the same comments apply here.

(When you're done, please double-check that the only differences between -1 and -2 are the case of the n/N.)

::: layout/style/nsCSSParser.cpp:6354
(Diff revision 7)
> +  if (mToken.IsSymbol('+') || mToken.IsSymbol('-')) {
> +      // This can only be +n or -n, since +an, -an, +a, -a will all

2-space indent, please

::: layout/style/test/test_selectors.html:719
(Diff revision 7)
> -    test_parseable(":nth-child( -/**/2/**/n/**/+/**/4 )");
> +    test_balanced_unparseable(":nth-child( -/**/2/**/n/**/+/**/4 )");
>      test_balanced_unparseable(":nth-child( -/**/ 2/**/n/**/+/**/4 )");
>      test_balanced_unparseable(":nth-child( -/**/2 /**/n/**/+/**/4 )");
>      test_balanced_unparseable(":nth-child( -/**/2/**/ n/**/+/**/4 )");
> -    test_parseable(":nth-child( -/**/2/**/n /**/+/**/4 )");
> -    test_parseable(":nth-child( -/**/2/**/n/**/ +/**/4 )");
> +    test_balanced_unparseable(":nth-child( -/**/2/**/n /**/+/**/4 )");
> +    test_balanced_unparseable(":nth-child( -/**/2/**/n/**/ +/**/4 )");

I think this just needs the first of the many test_balanced_unparseable now.

But then you should add a test_parseable() for the same thing with the most allowed comments, and then an appropriate corresponding test for each whitespace insertion within that -- to continue testing what was being tested here.
Attachment #8873636 - Flags: review?(dbaron) → review+
Comment on attachment 8873636 [details]
Bug 1364009 - Don't allow comments/spaces between signs,numbers,and `n` in an+b syntax for nth-child;

https://reviewboard.mozilla.org/r/145026/#review149442

> Isn't this test still valid?  Shouldn't it stay?  (Same for -2 and the corresponding -ref changes?)

+3n is a single dimension token. The only time comments between + and n are allowed are when there's no number involved.

> could you also add tests for the same thing with 3n instead of n?
> 
> But don't bother if test_selectors tests cover it...

(it does)
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c3ac266d12a8
Don't allow comments/spaces between signs,numbers,and `n` in an+b syntax for nth-child; r=dbaron
Blocks: 1370353
You need to log in before you can comment on or make changes to this bug.