WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED WONTFIX
100113
Can not click the node which is shown only if it is hovered when touch events are enabled.
https://bugs.webkit.org/show_bug.cgi?id=100113
Summary
Can not click the node which is shown only if it is hovered when touch events...
Eunmi Lee
Reported
2012-10-23 05:56:08 PDT
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.
Attachments
Patch
(2.94 KB, patch)
2012-10-23 06:12 PDT
,
Eunmi Lee
no flags
Details
Formatted Diff
Diff
Patch
(2.77 KB, patch)
2012-10-23 06:47 PDT
,
Eunmi Lee
no flags
Details
Formatted Diff
Diff
Patch
(7.13 KB, patch)
2013-05-08 00:14 PDT
,
Eunmi Lee
no flags
Details
Formatted Diff
Diff
Patch
(7.30 KB, patch)
2013-05-08 03:56 PDT
,
Eunmi Lee
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Eunmi Lee
Comment 1
2012-10-23 06:12:59 PDT
Created
attachment 170141
[details]
Patch
Allan Sandfeld Jensen
Comment 2
2012-10-23 06:22:24 PDT
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 3
2012-10-23 06:32:02 PDT
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
Comment 4
2012-10-23 06:47:54 PDT
Created
attachment 170149
[details]
Patch
Allan Sandfeld Jensen
Comment 5
2012-10-23 07:42:56 PDT
Comment on
attachment 170149
[details]
Patch Looks good. Would it be possible to write a test for this using the eventSender?
Alexey Proskuryakov
Comment 6
2012-10-23 13:43:08 PDT
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
Comment 7
2012-10-23 17:52:26 PDT
(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
Comment 8
2012-10-23 17:52:58 PDT
(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
Comment 9
2013-05-08 00:14:57 PDT
Created
attachment 201033
[details]
Patch Add a test and rebase.
Eunmi Lee
Comment 10
2013-05-08 03:56:35 PDT
Created
attachment 201053
[details]
Patch Modify test case.
Allan Sandfeld Jensen
Comment 11
2013-05-08 04:37:04 PDT
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
Comment 12
2013-05-08 05:28:00 PDT
(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
Comment 13
2013-05-08 05:53:44 PDT
(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
Comment 14
2013-05-08 20:03:07 PDT
(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
Comment 15
2013-05-09 05:02:33 PDT
I think you have the rights. You just need to pick 'resolved' and the resolution to close bugs.
Allan Sandfeld Jensen
Comment 16
2013-05-09 05:02:57 PDT
Comment on
attachment 201053
[details]
Patch Clearing flags to take it out of review queue.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug