Bug 103283 - Hover effects from a GestureTapDown are dismissed by a GestureTap on the hover element
Summary: Hover effects from a GestureTapDown are dismissed by a GestureTap on the hove...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Terry Anderson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-26 13:30 PST by Terry Anderson
Modified: 2013-03-20 10:33 PDT (History)
8 users (show)

See Also:


Attachments
Patch (6.08 KB, patch)
2012-11-26 13:56 PST, Terry Anderson
no flags Details | Formatted Diff | Diff
WIP, not for review (5.95 KB, patch)
2013-03-05 11:40 PST, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch (11.39 KB, patch)
2013-03-05 18:16 PST, Terry Anderson
no flags Details | Formatted Diff | Diff
Patch (13.01 KB, patch)
2013-03-14 14:05 PDT, Terry Anderson
no flags Details | Formatted Diff | Diff
comments from rbyers addressed (13.48 KB, patch)
2013-03-15 15:47 PDT, Terry Anderson
no flags Details | Formatted Diff | Diff
comments from rbyers addressed (13.45 KB, patch)
2013-03-15 15:53 PDT, Terry Anderson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Terry Anderson 2012-11-26 13:30:25 PST
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.
Comment 1 Terry Anderson 2012-11-26 13:56:44 PST
Created attachment 176058 [details]
Patch
Comment 2 Terry Anderson 2012-11-26 14:00:08 PST
(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.
Comment 3 Terry Anderson 2013-03-05 11:40:37 PST
Created attachment 191523 [details]
WIP, not for review
Comment 4 Terry Anderson 2013-03-05 18:16:04 PST
Created attachment 191628 [details]
Patch
Comment 5 Antonio Gomes 2013-03-05 22:33:49 PST
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'
Comment 6 Antonio Gomes 2013-03-05 22:44:58 PST
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?
Comment 7 Terry Anderson 2013-03-06 12:52:21 PST
(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.
Comment 8 Terry Anderson 2013-03-14 14:05:42 PDT
Created attachment 193181 [details]
Patch
Comment 9 Terry Anderson 2013-03-14 14:09:17 PDT
(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.
Comment 10 Rick Byers 2013-03-14 18:37:30 PDT
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).
Comment 11 Terry Anderson 2013-03-15 15:47:46 PDT
Created attachment 193386 [details]
comments from rbyers addressed
Comment 12 Terry Anderson 2013-03-15 15:53:34 PDT
Created attachment 193388 [details]
comments from rbyers addressed
Comment 13 Rick Byers 2013-03-15 19:35:35 PDT
Looks good to me.  Antonio?
Comment 14 Antonio Gomes 2013-03-16 12:17:30 PDT
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?
Comment 15 Terry Anderson 2013-03-18 09:06:42 PDT
(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
Comment 16 Antonio Gomes 2013-03-19 10:17:31 PDT
(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?
Comment 17 Terry Anderson 2013-03-19 10:20:47 PDT
(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?
Comment 18 gmak 2013-03-19 10:59:38 PDT
This looks consistent with the blackberry port.
Comment 19 WebKit Review Bot 2013-03-19 11:22:57 PDT
Comment on attachment 193388 [details]
comments from rbyers addressed

Clearing flags on attachment: 193388

Committed r146224: <http://trac.webkit.org/changeset/146224>
Comment 20 WebKit Review Bot 2013-03-19 11:23:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Ryosuke Niwa 2013-03-19 21:05:04 PDT
Fixed the expected results added in http://trac.webkit.org/changeset/146298. Please be cautious in the future.
Comment 22 Terry Anderson 2013-03-20 10:33:41 PDT
(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.