Bug 98836 - Generated should not be supported for things with a shadow
Summary: Generated should not be supported for things with a shadow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Elliott Sprehn
URL:
Keywords:
Depends on: 98687 98685 100412 100565 100609
Blocks: 59827 100161 95117
  Show dependency treegraph
 
Reported: 2012-10-09 17:07 PDT by Elliott Sprehn
Modified: 2019-05-03 19:52 PDT (History)
10 users (show)

See Also:


Attachments
Patch (142.51 KB, patch)
2012-10-18 15:58 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (143.27 KB, patch)
2012-10-19 15:01 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch for landing (136.00 KB, patch)
2012-10-23 13:23 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch for landing (142.43 KB, patch)
2012-10-23 14:03 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch for re-landing (206.96 KB, patch)
2012-10-26 14:30 PDT, Vincent Scheib
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Elliott Sprehn 2012-10-09 17:07:41 PDT
As far as CSS is concerned :before and :after don't apply to things like <input> because they're opaque replaced content. At one point there was code to prevent this, but the new Shadow DOM based implementations of some input types started supporting it but shouldn't have (yet). It looks like we only supported it for inputs of type="time" / month, week, date anyway.

The new generated content implementation doesn't support generated content on things with shadows (yet) so these tests now fail. Until we know what to do with generated content when you have a shadow we should just hide it when you have a shadow.

See:

fast/forms/time-multiple-fields/time-multiple-fields-appearance-pseudo-elements.html
fast/forms/month-multiple-fields/month-multiple-fields-appearance-pseudo-elements.html
fast/forms/week-multiple-fields/week-multiple-fields-appearance-pseudo-elements.html
fast/forms/date-multiple-fields/date-multiple-fields-appearance-pseudo-elements.html
Comment 1 Elliott Sprehn 2012-10-18 15:58:49 PDT
Created attachment 169495 [details]
Patch
Comment 2 WebKit Review Bot 2012-10-18 18:52:29 PDT
Comment on attachment 169495 [details]
Patch

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

New failing tests:
fast/forms/pseudo-elements.html
Comment 3 Elliott Sprehn 2012-10-18 19:42:31 PDT
(In reply to comment #2)
> (From update of attachment 169495 [details])
> Attachment 169495 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/14459272
> 
> New failing tests:
> fast/forms/pseudo-elements.html

Hmm, this doesn't fail locally. I'll build it on cr-linux and try to run the tests. We should figure out how to make it email you the failing test output too.
Comment 4 Elliott Sprehn 2012-10-19 15:01:01 PDT
Created attachment 169703 [details]
Patch
Comment 5 Elliott Sprehn 2012-10-19 15:16:20 PDT
(In reply to comment #4)
> Created an attachment (id=169703) [details]
> Patch

I figured out the test failures. We were missing a canHaveGeneratedChildren() { return false; } on RenderListBox which seems to be just an oversight. We already have it on RenderMenuList. The only reason this appears to pass on Mac is because the generated content is above the first multi select you can't see it.
Comment 6 Kent Tamura 2012-10-21 18:12:47 PDT
Comment on attachment 169703 [details]
Patch

The behavior changes for RenderListBox and multiple-fields input elements is ok.  However I'm not sure if we should disable generated contents for shadow hots at all.
Comment 7 Elliott Sprehn 2012-10-21 21:32:20 PDT
(In reply to comment #6)
> (From update of attachment 169703 [details])
> The behavior changes for RenderListBox and multiple-fields input elements is ok.  However I'm not sure if we should disable generated contents for shadow hots at all.

There's still debate about how :before and :after should work with shadow. For example Tab was saying we should make <content> select them and project them into the shadow and that's really hard with the current implementation, but will be much easier with the new implementation.

I'd really like to get this landed as a temporary measure so people don't depend on our unspecified behavior which might have to change and because the new generated content implementation is going to temporarily disable support. Getting the new generated content landed is really important because it removes a lot of complexity, fixes lots of bugs, and gives us support for things like animating pseudos.

Once the new generated content implementation in Bug 98836 is relanded we can add support back. I certainly don't think we should disable support forever. Does that work for you? :)
Comment 8 Elliott Sprehn 2012-10-21 21:33:44 PDT
(In reply to comment #7)
> (In reply to comment #6)
> ...
> Once the new generated content implementation in Bug 98836 is relanded

Sorry that's Bug 95117
Comment 9 Elliott Sprehn 2012-10-22 22:59:16 PDT
@dglazgov @morrita Does the above plan work for you? This blocks the new generated content implementation that should make our lives easier after it lands. :) Then we can bring support back for :before and :after on shadows after Tab/Dimitri updates the spec.
Comment 10 Hajime Morrita 2012-10-22 23:35:12 PDT
(In reply to comment #9)
> @dglazgov @morrita Does the above plan work for you? This blocks the new generated content implementation that should make our lives easier after it lands. :) Then we can bring support back for :before and :after on shadows after Tab/Dimitri updates the spec.

The plan sounds reasonable. Could you file a bug for get shadow+generated back?
I'm not sure how we can support new-style generated contents for element with shadow.
We need to figure out the way after Bug 95117 is landed.
Comment 11 Dimitri Glazkov (Google) 2012-10-23 09:12:21 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > @dglazgov @morrita Does the above plan work for you? This blocks the new generated content implementation that should make our lives easier after it lands. :) Then we can bring support back for :before and :after on shadows after Tab/Dimitri updates the spec.
> 
> The plan sounds reasonable. Could you file a bug for get shadow+generated back?
> I'm not sure how we can support new-style generated contents for element with shadow.
> We need to figure out the way after Bug 95117 is landed.

SGTM.
Comment 12 Dimitri Glazkov (Google) 2012-10-23 09:13:20 PDT
Comment on attachment 169703 [details]
Patch

The pills are all purple. Can haz green pills before land?
Comment 13 Elliott Sprehn 2012-10-23 13:23:09 PDT
Created attachment 170216 [details]
Patch for landing
Comment 14 Elliott Sprehn 2012-10-23 14:03:45 PDT
Created attachment 170233 [details]
Patch for landing
Comment 15 WebKit Review Bot 2012-10-23 14:40:58 PDT
Comment on attachment 170233 [details]
Patch for landing

Clearing flags on attachment: 170233

Committed r132269: <http://trac.webkit.org/changeset/132269>
Comment 16 WebKit Review Bot 2012-10-23 14:41:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Elliott Sprehn 2012-10-25 01:10:40 PDT
This broke the checkboxes and radio buttons in Chrome's settings page http://code.google.com/p/chromium/issues/detail?id=157664

It also appears that people have used the radio button/checkbox hack elsewhere:
http://www.inserthtml.com/2012/06/custom-form-radio-checkbox/
http://www.red-team-design.com/css-generated-content-replaced-elements

Mozilla has said they have no plans to implement it though because the spec said we shouldn't have supported it anyway, and it appears WebKit is the only browser that supports it (by accident too):
https://bugzilla.mozilla.org/show_bug.cgi?id=241985

This raises the question, should we add back support for checkboxes and radio buttons because fixing this bug on form controls could possibly break some WebKit specific apps (maybe iOS/Android only web apps)?
Comment 18 Elliott Sprehn 2012-10-25 01:24:39 PDT
(In reply to comment #17)
> ...
> 
> This raises the question, should we add back support for checkboxes and radio buttons because fixing this bug on form controls could possibly break some WebKit specific apps (maybe iOS/Android only web apps)?

Awesomely this also broke a demo from Eric Bidelman:

http://ericbidelman.tumblr.com/post/23615290220/data-binding-using-data-attributes

and the cool UI demo from a few months back:

http://lab.simurai.com/umbrui/

It's also notable that in all the comments on these pages lots of people mention the WebKit behavior as being a bug, ex. Lea Verou's comment and many others on the red team design page, or the first comment on Eric's blog post.

I'm leaning towards leaving this fix in, but it is unfortunate people started messing with it in the past year.
Comment 19 Hajime Morrita 2012-10-25 03:06:38 PDT
I believe certain number of mobile websites relies on this.

When I made this available for <input> by accident, some people took it as a good thing.
It looks people are aware of this kind of problem and hacks it intentionally.
That's so unfortunate. But we need to make it work since there is no easy replacement
unlike API unprefixing.
Comment 20 Kent Tamura 2012-10-25 05:51:19 PDT
Why did this affect checkbox and radio? They don't have shadow trees, right?
Comment 21 Dimitri Glazkov (Google) 2012-10-25 08:33:08 PDT
Argh. This sucks. Typically, if there's a content in the wild that uses non-standard, under-specified feature, we WebKit peeps are stuck with supporting it. Don't break the web and that sort of thing.
Comment 22 Tab Atkins 2012-10-25 10:32:32 PDT
(In reply to comment #19)
> I believe certain number of mobile websites relies on this.
> 
> When I made this available for <input> by accident, some people took it as a good thing.
> It looks people are aware of this kind of problem and hacks it intentionally.
> That's so unfortunate. But we need to make it work since there is no easy replacement
> unlike API unprefixing.

I'd like us to try just breaking those sites first, before we unilaterally decide we're stuck with it.  It appears that lots of authors *know* that it's WebKit-specific and technically a bug.

Do we know what kind of sites/numbers we're looking at?
Comment 23 Eric Seidel (no email) 2012-10-25 11:01:22 PDT
I would agree.  Not breaking it seems to be implicitly asking other engines to eventually add this quirk.
Comment 24 WebKit Review Bot 2012-10-25 13:33:27 PDT
Re-opened since this is blocked by bug 100412
Comment 25 Vincent Scheib 2012-10-26 14:30:41 PDT
Created attachment 171011 [details]
Patch for re-landing
Comment 26 Vincent Scheib 2012-10-26 14:38:56 PDT
Re-landed as http://trac.webkit.org/changeset/132696, will likely need baseline cleanup which I will monitor for.
Comment 27 WebKit Review Bot 2012-10-26 14:56:06 PDT
Re-opened since this is blocked by bug 100565
Comment 28 Elliott Sprehn 2012-11-30 16:08:51 PST
This was rolled out again because it breaks all the radio buttons and checkboxes in Chrome. We need a new patch that allows generated content on checkboxes and radio buttons with -webkit-appearance: none and also has this.