Bug 98836

Summary:

Generated should not be supported for things with a shadow

Product:

WebKit

Reporter:

Elliott Sprehn <esprehn>

Component:

Layout and Rendering

Assignee:

Elliott Sprehn <esprehn>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov, eric, morrita, ojan, raviksharma9021, tabatkins, tasak, tkent, webcomponents-bugzilla, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

98687, 98685, 100412, 100565, 100609

Bug Blocks:

59827, 95117, 100161

Attachments:

Description	Flags
Patch	none
Patch	none
Patch for landing	none
Patch for landing	none
Patch for re-landing	none

Elliott Sprehn

Reported 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

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Elliott Sprehn

Comment 1 2012-10-18 15:58:49 PDT

Created attachment 169495 [details] Patch

WebKit Review Bot

Comment 2 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

Elliott Sprehn

Comment 3 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.

Elliott Sprehn

Comment 4 2012-10-19 15:01:01 PDT

Created attachment 169703 [details] Patch

Elliott Sprehn

Comment 5 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.

Kent Tamura

Comment 6 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.

Elliott Sprehn

Comment 7 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? :)

Elliott Sprehn

Comment 8 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

Elliott Sprehn

Comment 9 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.

Hajime Morrita

Comment 10 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.

Dimitri Glazkov (Google)

Comment 11 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.

Dimitri Glazkov (Google)

Comment 12 2012-10-23 09:13:20 PDT

Comment on attachment 169703 [details] Patch The pills are all purple. Can haz green pills before land?

Elliott Sprehn

Comment 13 2012-10-23 13:23:09 PDT

Created attachment 170216 [details] Patch for landing

Elliott Sprehn

Comment 14 2012-10-23 14:03:45 PDT

Created attachment 170233 [details] Patch for landing

WebKit Review Bot

Comment 15 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>

WebKit Review Bot

Comment 16 2012-10-23 14:41:03 PDT

All reviewed patches have been landed. Closing bug.

Elliott Sprehn

Comment 17 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)?

Elliott Sprehn

Comment 18 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.

Hajime Morrita

Comment 19 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.

Kent Tamura

Comment 20 2012-10-25 05:51:19 PDT

Why did this affect checkbox and radio? They don't have shadow trees, right?

Dimitri Glazkov (Google)

Comment 21 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.

Tab Atkins

Comment 22 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?

Eric Seidel (no email)

Comment 23 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.

WebKit Review Bot

Comment 24 2012-10-25 13:33:27 PDT

Re-opened since this is blocked by bug 100412

Vincent Scheib

Comment 25 2012-10-26 14:30:41 PDT

Created attachment 171011 [details] Patch for re-landing

Vincent Scheib

Comment 26 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.

WebKit Review Bot

Comment 27 2012-10-26 14:56:06 PDT

Re-opened since this is blocked by bug 100565

Elliott Sprehn

Comment 28 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.

Note You need to log in before you can comment on or make changes to this bug.