Bug 91723 - display:inline's hover behavior is not applied to ::before and ::after pseudo elements
: display:inline's hover behavior is not applied to ::before and ::after pseudo...
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-07-19 00:53 PST by
Modified: 2013-03-04 13:09 PST (History)


Attachments
Reduced test case (CSS hover style only) (379 bytes, text/html)
2012-07-19 00:53 PST, Tim Okrongli
no flags Details
Another testpage with more examples (1.36 KB, text/html)
2012-10-08 06:28 PST, Martin Leutelt
no flags Details
Proposed patch (10.37 KB, patch)
2012-10-12 04:24 PST, Martin Leutelt
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Proposed patch (6.93 KB, patch)
2012-10-12 06:47 PST, Martin Leutelt
buildbot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Updated patch (13.82 KB, patch)
2012-10-16 01:32 PST, Martin Leutelt
no flags Review Patch | Details | Formatted Diff | Diff
Patch (4.36 KB, patch)
2013-01-09 18:43 PST, Elliott Sprehn
no flags Review Patch | Details | Formatted Diff | Diff
Patch (4.49 KB, patch)
2013-01-14 15:12 PST, Elliott Sprehn
no flags Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2012-07-19 00:53:50 PST
Created an attachment (id=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
------- Comment #1 From 2012-09-13 02:52:38 PST -------
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.
------- Comment #2 From 2012-10-08 02:44:26 PST -------
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...
------- Comment #3 From 2012-10-08 06:28:10 PST -------
Created an attachment (id=167532) [details]
Another testpage with more examples
------- Comment #4 From 2012-10-12 00:40:50 PST -------
Found the problem, patch is coming soon.
------- Comment #5 From 2012-10-12 04:24:48 PST -------
Created an attachment (id=168393) [details]
Proposed patch
------- Comment #6 From 2012-10-12 05:15:35 PST -------
(From update of attachment 168393 [details])
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
------- Comment #7 From 2012-10-12 06:47:46 PST -------
Created an attachment (id=168414) [details]
Proposed patch
------- Comment #8 From 2012-10-12 08:11:28 PST -------
(From update of attachment 168414 [details])
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
------- Comment #9 From 2012-10-12 10:51:49 PST -------
(From update of attachment 168414 [details])
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
------- Comment #10 From 2012-10-15 01:14:52 PST -------
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.
------- Comment #11 From 2012-10-16 01:32:23 PST -------
Created an attachment (id=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.
------- Comment #12 From 2012-10-16 01:43:45 PST -------
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? :)
------- Comment #13 From 2012-10-16 02:29:28 PST -------
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.
------- Comment #14 From 2012-10-16 11:45:15 PST -------
(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?
------- Comment #15 From 2012-10-16 23:34:56 PST -------
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.
------- Comment #16 From 2013-01-08 01:49:35 PST -------
@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?
------- Comment #17 From 2013-01-09 18:43:28 PST -------
Created an attachment (id=182042) [details]
Patch
------- Comment #18 From 2013-01-09 18:46:57 PST -------
(In reply to comment #17)
> Created an attachment (id=182042) [details] [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.
------- Comment #19 From 2013-01-09 19:09:31 PST -------
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.
------- Comment #20 From 2013-01-09 20:49:08 PST -------
(From update of attachment 182042 [details])
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
------- Comment #21 From 2013-01-09 21:50:46 PST -------
(From update of attachment 182042 [details])
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
------- Comment #22 From 2013-01-09 22:50:18 PST -------
(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.
------- Comment #23 From 2013-01-10 02:16:51 PST -------
(In reply to comment #22)
> (From update of attachment 182042 [details] [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.
------- Comment #24 From 2013-01-14 15:12:48 PST -------
Created an attachment (id=182636) [details]
Patch
------- Comment #25 From 2013-01-14 22:58:15 PST -------
(From update of attachment 182636 [details])
LGTM.  I'll make sure carewolf is CC'd as he's been in hittesting most recently.
------- Comment #26 From 2013-01-15 07:22:07 PST -------
(From update of attachment 182636 [details])
I've tested your patch on the test subject and it works as expected, thanks for updating the fix Elliott :)
------- Comment #27 From 2013-01-15 07:44:21 PST -------
(From update of attachment 182636 [details])
Clearing flags on attachment: 182636

Committed r139739: <http://trac.webkit.org/changeset/139739>
------- Comment #28 From 2013-01-15 07:44:28 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #29 From 2013-03-01 18:06:43 PST -------
*** Bug 58422 has been marked as a duplicate of this bug. ***