Bug 9561 – REGRESSION: Content property on :before of button causes hang on click or hover

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Troy Brandt]

The content property in this example causes WebKit to hang. 

The example uses this:

 #submit:before {
content: "\2714";
}
#clear:before {
content: "\2718";
text-shadow: black 0.2em 0.1em 0.1em
}

And on load the app hangs. Removing the content property allows the page to load normally. 

This works as expected in Safari 2.0.3 and Camino.

[Troy Brandt]

Created attachment 8993 [details]
Example document.

Created attachment 8998 [details]
Reduction of test case, non-aqua button

Hovering the button in this test case will hang Safari.

Created attachment 8999 [details]
Reduction of test case, aqua button

When reducing the test case even further, the button reverts to an aqua look, and doesn't seem to react to hovering at all, so WebKit doesn't hang. Clicking the button hangs WebKit, though.

I just reduced the original example a bit, to make it more to the point. It seems that it is the style change on hovering or clicking that triggers the hang.

(In reply to comment #3)
> Created an attachment (id=8999) [edit]
> Reduction of test case, aqua button 

A debug build fails the assertion
KHTMLAssert(beforeChild->parent()->isAnonymousBlock())
in RenderBlock::addChildToFlow() when I click the button.

Created attachment 9019 [details]
Possible fix, no test or change log yet

This patch is somewhat cumbersome. Another approach I considered was to patch RenderButton::setStyle() and RenderButton::addChild() to copy the pseudostyles to the inner div, but I had a feeling it would end up being at least as complicated as this patch's approach.

[Darin Adler]

Comment on attachment 9019 [details]
Possible fix, no test or change log yet

I'm not a big fan of functions that have those "do" prefixes. It's always hard to know how they differ from the version without the "do" prefix. It's just "stage 2". We should seek out a better name for the non-virtual core implementation of updatePseudoChild.

In what cases will it ever be good for RenderButton to call doUpdatePseudoChild on itself rather than on m_inner?

Architecturally, the worst thing here is the knowledge in RenderContainer::updatePseudoChild that the parent is special when it's a RenderButton. That needs a comment at least. And we should see what we can do to avoid this new slightly-fragile linkage between the classes. Even having a virtual function that was named something cumbersome like "updatesPseudoChildForChildren" might be better.

(In reply to comment #7) 
> We should seek out a better name for the non-virtual core
> implementation of updatePseudoChild.

I'll give it more thought.

> In what cases will it ever be good for RenderButton to call doUpdatePseudoChild
> on itself rather than on m_inner?

<style>
    button:hover:before { content: 'foo' }
</style>
<button></button>

The first time you hover, m_inner is 0.

> Architecturally, the worst thing here is the knowledge in
> RenderContainer::updatePseudoChild that the parent is special when it's a
> RenderButton. That needs a comment at least. And we should see what we can do
> to avoid this new slightly-fragile linkage between the classes. Even having a
> virtual function that was named something cumbersome like
> "updatesPseudoChildForChildren" might be better.

I would just like to mention that there is already one virtual function that uniquely identifies buttons, and it's called allowsReusingAnonymousChild() :-).

[Dave Hyatt]

Comment on attachment 9019 [details]
Possible fix, no test or change log yet

Gist of the fix looks right.  I would actually recommend reusing the virtual "allowsReusingAnonymousChild" for this code.  It has the advantage of being slightly more generic (since I expect the select popup control may run into this issue too) and entirely appropriate.  Then you don't need the new isButton method at all.

[Dave Hyatt]

Ah, I should have read the comments (just jumped right to the patch from the review queue). :)

Created attachment 9065 [details]
Updated patch, including change log and test

I unified !allowsReusingAnonymousChild() with isButton() and renamed it createsAnonymousWrapper(). Also renamed doUpdatePseudoChild() to updatePseudoChildForObject() but I'll welcome other suggestions.

[Darin Adler]

Comment on attachment 9065 [details]
Updated patch, including change log and test

Looks good. r=me

Perhaps we can think of an even better name for the updatePseudoChildForObject function some day, but this seems good.

[Darin Adler]

Committed revision 15079.