display:inline's hover behavior is not applied to ::before and ::after pseudo elements

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Okrongli]

Created attachment 153198 [details]
Reduced test case (CSS hover style only)

When an element has "display: inline" the ::before and ::after pseudo-elements are not considered to be part of the actual elements for hovering purposes (hovering over them neither causes the :hover styles to be applied nor does it fire an onmouseover event). This is not the case, however, when a <p> has "display: inline" set.

Steps to reproduce:
1. Open the attached document.
2. Hover over the elements themselves (the upper case tag names) and their pseudo-elements (the lower case "before" and "after").

Expected behavior:
All lines behave the same way: Either the lines turn blue when the pseudo-elements are hovered (like in other browsers) or they only turn blue when the element proper is hovered.

Observed behavior:
The "SPAN" and "DIV" lines only turn blue when the element proper is hovered but the "P" line also turns blue when the pseudo-elements are hovered.

Observed with:
WebKit 534.55.3, Safari 5.1.5, Windows Vista
WebKit 563.5, Chrome 19.0.1084.52, Windows Vista
WebKit 535.19, Chromium 18.0.1025.168, Ubuntu Linux 11.10
WebKit 534.34, Arora 0.11.0, Ubuntu Linux 11.10

[Martin Leutelt]

This indeed looks like a bug. For the time being maybe you can work around it by using 'inline-block' instead of 'inline', as that seems to work fine.
With Firefox 13 and Opera 12 'inline' works fine so I guess that is the behavior WebKit should mirror.

[Martin Leutelt]

I had another look at this and I noticed some interesting behavior. Hovering on the pseudo elements starts working when:
- no style is set at all or display is set to inline-block
- display is inline AND one of the following things is set:
 - tabindex is set on the element and it's being focused by pressing the tab key (or set the style to :hover with the inspector)
 - a border is defined ('solid' works but 'none' doesn't, so it has to be visible)
 - a background color set ('transparent' doesn't work)

If someone has a hint what the actual problem could be please comment. I'm trying to debug this but so far I'm not sure what part of the code could be to blame...

[Martin Leutelt]

Created attachment 167532 [details]
Another testpage with more examples

[Martin Leutelt]

Found the problem, patch is coming soon.

[Martin Leutelt]

Created attachment 168393 [details]
Proposed patch

[WebKit Review Bot]

Comment on attachment 168393 [details]
Proposed patch

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

New failing tests:
fast/selectors/hovering-before-after-pseudo-elements.html
fast/text/capitalize-empty-generated-string.html

[Martin Leutelt]

Created attachment 168414 [details]
Proposed patch

[Build Bot]

Comment on attachment 168414 [details]
Proposed patch

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

New failing tests:
fast/text/capitalize-empty-generated-string.html

[WebKit Review Bot]

Comment on attachment 168414 [details]
Proposed patch

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

New failing tests:
fast/text/capitalize-empty-generated-string.html

[Martin Leutelt]

I'm not sure if that one failing test simply needs rebaselining or if I have to modify my patch. The expected render tree before my patch shows a RenderInline with a 0x0 size being created, with my patch the same RenderInline is created with size 0x19 which seems correct to me. 
I would be greatful if someone could comment on this.

[Martin Leutelt]

Created attachment 168888 [details]
Updated patch

I updated the expected.txt files for qt, gtk, efl and mac. Skipping the failing test for chromium, the expected.txt files 
there need to be rebaselined.

[Elliott Sprehn]

Can you explain what's going on in this patch? It's not clear what's different from normal DOM nodes compared to generated content. Why do you need this override to always create line boxes for generated content? :)

[Martin Leutelt]

For some elements (h1-h6, p, small, ...) the code did make some assumptions that were also true when creating content with the before and after css selectors. Example: h1-h6 have styles applied to them automatically so hasMargin() will turn out to be true. 

For other elements however (like div and a and a couple more) those assumptions aren't true necessarily, that's why an additional check to see if pseudo styles are applied is needed in those cases.

[Elliott Sprehn]

(In reply to comment #13)
> For some elements (h1-h6, p, small, ...) the code did make some assumptions that were also true when creating content with the before and after css selectors. Example: h1-h6 have styles applied to them automatically so hasMargin() will turn out to be true. 
> 
> For other elements however (like div and a and a couple more) those assumptions aren't true necessarily, that's why an additional check to see if pseudo styles are applied is needed in those cases.

Hmm, It's still not clear to me why the :before/:after is different than the normal text node inside the <div>. Why does that get a line box but the generated content doesn't?

[Martin Leutelt]

Yeah, now that you say it I'm not sure either. The thing is though that while debugging I saw that for the cases where hovering on the pseudo elements works line boxes are always created for elements like h1, p and so on but for the not working cases this branch of the code was never hit. The additional check fixes the bug and as far as I understand doesn't introduce any other issues. If you have a suggestion on how to fix this in a better way I'm happy to hear it.

[Elliott Sprehn]

@dhyatt Can you explain what's going on here? What's the purpose of the alwaysCreateLineBoxes boolean, and why does padding or margin affect this?

Adding padding/margin to the test case makes the hover start working since we create line boxes, but it's not clear why the generated content didn't create the line boxes to begin with. Is this because the text in the generated content has no Text node?

[Elliott Sprehn]

Created attachment 182042 [details]
Patch

[Elliott Sprehn]

(In reply to comment #17)
> Created an attachment (id=182042) [details]
> Patch

I figured out the real issue which is that we hit the anonymous RenderText during hit testing and should actually hit the generated content node itself. Fixing this is much nicer than forcing the creation of line boxes for everything with :before or :after which would defeat the memory optimization from linebox culling.

[Elliott Sprehn]

Hmm, it's interesting that the mac port fails but the chromium one doesn't, I think this might be because we didn't cause a layout when we do the hover as mentioned in the original patch on this bug.

[Build Bot]

Comment on attachment 182042 [details]
Patch

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

New failing tests:
fast/css-generated-content/hover-inline.html

[Build Bot]

Comment on attachment 182042 [details]
Patch

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

New failing tests:
fast/css-generated-content/hover-inline.html

[Eric Seidel (no email)]

Comment on attachment 182042 [details]
Patch

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

> Source/WebCore/rendering/RenderObject.cpp:2628
> +    // If we hit the anonymous renderers inside generated content we should
> +    // actually hit the generated content so walk up to the PseudoElement.
> +    if (!node && parent() && parent()->isBeforeOrAfterContent()) {
> +        for (RenderObject* renderer = parent(); renderer && !node; renderer = renderer->parent())
> +            node = renderer->node();
> +    }

It's difficult for me to judge, what, if any other anonymous renderers will be effected by this.

[Elliott Sprehn]

(In reply to comment #22)
> (From update of attachment 182042 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182042&action=review
> 
> > Source/WebCore/rendering/RenderObject.cpp:2628
> > +    // If we hit the anonymous renderers inside generated content we should
> > +    // actually hit the generated content so walk up to the PseudoElement.
> > +    if (!node && parent() && parent()->isBeforeOrAfterContent()) {
> > +        for (RenderObject* renderer = parent(); renderer && !node; renderer = renderer->parent())
> > +            node = renderer->node();
> > +    }
> 
> It's difficult for me to judge, what, if any other anonymous renderers will be effected by this.

It should only have an effect on things inside :before and :after because of the check around it. Inside of generated content it only applies to the anonymous RenderText (which includes RenderCounter and RenderQuote), RenderImage and RenderTextFragment from :first-letter. In all those cases hitting the anonymous thing should actually hit the PseudoElement.

In an ideal world we'd create Text nodes and HTMLImageElement's for each content item and not need this special case, but when I first tried that editing and selection had lots of problems. We should try again in the future though.

[Elliott Sprehn]

Created attachment 182636 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 182636 [details]
Patch

LGTM.  I'll make sure carewolf is CC'd as he's been in hittesting most recently.

[Bruno Abinader (history only)]

Comment on attachment 182636 [details]
Patch

I've tested your patch on the test subject and it works as expected, thanks for updating the fix Elliott :)

[WebKit Review Bot]

Comment on attachment 182636 [details]
Patch

Clearing flags on attachment: 182636

Committed r139739: <http://trac.webkit.org/changeset/139739>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Martin Robinson]

*** Bug 58422 has been marked as a duplicate of this bug. ***