Bug 103283 – Hover effects from a GestureTapDown are dismissed by a GestureTap on the hover element

• Component: UI Events
• Status: RESOLVED FIXED

[Terry Anderson]

Steps to reproduce:
1. Go to http://jsfiddle.net/rbyers/9bK2D/
2. Tap with touch on 'Hover me' (causing the green 'Click me' element to appear)
3. Tap with touch on 'Click me'

What happens:
The GestureTap from step 3 will dismiss the hover effect before dispatching the tap event. This will cause the 'Click me' element to disappear and the tap will be incorrectly dispatched to "Don't click here".

What should happen instead:
The GestureTap from step 3 should dispatch the tap on the 'Click me' element, and this element should remain visible after the tap has completed.

[Terry Anderson]

Created attachment 176058 [details]
Patch

[Terry Anderson]

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

This patch fixes the problem, but @rbyers pointed out that we will probably get this fix for free as a result of the refactoring in webk.it/97040 (which won't introduce another 'hack' to handleGestureEvent()). Setting that issue as a blocker.

[Terry Anderson]

Created attachment 191523 [details]
WIP, not for review

[Terry Anderson]

Created attachment 191628 [details]
Patch

[Antonio Gomes]

Comment on attachment 191628 [details]
Patch

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

So, you want to proceed here, even if the other bug is fixing it differently?

At least the test is important

> Source/WebCore/page/EventHandler.cpp:2507
> +    case PlatformEvent::GestureTapDownCancel:

do you lack a break for this 'case'

[Antonio Gomes]

Comment on attachment 191628 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2403
> +void EventHandler::releaseFocus(const IntPoint& point)

I think releasing focus is different from releasing hover. Is this name accurate?

> Source/WebCore/page/EventHandler.cpp:2406
> +    hitTestResultAtPoint(hitTestPoint, HitTestRequest::Release | HitTestRequest::AllowFrameScrollbars);

do not you just need to call Document::updateHoverActiveState here?

[Terry Anderson]

(In reply to comment #5)
> (From update of attachment 191628 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191628&action=review
> 
> So, you want to proceed here, even if the other bug is fixing it differently?
> 
> At least the test is important

Yes, I don't think we will get this fixed as a result of the refactoring bug so I removed the dependency. 

Thanks for the feedback so far. I realized today that I want to do a bit more thinking about this problem before trying to land any code, so I'm going to clear the review flag for the time being.

[Terry Anderson]

Created attachment 193181 [details]
Patch

[Terry Anderson]

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

This new approach avoids adding another hit test and I believe it is more in line with mouse behavior as well. If you go to http://jsfiddle.net/rbyers/9bK2D/ and hover with your mouse cursor, you are able to mouse-click the "click me" div as many times as you want without the hover effect being dismissed. With this patch, the hover effect is not dismissed after gesture-tapping the "click me" div. Gesture-tapping or moving your mouse cursor outside of the "click me" div will still clear the hover effects, though.

[Rick Byers]

Comment on attachment 193181 [details]
Patch

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

> Source/WebCore/page/EventHandler.cpp:2481
> +        hitType |= HitTestRequest::ReadOnly;

This means we're now relying on the synthetic mouseup event to clear the :active state.  Conceptually I'd prefer if the synthetic mouse events were separate from achieving the correct hover/active behavior (since I think in the future we'll probably want a mechanism for disabling the synthetic mouse events without also disabling touch-based hover/active).  This might be OK - this is certainly a nice simple CL to address the problem, and I don't see any obvious problem that would result from doing it this way for now.  But you should at least add a comment here saying why it's OK for the hit test to be ReadOnly (that we expect a mouseup to clear the active state for us).

The alternative approach would be to modify HandleGestureTap to simulate mousedown/mouseup in a different way which either avoids the hit test or does only a ReadOnly hit test (eg. maybe by dispatching the DOM events directly, rather than simulating a real platform event).  There are good reason why we will want to do something like this sometime soon - eg. to be able to mark mouse events that were generated by touch (a common web platform feature request), and (more immediately) to fix the hover problem where we get mousemove events on scroll after tap.  If we switch to this approach then we'd probably need to modify Document::updateHoverActiveState to not clear hover states on touchRelease (today hover is probably getting cleared on GestureTap and set again on the synthetic mousedown - though I haven't verified this).

Anyway, I'm OK with this approach in the short term as long as you add a comment explaining it.  But I suspect we'll want to revisit after fixing the scroll after tap case.

> LayoutTests/fast/events/touch/gesture/gesture-tap-hover-clear.html:46
> +// Terry: change this 

Something you wanted to change here?

> LayoutTests/fast/events/touch/gesture/gesture-tap-hover-clear.html:72
> +    eventSender.gestureTap(250, 250);

It would be nice (given the test bugs we've had in the past) to double check here and below that you're taping on the element you expect (eg. with elementFromPoint).

[Terry Anderson]

Created attachment 193386 [details]
comments from rbyers addressed

[Terry Anderson]

Created attachment 193388 [details]
comments from rbyers addressed

[Rick Byers]

Looks good to me.  Antonio?

[Antonio Gomes]

Comment on attachment 193388 [details]
comments from rbyers addressed

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

> Source/WebCore/page/EventHandler.cpp:2483
> +    else if (gestureEvent.type() == PlatformEvent::GestureTap) {
> +        // The mouseup event synthesized for this gesture will clear the active state of the
> +        // targeted node, so performing a ReadOnly hit test here is fine.
> +        hitType |= HitTestRequest::ReadOnly;

I think it looks ok. Question: is it safe to generalize this?

Like the case you are trying to fix is essentially:
1 - click on 1
2 - 2 shows up as a result of hovering over 1
3 - click on 2
(3) fails before 1 gets unactive and dismisses the hover, making 2 disappear.

What if I just want to click (tap) on a element that responds to hover? Once I finish, would it remain as active till I click elsewhere?

[Terry Anderson]

(In reply to comment #14)

> I think it looks ok. Question: is it safe to generalize this?
> 
> Like the case you are trying to fix is essentially:
> 1 - click on 1
> 2 - 2 shows up as a result of hovering over 1
> 3 - click on 2
> (3) fails before 1 gets unactive and dismisses the hover, making 2 disappear.
> 
> What if I just want to click (tap) on a element that responds to hover? Once I finish, would it remain as active till I click elsewhere?

I think this will reproduce the case you are asking about (please correct me if I am mistaken):

1. Go to www.rbyers.net/eventTest.html
2. Tap on the purple square
// Notice that the square shows :hover :active while your finger is pressed down on the square, but only :hover once your finger is lifted off of the square (which is consistent with the behavior expected from a mouse)
3. Tap elsewhere on the page
// The :hover state is cleared from the purple square, which is consistent with the behavior expected from a mouse

[Antonio Gomes]

(In reply to comment #15)
> (In reply to comment #14)
> ..
> I think this will reproduce the case you are asking about (please correct me if I am mistaken):
> 
> 1. Go to www.rbyers.net/eventTest.html
> 2. Tap on the purple square
> // Notice that the square shows :hover :active while your finger is pressed down on the square, but only :hover once your finger is lifted off of the square (which is consistent with the behavior expected from a mouse)
> 3. Tap elsewhere on the page
> // The :hover state is cleared from the purple square, which is consistent with the behavior expected from a mouse

Right. Is this behavior with your patch, right? 


Gen/Eli: Do you have an opinion?

[Terry Anderson]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > ..
> > I think this will reproduce the case you are asking about (please correct me if I am mistaken):
> > 
> > 1. Go to www.rbyers.net/eventTest.html
> > 2. Tap on the purple square
> > // Notice that the square shows :hover :active while your finger is pressed down on the square, but only :hover once your finger is lifted off of the square (which is consistent with the behavior expected from a mouse)
> > 3. Tap elsewhere on the page
> > // The :hover state is cleared from the purple square, which is consistent with the behavior expected from a mouse
> 
> Right. Is this behavior with your patch, right? 
> 

Yes, this behavior is with my patch.

> 
> Gen/Eli: Do you have an opinion?

This looks consistent with the blackberry port.

[WebKit Review Bot]

Comment on attachment 193388 [details]
comments from rbyers addressed

Clearing flags on attachment: 193388

Committed r146224: <http://trac.webkit.org/changeset/146224>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Fixed the expected results added in http://trac.webkit.org/changeset/146298. Please be cautious in the future.

[Terry Anderson]

(In reply to comment #21)
> Fixed the expected results added in http://trac.webkit.org/changeset/146298. Please be cautious in the future.

Oops, sorry about that :\ Thanks a lot for fixing these up for me.