Closed Bug 1364009 Opened 8 years ago Closed 8 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.

Attachment

General

Creator:
Created:
Updated:
Size: