According to CSS Paged Media Module Level 3 http://dev.w3.org/csswg/css3-page/ , :first, :left, and :right pseudo-classes can be used only for pages, and no other pseudo-classes can be used for pages. Fix the CSS parser to enforce the above.
The following bugs were caused by the improper handling mentioned above. https://bugs.webkit.org/show_bug.cgi?id=38272 https://bugs.webkit.org/show_bug.cgi?id=38697
Created attachment 55352 [details] Make CSS parser properly handle page-only pseudo-classes.
Attachment 55352 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1 WebCore/css/CSSSelector.h:90: One space before end of line comments [whitespace/comments] [5] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 55355 [details] Make CSS parser properly handle page-only pseudo-classes.
I don't know this code well enough to review, but here are some comments about regression tests: document.getElementById("result").innerHTML = result; + + try { + document.querySelectorAll(".login-popup:first"); + } catch (e) { + document.getElementById("test2").style.display = "none"; + } + try { + document.querySelectorAll("::first"); + } catch (e) { + document.getElementById("test3").style.display = "none"; + } As you're adding this, you can remove the test I added yesterday. +<div id="test2">FAIL: Test 2</div> +<div id="test3">FAIL: Test 3</div> Ideally, subtest output says "Test didn't run" initially, and then changes to PASS or FAIL. But it's not very important. Please test left and right, too. Does this patch also make things like "@page :visited" invalid (as they should be)? We'd need tests for that, too.
Comment on attachment 55355 [details] Make CSS parser properly handle page-only pseudo-classes. The C++ code change looks sane to me but I'm putting r- for this patch per Alexey's comment (thanks!). > @@ -431,6 +431,7 @@ void CSSSelector::extractPseudoType() const > > bool element = false; // pseudo-element > bool compat = false; // single colon compatbility mode > + bool page = false; // pseudo-page > > switch (m_pseudoType) { > case PseudoAfter: I slightly prefer more descriptive name like isPseudoPage. > @@ -529,12 +530,13 @@ void CSSSelector::extractPseudoType() const > case PseudoFirstPage: > case PseudoLeftPage: > case PseudoRightPage: > - // FIXME: These should only be allowed in @page rules. Disabled them altogether until that's implemented correctly. > - m_pseudoType = PseudoUnknown; > - return; > + page = true; > + break; > } > > - if (m_match == PseudoClass && element) { > + if ((m_match == PseudoPage && !page) || (m_match != PseudoPage && page)) > + m_pseudoType = PseudoUnknown; > + else if (m_match == PseudoClass && element) { > if (!compat) > m_pseudoType = PseudoUnknown; > else I'd write bool inPage = (m_match == PseudoPage); if (inPage != isPseudoPage) m_pseudoType = PseudoUnknown; but I'm not sure if this is the preferred style in WebKit.
Created attachment 55539 [details] Make CSS parser properly handle page-only pseudo-classes.
Created attachment 55541 [details] Make CSS parser properly handle page-only pseudo-classes.
Alexey, Shinichiro, Thank you for reviewing this. (In reply to comment #5) > I don't know this code well enough to review, but here are some comments about regression tests: > > document.getElementById("result").innerHTML = result; > + > + try { > + document.querySelectorAll(".login-popup:first"); > + } catch (e) { > + document.getElementById("test2").style.display = "none"; > + } > + try { > + document.querySelectorAll("::first"); > + } catch (e) { > + document.getElementById("test3").style.display = "none"; > + } > > As you're adding this, you can remove the test I added yesterday. Done. > > +<div id="test2">FAIL: Test 2</div> > +<div id="test3">FAIL: Test 3</div> > > Ideally, subtest output says "Test didn't run" initially, and then changes to PASS or FAIL. But it's not very important. Done, except that the string is hidden in case of success. > > Please test left and right, too. Done. > > Does this patch also make things like "@page :visited" invalid (as they should be)? We'd need tests for that, too. Added tests. Fixed the bug found by the tests. (Thank you!) (In reply to comment #6) > (From update of attachment 55355 [details]) > The C++ code change looks sane to me but I'm putting r- for this patch per Alexey's comment (thanks!). > > > @@ -431,6 +431,7 @@ void CSSSelector::extractPseudoType() const > > > > bool element = false; // pseudo-element > > bool compat = false; // single colon compatbility mode > > + bool page = false; // pseudo-page > > > > switch (m_pseudoType) { > > case PseudoAfter: > > I slightly prefer more descriptive name like isPseudoPage. Renamed it isPagePseudoClass. > > > @@ -529,12 +530,13 @@ void CSSSelector::extractPseudoType() const > > case PseudoFirstPage: > > case PseudoLeftPage: > > case PseudoRightPage: > > - // FIXME: These should only be allowed in @page rules. Disabled them altogether until that's implemented correctly. > > - m_pseudoType = PseudoUnknown; > > - return; > > + page = true; > > + break; > > } > > > > - if (m_match == PseudoClass && element) { > > + if ((m_match == PseudoPage && !page) || (m_match != PseudoPage && page)) > > + m_pseudoType = PseudoUnknown; > > + else if (m_match == PseudoClass && element) { > > if (!compat) > > m_pseudoType = PseudoUnknown; > > else > > I'd write > > bool inPage = (m_match == PseudoPage); > if (inPage != isPseudoPage) > m_pseudoType = PseudoUnknown; > > but I'm not sure if this is the preferred style in WebKit. Done.
Comment on attachment 55541 [details] Make CSS parser properly handle page-only pseudo-classes. A few more comments on the test case. > +@page world:right { color: red; } We may want no reds in expectations, even though this won't show any red in rendering result. > +@page a_page_name:visited { /* :visited is invalid for @page */ > + color: blue; > +} I think this should not be shown, right? If so, I think red would be better. > + try { > + document.querySelectorAll(".login-popup:first"); > + document.getElementById("test2").innerHTML = "Test 2: FAIL"; > + } catch (e) { > + document.getElementById("test2").style.display = "none"; > + } > + try { > + document.querySelectorAll("::first"); > + document.getElementById("test3").innerHTML = "Test 3: FAIL"; > + } catch (e) { > + document.getElementById("test3").style.display = "none"; > + } > + > + try { > + document.querySelectorAll(".login-popup:left"); > + document.getElementById("test4").innerHTML = "Test 4: FAIL"; > + } catch (e) { > + document.getElementById("test4").style.display = "none"; > + } > + try { > + document.querySelectorAll("::left"); > + document.getElementById("test5").innerHTML = "Test 5: FAIL"; > + } catch (e) { > + document.getElementById("test5").style.display = "none"; > + } > + > + try { > + document.querySelectorAll(".login-popup:right"); > + document.getElementById("test6").innerHTML = "Test 6: FAIL"; > + } catch (e) { > + document.getElementById("test6").style.display = "none"; > + } > + try { > + document.querySelectorAll("::right"); > + document.getElementById("test7").innerHTML = "Test 7: FAIL"; > + } catch (e) { > + document.getElementById("test7").style.display = "none"; > + } I'd write this with a loop. Also, as Alexey suggested, it would be better to have strings like "PASS" when a test passes. Like, document.getElementById("test7").innerHTML = "Test 7: PASS"; Are there any reason we should remove these divs? > + bool matchPagePseudoClass = (m_match == PagePseudoClass); > + > + if (matchPagePseudoClass != isPagePseudoClass) I'm not sure if we need a blank line here.
Created attachment 55664 [details] Make CSS parser properly handle page-only pseudo-classes.
Thank you for the review. (In reply to comment #10) > (From update of attachment 55541 [details]) > A few more comments on the test case. > > > +@page world:right { color: red; } > > We may want no reds in expectations, even though this won't show any red in rendering result. > > > +@page a_page_name:visited { /* :visited is invalid for @page */ > > + color: blue; > > +} > > I think this should not be shown, right? If so, I think red would be better Changed to use red. . > > > + try { > > + document.querySelectorAll(".login-popup:first"); > > + document.getElementById("test2").innerHTML = "Test 2: FAIL"; > > + } catch (e) { > > + document.getElementById("test2").style.display = "none"; > > + } > > + try { > > + document.querySelectorAll("::first"); > > + document.getElementById("test3").innerHTML = "Test 3: FAIL"; > > + } catch (e) { > > + document.getElementById("test3").style.display = "none"; > > + } > > + > > + try { > > + document.querySelectorAll(".login-popup:left"); > > + document.getElementById("test4").innerHTML = "Test 4: FAIL"; > > + } catch (e) { > > + document.getElementById("test4").style.display = "none"; > > + } > > + try { > > + document.querySelectorAll("::left"); > > + document.getElementById("test5").innerHTML = "Test 5: FAIL"; > > + } catch (e) { > > + document.getElementById("test5").style.display = "none"; > > + } > > + > > + try { > > + document.querySelectorAll(".login-popup:right"); > > + document.getElementById("test6").innerHTML = "Test 6: FAIL"; > > + } catch (e) { > > + document.getElementById("test6").style.display = "none"; > > + } > > + try { > > + document.querySelectorAll("::right"); > > + document.getElementById("test7").innerHTML = "Test 7: FAIL"; > > + } catch (e) { > > + document.getElementById("test7").style.display = "none"; > > + } > > I'd write this with a loop. Also, as Alexey suggested, it would be better to have strings like "PASS" when a test passes. Like, Changed to use a loop and show "PASS" on success. > > document.getElementById("test7").innerHTML = "Test 7: PASS"; > > Are there any reason we should remove these divs? > > > + bool matchPagePseudoClass = (m_match == PagePseudoClass); > > + > > + if (matchPagePseudoClass != isPagePseudoClass) > > I'm not sure if we need a blank line here. Removed the blank line.
Comment on attachment 55664 [details] Make CSS parser properly handle page-only pseudo-classes. > + var pseudoClasses = [":first", ":left", ":right", "::first", "::left", "::right"]; > + for (var testId = 2; testId <= 7; testId++) { > + var element = document.getElementById("test" + testId); > + var pseudoClass = pseudoClasses[testId - 2]; > + try { > + document.querySelectorAll(".login-popup" + pseudoClass); > + element.innerHTML = "Test " + testId + ": FAIL"; > + } catch (e) { > + element.innerHTML = "Test " + testId + ": PASS"; > + element.style.color = "green"; > + } > + } You were checking ".login-popup:first" and "::first" in the previous patch. I guess it's better to test all combinations? var pseudoClasses = [":first", "::first", ".login-popup:first", ".loging-popup::first", ...] Otherwise, this patch looks sane to me. I'd put r+ after while unless someone object.
Created attachment 55668 [details] Make CSS parser properly handle page-only pseudo-classes.
(In reply to comment #13) > (From update of attachment 55664 [details]) > > + var pseudoClasses = [":first", ":left", ":right", "::first", "::left", "::right"]; > > + for (var testId = 2; testId <= 7; testId++) { > > + var element = document.getElementById("test" + testId); > > + var pseudoClass = pseudoClasses[testId - 2]; > > + try { > > + document.querySelectorAll(".login-popup" + pseudoClass); > > + element.innerHTML = "Test " + testId + ": FAIL"; > > + } catch (e) { > > + element.innerHTML = "Test " + testId + ": PASS"; > > + element.style.color = "green"; > > + } > > + } > > You were checking ".login-popup:first" and "::first" in the previous patch. I guess it's better to test all combinations? > > var pseudoClasses = [":first", "::first", ".login-popup:first", ".loging-popup::first", ...] > > Otherwise, this patch looks sane to me. I'd put r+ after while unless someone object. Thank you for the review. Changed to test 12 combinations.
Comment on attachment 55668 [details] Make CSS parser properly handle page-only pseudo-classes. This patch looks good to me, but let's wait one or two days in case CSS specialists have some thought on this.
Committed r59751: <http://trac.webkit.org/changeset/59751>
This may have caused some serious problems: https://bugs.webkit.org/show_bug.cgi?id=40452
> According to CSS Paged Media Module Level 3 http://dev.w3.org/csswg/css3-page/ , > :first, :left, and :right pseudo-classes can be used only for pages, > and no other pseudo-classes can be used for pages. You have forgotten :blank