Bug 38731 – Make CSS Parser properly handle only-for-pages pseudo-classes

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Yuzo Fujishima]

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.

[Yuzo Fujishima]

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

[Yuzo Fujishima]

Created attachment 55352 [details]
Make CSS parser properly handle page-only pseudo-classes.

[WebKit Review Bot]

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.

[Yuzo Fujishima]

Created attachment 55355 [details]
Make CSS parser properly handle page-only pseudo-classes.

[Alexey Proskuryakov]

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.

[Shinichiro Hamaji]

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.

[Yuzo Fujishima]

Created attachment 55539 [details]
Make CSS parser properly handle page-only pseudo-classes.

[Yuzo Fujishima]

Created attachment 55541 [details]
Make CSS parser properly handle page-only pseudo-classes.

[Yuzo Fujishima]

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.

[Shinichiro Hamaji]

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.

[Yuzo Fujishima]

Created attachment 55664 [details]
Make CSS parser properly handle page-only pseudo-classes.

[Yuzo Fujishima]

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.

[Shinichiro Hamaji]

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.

[Yuzo Fujishima]

Created attachment 55668 [details]
Make CSS parser properly handle page-only pseudo-classes.

[Yuzo Fujishima]

(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.

[Shinichiro Hamaji]

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.

[Yuzo Fujishima]

Committed r59751: <http://trac.webkit.org/changeset/59751>

[Simon Fraser (smfr)]

This may have caused some serious problems:
https://bugs.webkit.org/show_bug.cgi?id=40452

[Nicholas Shanks]

> 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