Bug 38731

Summary: Make CSS Parser properly handle only-for-pages pseudo-classes
Product: WebKit Reporter: Yuzo Fujishima <yuzo@google.com>
Component: CSSAssignee: Yuzo Fujishima <yuzo@google.com>
Status: RESOLVED FIXED    
Severity: Normal CC: ap@webkit.org, hamaji@chromium.org, hayato@chromium.org, hyatt@apple.com, mitz@webkit.org, nickshanks@nickshanks.com, simon.fraser@apple.com, webkit.review.bot@gmail.com, yuzo@google.com
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Mac OS X 10.5   
Bug Depends on:    
Bug Blocks: 15548    
Attachments:
Description Flags
Make CSS parser properly handle page-only pseudo-classes.
none
Make CSS parser properly handle page-only pseudo-classes.
none
Make CSS parser properly handle page-only pseudo-classes.
none
Make CSS parser properly handle page-only pseudo-classes.
none
Make CSS parser properly handle page-only pseudo-classes.
none
Make CSS parser properly handle page-only pseudo-classes. hamaji: review+

Description From 2010-05-07 02:19:43 PST
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.
------- Comment #1 From 2010-05-07 02:22:37 PST -------
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
------- Comment #2 From 2010-05-07 02:40:25 PST -------
Created an attachment (id=55352) [details]
Make CSS parser properly handle page-only pseudo-classes.
------- Comment #3 From 2010-05-07 02:45:38 PST -------
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.
------- Comment #4 From 2010-05-07 03:16:45 PST -------
Created an attachment (id=55355) [details]
Make CSS parser properly handle page-only pseudo-classes.
------- Comment #5 From 2010-05-07 08:55:10 PST -------
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 #6 From 2010-05-10 02:14:38 PST -------
(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.

> @@ -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.
------- Comment #7 From 2010-05-10 03:47:16 PST -------
Created an attachment (id=55539) [details]
Make CSS parser properly handle page-only pseudo-classes.
------- Comment #8 From 2010-05-10 04:00:19 PST -------
Created an attachment (id=55541) [details]
Make CSS parser properly handle page-only pseudo-classes.
------- Comment #9 From 2010-05-10 04:01:11 PST -------
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] [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 #10 From 2010-05-10 04:59:48 PST -------
(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.

> +        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.
------- Comment #11 From 2010-05-10 22:40:01 PST -------
Created an attachment (id=55664) [details]
Make CSS parser properly handle page-only pseudo-classes.
------- Comment #12 From 2010-05-10 22:42:23 PST -------
Thank you for the review.

(In reply to comment #10)
> (From update of attachment 55541 [details] [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 #13 From 2010-05-10 22:58:10 PST -------
(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.
------- Comment #14 From 2010-05-10 23:07:27 PST -------
Created an attachment (id=55668) [details]
Make CSS parser properly handle page-only pseudo-classes.
------- Comment #15 From 2010-05-10 23:08:26 PST -------
(In reply to comment #13)
> (From update of attachment 55664 [details] [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 #16 From 2010-05-13 02:14:15 PST -------
(From update of attachment 55668 [details])
This patch looks good to me, but let's wait one or two days in case CSS specialists have some thought on this.
------- Comment #17 From 2010-05-19 00:33:46 PST -------
Committed r59751: <http://trac.webkit.org/changeset/59751>
------- Comment #18 From 2010-06-10 16:22:32 PST -------
This may have caused some serious problems:
https://bugs.webkit.org/show_bug.cgi?id=40452
------- Comment #19 From 2012-06-20 07:35:48 PST -------
> 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