Bug 100113 – Can not click the node which is shown only if it is hovered when touch events are enabled.

• Status: RESOLVED
• Resolution: WONTFIX
• Component: UI Events

[Eunmi Lee]

When touch events are enabled, we can not click the node which is shown only if it is hovered
because the hover property is removed from the node when touch is released. (it was patched by Bug 77620)
The click event comes after touch is released, so it could not reach to the node.
For example, we can not click the "more menu" in the left-top of espn.go.com, because the list is hided when touch is released.

I think the aim of Bug 77620 is that clearing the hover property from the node when the finger is released off the node.
So, it is better to not remove hover property if the hovered node and touch-released node are same.

[Eunmi Lee]

Created attachment 170141 [details]
Patch

[Allan Sandfeld Jensen]

Comment on attachment 170141 [details]
Patch

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

The patch makes some sense. Though I guess there would be cases where it ends up looking weird, it is better than having stuff that is not accesible.

> Source/WebCore/dom/Document.cpp:5885
> +            size_t removeCount = nodesToRemoveFromChain.size();
> +            for (size_t i = 0; i < removeCount; ++i)
> +                nodesToRemoveFromChain[i]->setHovered(false);
> +            setHoverNode(0);
> +
> +            // A touch release can not set new hover or active target.
> +            return;

I guess you can also just set newHoverNode to 0 here. Which would make the code simpler.

[Eunmi Lee]

Comment on attachment 170141 [details]
Patch

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

>> Source/WebCore/dom/Document.cpp:5885
>> +            return;
> 
> I guess you can also just set newHoverNode to 0 here. Which would make the code simpler.

Yes, right. I will change like that :)

[Eunmi Lee]

Created attachment 170149 [details]
Patch

[Allan Sandfeld Jensen]

Comment on attachment 170149 [details]
Patch

Looks good. Would it be possible to write a test for this using the eventSender?

[Alexey Proskuryakov]

Comment on attachment 170149 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        No new tests, no change in behavior.

How can this be accurate? The title describes a bug, fixing it is a change in behavior.

[Eunmi Lee]

(In reply to comment #5)
> (From update of attachment 170149 [details])
> Looks good. Would it be possible to write a test for this using the eventSender?

OK, I'll try to write a test for this :)

[Eunmi Lee]

(In reply to comment #6)
> (From update of attachment 170149 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170149&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        No new tests, no change in behavior.
> 
> How can this be accurate? The title describes a bug, fixing it is a change in behavior.

Yes, I will modify that after writing a test.

[Eunmi Lee]

Created attachment 201033 [details]
Patch

Add a test and rebase.

[Eunmi Lee]

Created attachment 201053 [details]
Patch

Modify test case.

[Allan Sandfeld Jensen]

Is the click event triggered by touch in this case? Or how are the touch events affecting mouse click events?

Perhaps the correct solution in the long run is to stop touch events from affecting hover/active, and let gestures do it.

[Eunmi Lee]

(In reply to comment #11)
> Is the click event triggered by touch in this case? Or how are the touch events affecting mouse click events?
> 
> Perhaps the correct solution in the long run is to stop touch events from affecting hover/active, and let gestures do it.

I see what is the point.

I thought the case that the GestureTap event is occurred after touch start and release event.
But in that case, the shouldGesturesTriggerActive() should be true and touch events should not affect to the hover/active state. right?
so, there is no problem for click and I have to modify the patch.

Then, thinking about the case there is no gesture events and there exists only touch events.
In that case, we can not show the hover menu using touch start event because touch release event clears the hover state.
Is that ok that we can not set hover only with touch events?

[Allan Sandfeld Jensen]

(In reply to comment #12)
> (In reply to comment #11)
> > Is the click event triggered by touch in this case? Or how are the touch events affecting mouse click events?
> > 
> > Perhaps the correct solution in the long run is to stop touch events from affecting hover/active, and let gestures do it.
> 
> I see what is the point.
> 
> I thought the case that the GestureTap event is occurred after touch start and release event.
> But in that case, the shouldGesturesTriggerActive() should be true and touch events should not affect to the hover/active state. right?
> so, there is no problem for click and I have to modify the patch.
> 
> Then, thinking about the case there is no gesture events and there exists only touch events.
> In that case, we can not show the hover menu using touch start event because touch release event clears the hover state.
> Is that ok that we can not set hover only with touch events?

Depends on platforms that are not using gesture events. The only reason shouldGesturesTriggerActive() is currently false is because WebKit2 (Qt/EFL) does not send the GestureTapDown event. If we did gesture handling can set the hover/active much better since it would match the element that touch-adjustment adjusts the touch to.

[Eunmi Lee]

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > Is the click event triggered by touch in this case? Or how are the touch events affecting mouse click events?
> > > 
> > > Perhaps the correct solution in the long run is to stop touch events from affecting hover/active, and let gestures do it.
> > 
> > I see what is the point.
> > 
> > I thought the case that the GestureTap event is occurred after touch start and release event.
> > But in that case, the shouldGesturesTriggerActive() should be true and touch events should not affect to the hover/active state. right?
> > so, there is no problem for click and I have to modify the patch.
> > 
> > Then, thinking about the case there is no gesture events and there exists only touch events.
> > In that case, we can not show the hover menu using touch start event because touch release event clears the hover state.
> > Is that ok that we can not set hover only with touch events?
> 
> Depends on platforms that are not using gesture events. The only reason shouldGesturesTriggerActive() is currently false is because WebKit2 (Qt/EFL) does not send the GestureTapDown event. If we did gesture handling can set the hover/active much better since it would match the element that touch-adjustment adjusts the touch to.

Yes, then it is better to use GestureTapDown event later and we don't have to care about the case only has touch events now.
I think this patch is not necessary anymore.
Would you change the status of this patch? I don't have permission so I can see only 'new, assigned, resolved' options.

[Allan Sandfeld Jensen]

I think you have the rights. You just need to pick 'resolved' and the resolution to close bugs.

[Allan Sandfeld Jensen]

Comment on attachment 201053 [details]
Patch

Clearing flags to take it out of review queue.