Bug 100567 – Allow :before and :after on radio buttons and checkboxes

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Elliott Sprehn]

Allow :before and :after on radio buttons and checkboxes

[Elliott Sprehn]

Created attachment 171031 [details]
Patch

[Elliott Sprehn]

See https://bugs.webkit.org/show_bug.cgi?id=98836 for context on this hack.

[Elliott Sprehn]

Created attachment 171033 [details]
Patch

[Eric Seidel (no email)]

I think this is what the "Rebaseline" keyword is for. :)

If the hack isn't tested, I suspect it will break.  Despite the annoyance, I think we need some testing.

[Elliott Sprehn]

Created attachment 171039 [details]
Patch

[Elliott Sprehn]

(In reply to comment #4)
> I think this is what the "Rebaseline" keyword is for. :)
> 
> If the hack isn't tested, I suspect it will break.  Despite the annoyance, I think we need some testing.

Okay, I added a test. I'll rebaseline it after it lands.

[Elliott Sprehn]

(In reply to comment #6)
> (In reply to comment #4)
> > I think this is what the "Rebaseline" keyword is for. :)
> > 
> > If the hack isn't tested, I suspect it will break.  Despite the annoyance, I think we need some testing.
> 
> Okay, I added a test. I'll rebaseline it after it lands.

Also those failures are happening on the bots, someone else made the tree red.

[Eric Seidel (no email)]

Comment on attachment 171039 [details]
Patch

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

LGTM besides the ChangeLog typo.

> Source/WebCore/ChangeLog:19
> +        No new tests, it's really hard to test what generated content looks like
> +        on a form control without using a pixel test, and I don't think generating
> +        tons of baselines right now is worth it for this hack.

:)

[Elliott Sprehn]

(In reply to comment #8)
> (From update of attachment 171039 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171039&action=review
> 
> LGTM besides the ChangeLog typo.
> 
> > Source/WebCore/ChangeLog:19
> > +        No new tests, it's really hard to test what generated content looks like
> > +        on a form control without using a pixel test, and I don't think generating
> > +        tons of baselines right now is worth it for this hack.
> 
> :)

That seems to be in an older version of the patch, now the one you r+'ed? Not sure what typo you mean.

[Elliott Sprehn]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 171039 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=171039&action=review
> > 
> > LGTM besides the ChangeLog typo.
> > 
> > > Source/WebCore/ChangeLog:19
> > > +        No new tests, it's really hard to test what generated content looks like
> > > +        on a form control without using a pixel test, and I don't think generating
> > > +        tons of baselines right now is worth it for this hack.
> > 
> > :)
> 
> That seems to be in an older version of the patch, now the one you r+'ed? Not sure what typo you mean.

Oh I'm dumb. Nevermind.

[Elliott Sprehn]

Created attachment 171041 [details]
Patch for landing

[Elliott Sprehn]

(In reply to comment #11)
> Created an attachment (id=171041) [details]
> Patch for landing

@eseidel can we get this landed so the buttons start working again in Chrome Canary?

[Build Bot]

Comment on attachment 171041 [details]
Patch for landing

Attachment 171041 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14593965

New failing tests:
fast/forms/pseudo-elements-appearance-none.html
fast/multicol/span/span-as-immediate-columns-child-dynamic.html
fast/multicol/span/positioned-objects-not-removed-crash.html
fast/multicol/span/textbox-not-removed-crash.html
fast/multicol/span/span-as-immediate-child-generated-content.html
fast/lists/remove-listmarker-from-anonblock-with-continuation-crash.html
fast/multicol/anonymous-block-split-crash.html

[WebKit Review Bot]

Comment on attachment 171041 [details]
Patch for landing

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

New failing tests:
fast/multicol/span/span-as-immediate-child-generated-content.html
fast/multicol/span/positioned-objects-not-removed-crash.html
fast/multicol/anonymous-block-split-crash.html
fast/lists/remove-listmarker-from-anonblock-with-continuation-crash.html
fast/multicol/span/textbox-not-removed-crash.html

[Dimitri Glazkov (Google)]

How can I help? Looks like there are failing tests...

[Elliott Sprehn]

(In reply to comment #15)
> How can I help? Looks like there are failing tests...

It would seem I forgot to add [ Failure ] for mac TestExpectations. I didn't realize that mac ews ran tests now.

I think the crashes may be unrelated, two of them don't even use :before or :after.

I'm away from my computer this weekend and only have a Chromebook, could you upload a patch that copies the TestExpectations line for the mac port?

[WebKit Review Bot]

Comment on attachment 171041 [details]
Patch for landing

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

New failing tests:
fast/multicol/span/span-as-immediate-child-generated-content.html
fast/multicol/span/positioned-objects-not-removed-crash.html
fast/multicol/anonymous-block-split-crash.html
fast/lists/remove-listmarker-from-anonblock-with-continuation-crash.html
fast/multicol/span/textbox-not-removed-crash.html

[Dimitri Glazkov (Google)]

Can we not just roll out 98836, wait until Chrome Web UI is fixed, and then reland it? This fix seems nasty and not in line with the spirit of what Tab was suggesting.

[Elliott Sprehn]

(In reply to comment #18)
> Can we not just roll out 98836, wait until Chrome Web UI is fixed, and then reland it? This fix seems nasty and not in line with the spirit of what Tab was suggesting.

We can roll it back if you want, this patch seemed like a good compromise since it doesn't break existing pages and unblocks the other patches. I figured we could do the full removal as the next step.

[Elliott Sprehn]

(In reply to comment #19)
> (In reply to comment #18)
> > Can we not just roll out 98836, wait until Chrome Web UI is fixed, and then reland it? This fix seems nasty and not in line with the spirit of what Tab was suggesting.
> 
> We can roll it back if you want, this patch seemed like a good compromise since it doesn't break existing pages and unblocks the other patches. I figured we could do the full removal as the next step.

Actually, yeah lets just roll it back and I'll fix Chrome on Monday.

[Dimitri Glazkov (Google)]

(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > Can we not just roll out 98836, wait until Chrome Web UI is fixed, and then reland it? This fix seems nasty and not in line with the spirit of what Tab was suggesting.
> > 
> > We can roll it back if you want, this patch seemed like a good compromise since it doesn't break existing pages and unblocks the other patches. I figured we could do the full removal as the next step.
> 
> Actually, yeah lets just roll it back and I'll fix Chrome on Monday.

https://bugs.webkit.org/show_bug.cgi?id=100609