Bug 77838 - Unknown pseudo-elements do not cause a rule to be dropped
: Unknown pseudo-elements do not cause a rule to be dropped
Status: ASSIGNED
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Alexis Menard (darktears)
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-05 12:54 PST by Boris Zbarsky
Modified: 2012-03-11 09:02 PDT (History)
11 users (show)

See Also:


Attachments
Testcase (186 bytes, text/html)
2012-02-05 12:54 PST, Boris Zbarsky
no flags Details
Patch (7.13 KB, patch)
2012-03-06 16:26 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
Patch for landing (6.99 KB, patch)
2012-03-06 17:33 PST, Alexis Menard (darktears)
abarth: commit‑queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Boris Zbarsky 2012-02-05 12:54:42 PST
Created attachment 125533 [details]
Testcase

The attached testcase renders correctly in Trident, Gecko, and Presto, but not WebKit.
Comment 1 Alexis Menard (darktears) 2012-03-06 16:26:44 PST
Created attachment 130465 [details]
Patch
Comment 2 Tony Chang 2012-03-06 16:51:15 PST
Comment on attachment 130465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130465&action=review

> LayoutTests/fast/css/getComputedStyle/getComputedStyle-with-pseudo-element.html:6
> +      ::Thisisaboguspseudoelement, body { color: red }

Nit: It would be nice to test : as well, even though we already pass for that case.
Comment 3 Alexis Menard (darktears) 2012-03-06 17:33:38 PST
Created attachment 130488 [details]
Patch for landing
Comment 4 Adam Barth 2012-03-07 09:52:01 PST
Comment on attachment 130488 [details]
Patch for landing

This patch makes many tests time out.
Comment 5 WebKit Review Bot 2012-03-08 09:46:27 PST
Comment on attachment 130488 [details]
Patch for landing

Attachment 130488 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11881034

New failing tests:
fast/dom/HTMLMeterElement/meter-styles-changing-pseudo.html
fast/dom/HTMLMeterElement/meter-writing-mode.html
fast/css/unknown-pseudo-element-matching.html
fast/dom/HTMLMeterElement/meter-element-repaint-on-update-value.html
editing/selection/4397952.html
fast/dom/HTMLMeterElement/meter-appearances-capacity.html
http/tests/security/isolatedWorld/userGestureEvents.html
http/tests/inspector/inspect-element.html
accessibility/aria-disabled.html
fast/css/css-set-selector-text.html
fast/dom/HTMLMeterElement/meter-element.html
fast/css/margin-top-bottom-dynamic.html
css2.1/20110323/abspos-containing-block-initial-004d.htm
fast/dom/HTMLProgressElement/progress-bar-value-pseudo-element.html
fast/dom/HTMLTextAreaElement/reset-textarea.html
fast/dom/HTMLInputElement/input-slider-update.html
fast/css/continuationCrash.html
css2.1/20110323/abspos-containing-block-initial-004b.htm
fast/dom/HTMLProgressElement/progress-writing-mode.html
fast/dom/HTMLTableColElement/resize-table-using-col-width.html
fast/dom/HTMLInputElement/input-slider-update-styled.html
fast/dom/HTMLMeterElement/meter-styles.html
fast/dom/HTMLMeterElement/meter-optimums.html
fast/css/button-height.html
fast/dom/HTMLMeterElement/meter-appearances-rating-relevancy.html
fast/css/rtl-ordering.html
editing/selection/5240265.html
fast/dom/HTMLMeterElement/meter-boundary-values.html
fast/css/css-selector-text.html
fast/block/float/float-avoidance.html
Comment 6 Alexis Menard (darktears) 2012-03-08 10:02:10 PST
(In reply to comment #5)
> (From update of attachment 130488 [details])
> Attachment 130488 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11881034
> 
> New failing tests:
> fast/dom/HTMLMeterElement/meter-styles-changing-pseudo.html
> fast/dom/HTMLMeterElement/meter-writing-mode.html
> fast/css/unknown-pseudo-element-matching.html
> fast/dom/HTMLMeterElement/meter-element-repaint-on-update-value.html
> editing/selection/4397952.html
> fast/dom/HTMLMeterElement/meter-appearances-capacity.html
> http/tests/security/isolatedWorld/userGestureEvents.html
> http/tests/inspector/inspect-element.html
> accessibility/aria-disabled.html
> fast/css/css-set-selector-text.html
> fast/dom/HTMLMeterElement/meter-element.html
> fast/css/margin-top-bottom-dynamic.html
> css2.1/20110323/abspos-containing-block-initial-004d.htm
> fast/dom/HTMLProgressElement/progress-bar-value-pseudo-element.html
> fast/dom/HTMLTextAreaElement/reset-textarea.html
> fast/dom/HTMLInputElement/input-slider-update.html
> fast/css/continuationCrash.html
> css2.1/20110323/abspos-containing-block-initial-004b.htm
> fast/dom/HTMLProgressElement/progress-writing-mode.html
> fast/dom/HTMLTableColElement/resize-table-using-col-width.html
> fast/dom/HTMLInputElement/input-slider-update-styled.html
> fast/dom/HTMLMeterElement/meter-styles.html
> fast/dom/HTMLMeterElement/meter-optimums.html
> fast/css/button-height.html
> fast/dom/HTMLMeterElement/meter-appearances-rating-relevancy.html
> fast/css/rtl-ordering.html
> editing/selection/5240265.html
> fast/dom/HTMLMeterElement/meter-boundary-values.html
> fast/css/css-selector-text.html
> fast/block/float/float-avoidance.html

I tracked down those failure and I know why they happen.

The problem with my patch is that it filters away all unknown pseudo elements. While it's fine for the test case I have added it unfortunately fires back when it comes to webkit specific pseudo elements. We find many of them in html.css (e.g ::-webkit-textfield-decoration-container, ::-webkit-validation-bubble, ...). Those are treated as unknown pseudo elements so they are filtered away and no rules are created which explains the failures. CSSSelector.h contains the list of valid pseudo elements and those are not part of this list.

One approach could be to make those pseudo element not unknown by supporting them in CSSSelector.h, the problem is that they seem to be added a bit randomly  and I'm not sure we want to pollute the enum of CSSSelector with lot of values.

One hack will be to create an additional enum value PseudoWebKit that will be set to true whenever the string of the pseudo element is starting with -webkit-*. It will solve the use case of the reporter by filtering away everything that doesn't exist (::foobar) but we still not protect this case ::-webkit-FooBar. It will also fix the layout tests failure by letting through all webkit specific declaration.

Any thought? Better ideas?
Comment 7 Erik Arvidsson 2012-03-08 10:42:05 PST
I believe we should try to get the spec changed here. With this change the following would not work:

#test, ::-webkit-scrollbar {
  background: red;
}

and indeed this breaks in all but webkit.

http://plexode.com/eval3/#ht=%3Cstyle%3E%0A%23test%2C%20%3A%3A-webkit-scrollbar%20{%0A%20%20background%3A%20red%3B%0A}%0A%0A%23test%20{%0A%20%20width%3A%20100px%3B%0A%20%20height%3A%20100px%3B%0A%20%20overflow%3A%20scroll%3B%0A}%0A%3C%2Fstyle%3E%0A%0A%3Cdiv%20id%3Dtest%3E%3C%2Fdiv%3E&ohh=1&ohj=1&jt=&ojh=1&ojj=1&ms=100&oth=0&otj=0&cex=1

I think the right behavior would be to allow unrecognized pseudo classes and unrecognized pseudo elements
Comment 8 Dimitri Glazkov (Google) 2012-03-08 10:56:15 PST
(In reply to comment #7)
> I believe we should try to get the spec changed here. With this change the following would not work:
> 
> #test, ::-webkit-scrollbar {
>   background: red;
> }
> 
> and indeed this breaks in all but webkit.
> 
> http://plexode.com/eval3/#ht=%3Cstyle%3E%0A%23test%2C%20%3A%3A-webkit-scrollbar%20{%0A%20%20background%3A%20red%3B%0A}%0A%0A%23test%20{%0A%20%20width%3A%20100px%3B%0A%20%20height%3A%20100px%3B%0A%20%20overflow%3A%20scroll%3B%0A}%0A%3C%2Fstyle%3E%0A%0A%3Cdiv%20id%3Dtest%3E%3C%2Fdiv%3E&ohh=1&ohj=1&jt=&ojh=1&ojj=1&ms=100&oth=0&otj=0&cex=1
> 
> I think the right behavior would be to allow unrecognized pseudo classes and unrecognized pseudo elements

I actually intentionally relaxed this in WebKit (https://bugs.webkit.org/show_bug.cgi?id=46595), so that we can use pseudoelements in shadow DOM. I am ambivalent on whether using pseudoelement syntax is the right approach here, but it is already present in WebKit.
Comment 9 Boris Zbarsky 2012-03-08 20:39:00 PST
> I believe we should try to get the spec changed here

It might be a bit of a hard sell given that everyone except WebKit implements the spec correctly and specifying any other behavior is nontrivial (because unknown pseudo-elements can have all sorts of arbitrary syntax).