Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1210253003:
Fix wheel event dispatch logic to fallback to document when hit-test fails (Closed)
|
Created:
5 years, 6 months ago by majidvp Modified:
5 years, 5 months ago Reviewers:
Rick Byers CC:
blink-reviews, bokan Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionFix wheel event dispatch logic to fallback to document when hit-test fails
Hit testing may fail to identify any element in certain layouts.
In these cases, we should dispatch the wheel event to the document.
TEST=fast/events/dispatch-mouse-events-to-window-always.html
BUG=500144
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=198210
Patch Set 1 #
Total comments: 13
Patch Set 2 : Improve and fix the test #Patch Set 3 : Update comment with link to the bug #Patch Set 4 : Replace with passing Active to be consistent #
Total comments: 8
Patch Set 5 : Address nits #
Messages
Total messages: 14 (2 generated)
majidvp@chromium.org changed reviewers: + rbyers@chromium.org
https://codereview.chromium.org/1210253003/diff/1/LayoutTests/fast/events/dis... File LayoutTests/fast/events/dispatch-mouse-events-to-window-always-expected.txt (right): https://codereview.chromium.org/1210253003/diff/1/LayoutTests/fast/events/dis... LayoutTests/fast/events/dispatch-mouse-events-to-window-always-expected.txt:9: FAIL dispatchCount[document.body][eventType] should be 2. Was 0. You've got FAILs here https://codereview.chromium.org/1210253003/diff/1/LayoutTests/fast/events/dis... File LayoutTests/fast/events/dispatch-mouse-events-to-window-always.html (right): https://codereview.chromium.org/1210253003/diff/1/LayoutTests/fast/events/dis... LayoutTests/fast/events/dispatch-mouse-events-to-window-always.html:21: <body> nit: omit body https://codereview.chromium.org/1210253003/diff/1/LayoutTests/fast/events/dis... LayoutTests/fast/events/dispatch-mouse-events-to-window-always.html:59: shouldBe('dispatchCount[window][eventType]', '4'); This seems a little indirect - eg. hard to debug when it fails. Would be better to have each call to generateEvents be a separate test case that validates the expected events are received. Then when it fails you know exactly what was missed. https://codereview.chromium.org/1210253003/diff/1/Source/core/input/EventHand... File Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1210253003/diff/1/Source/core/input/EventHand... Source/core/input/EventHandler.cpp:1767: node = m_frame->document(); How did you decide to fall back to 'document' vs. 'window'? Ideally this wouldn't be different for different types of events. Eg. what do touch events do? Any idea how other browsers behave? I assume this isn't spec'd, right?
https://codereview.chromium.org/1210253003/diff/1/LayoutTests/fast/events/dis... File LayoutTests/fast/events/dispatch-mouse-events-to-window-always-expected.txt (right): https://codereview.chromium.org/1210253003/diff/1/LayoutTests/fast/events/dis... LayoutTests/fast/events/dispatch-mouse-events-to-window-always-expected.txt:9: FAIL dispatchCount[document.body][eventType] should be 2. Was 0. On 2015/06/26 12:34:48, Rick Byers wrote: > You've got FAILs here Acknowledged. https://codereview.chromium.org/1210253003/diff/1/LayoutTests/fast/events/dis... File LayoutTests/fast/events/dispatch-mouse-events-to-window-always.html (right): https://codereview.chromium.org/1210253003/diff/1/LayoutTests/fast/events/dis... LayoutTests/fast/events/dispatch-mouse-events-to-window-always.html:21: <body> On 2015/06/26 12:34:49, Rick Byers wrote: > nit: omit body I added body element specifically to test that the event is dispatched to document but not body. https://codereview.chromium.org/1210253003/diff/1/LayoutTests/fast/events/dis... LayoutTests/fast/events/dispatch-mouse-events-to-window-always.html:59: shouldBe('dispatchCount[window][eventType]', '4'); On 2015/06/26 12:34:49, Rick Byers wrote: > This seems a little indirect - eg. hard to debug when it fails. Would be better > to have each call to generateEvents be a separate test case that validates the > expected events are received. Then when it fails you know exactly what was > missed. Acknowledged. https://codereview.chromium.org/1210253003/diff/1/Source/core/input/EventHand... File Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1210253003/diff/1/Source/core/input/EventHand... Source/core/input/EventHandler.cpp:1767: node = m_frame->document(); On 2015/06/26 12:34:49, Rick Byers wrote: > How did you decide to fall back to 'document' vs. 'window'? Ideally this > wouldn't be different for different types of events. Eg. what do touch events > do? Any idea how other browsers behave? I assume this isn't spec'd, right? Hit test falls back to document for clicks and touch events. The code for that lives in |DeprecatedPaintLayer::hitTest| as a special case that is triggered by specific hit testing flags (active|release). I didn't think it is a good idea to add a new hit-testing flag for wheel event. Per our discussion offline it maybe a good idea to move all of that special event specific fallback logic outside DeprecatedPaintLayer and into EventHandler. +dtapuska who may also be interested in this. FF, and IE both behaves by falling back to document but Safari has the same bug as Chromium.
https://codereview.chromium.org/1210253003/diff/1/LayoutTests/fast/events/dis... File LayoutTests/fast/events/dispatch-mouse-events-to-window-always.html (right): https://codereview.chromium.org/1210253003/diff/1/LayoutTests/fast/events/dis... LayoutTests/fast/events/dispatch-mouse-events-to-window-always.html:21: <body> On 2015/06/29 16:58:40, majidvp wrote: > On 2015/06/26 12:34:49, Rick Byers wrote: > > nit: omit body > > I added body element specifically to test that the event > is dispatched to document but not body. Duh, of course - sorry. https://codereview.chromium.org/1210253003/diff/1/Source/core/input/EventHand... File Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1210253003/diff/1/Source/core/input/EventHand... Source/core/input/EventHandler.cpp:1767: node = m_frame->document(); On 2015/06/29 16:58:40, majidvp wrote: > On 2015/06/26 12:34:49, Rick Byers wrote: > > How did you decide to fall back to 'document' vs. 'window'? Ideally this > > wouldn't be different for different types of events. Eg. what do touch events > > do? Any idea how other browsers behave? I assume this isn't spec'd, right? > > Hit test falls back to document for clicks and touch events. The code for that > lives in |DeprecatedPaintLayer::hitTest| as a special case that is triggered by > specific hit testing flags (active|release). I didn't think it is a good idea to > add a new hit-testing flag for wheel event. Per our discussion offline it maybe > a good idea to move all of that special event specific fallback logic outside > DeprecatedPaintLayer and into EventHandler. +dtapuska who may also be interested > in this. > > FF, and IE both behaves by falling back to document but Safari has the same bug > as Chromium. Great! That code in DeprecatedPaintLayer has seemed sketchy to me for awhile. I think this is great evidence that we should be trying to move it out (hoist it to the EventHandler layer above hit testing). Then we could at least share the logic between mouse, touch and wheel events here. Falling back to dispatching on the document sounds reasonable to me in all those cases. If, for some reason, it's too difficult to hoist this special case out, then I'm also fine with a wheel-specific hack like this. But maybe add a TODO (here and in the DeprecatedPaintLayer code) and file a bug to track the cleanup we'd ultimately like to do.
On 2015/06/29 17:07:59, Rick Byers wrote: > https://codereview.chromium.org/1210253003/diff/1/LayoutTests/fast/events/dis... > File LayoutTests/fast/events/dispatch-mouse-events-to-window-always.html > (right): > > https://codereview.chromium.org/1210253003/diff/1/LayoutTests/fast/events/dis... > LayoutTests/fast/events/dispatch-mouse-events-to-window-always.html:21: <body> > On 2015/06/29 16:58:40, majidvp wrote: > > On 2015/06/26 12:34:49, Rick Byers wrote: > > > nit: omit body > > > > I added body element specifically to test that the event > > is dispatched to document but not body. > > Duh, of course - sorry. > > https://codereview.chromium.org/1210253003/diff/1/Source/core/input/EventHand... > File Source/core/input/EventHandler.cpp (right): > > https://codereview.chromium.org/1210253003/diff/1/Source/core/input/EventHand... > Source/core/input/EventHandler.cpp:1767: node = m_frame->document(); > On 2015/06/29 16:58:40, majidvp wrote: > > On 2015/06/26 12:34:49, Rick Byers wrote: > > > How did you decide to fall back to 'document' vs. 'window'? Ideally this > > > wouldn't be different for different types of events. Eg. what do touch > events > > > do? Any idea how other browsers behave? I assume this isn't spec'd, right? > > > > Hit test falls back to document for clicks and touch events. The code for that > > lives in |DeprecatedPaintLayer::hitTest| as a special case that is triggered > by > > specific hit testing flags (active|release). I didn't think it is a good idea > to > > add a new hit-testing flag for wheel event. Per our discussion offline it > maybe > > a good idea to move all of that special event specific fallback logic outside > > DeprecatedPaintLayer and into EventHandler. +dtapuska who may also be > interested > > in this. > > > > FF, and IE both behaves by falling back to document but Safari has the same > bug > > as Chromium. > > Great! That code in DeprecatedPaintLayer has seemed sketchy to me for awhile. > I think this is great evidence that we should be trying to move it out (hoist it > to the EventHandler layer above hit testing). Then we could at least share the > logic between mouse, touch and wheel events here. Falling back to dispatching > on the document sounds reasonable to me in all those cases. > > If, for some reason, it's too difficult to hoist this special case out, then I'm > also fine with a wheel-specific hack like this. But maybe add a TODO (here and > in the DeprecatedPaintLayer code) and file a bug to track the cleanup we'd > ultimately like to do. There are code paths for hit testing that go directly into the LayoutView and don't go via the EventHandler.cpp
On 2015/06/29 17:51:52, Dave Tapuska wrote: > On 2015/06/29 17:07:59, Rick Byers wrote: > > > https://codereview.chromium.org/1210253003/diff/1/LayoutTests/fast/events/dis... > > File LayoutTests/fast/events/dispatch-mouse-events-to-window-always.html > > (right): > > > > > https://codereview.chromium.org/1210253003/diff/1/LayoutTests/fast/events/dis... > > LayoutTests/fast/events/dispatch-mouse-events-to-window-always.html:21: <body> > > On 2015/06/29 16:58:40, majidvp wrote: > > > On 2015/06/26 12:34:49, Rick Byers wrote: > > > > nit: omit body > > > > > > I added body element specifically to test that the event > > > is dispatched to document but not body. > > > > Duh, of course - sorry. > > > > > https://codereview.chromium.org/1210253003/diff/1/Source/core/input/EventHand... > > File Source/core/input/EventHandler.cpp (right): > > > > > https://codereview.chromium.org/1210253003/diff/1/Source/core/input/EventHand... > > Source/core/input/EventHandler.cpp:1767: node = m_frame->document(); > > On 2015/06/29 16:58:40, majidvp wrote: > > > On 2015/06/26 12:34:49, Rick Byers wrote: > > > > How did you decide to fall back to 'document' vs. 'window'? Ideally this > > > > wouldn't be different for different types of events. Eg. what do touch > > events > > > > do? Any idea how other browsers behave? I assume this isn't spec'd, > right? > > > > > > Hit test falls back to document for clicks and touch events. The code for > that > > > lives in |DeprecatedPaintLayer::hitTest| as a special case that is triggered > > by > > > specific hit testing flags (active|release). I didn't think it is a good > idea > > to > > > add a new hit-testing flag for wheel event. Per our discussion offline it > > maybe > > > a good idea to move all of that special event specific fallback logic > outside > > > DeprecatedPaintLayer and into EventHandler. +dtapuska who may also be > > interested > > > in this. > > > > > > FF, and IE both behaves by falling back to document but Safari has the same > > bug > > > as Chromium. > > > > Great! That code in DeprecatedPaintLayer has seemed sketchy to me for awhile. > > > I think this is great evidence that we should be trying to move it out (hoist > it > > to the EventHandler layer above hit testing). Then we could at least share > the > > logic between mouse, touch and wheel events here. Falling back to dispatching > > on the document sounds reasonable to me in all those cases. > > > > If, for some reason, it's too difficult to hoist this special case out, then > I'm > > also fine with a wheel-specific hack like this. But maybe add a TODO (here > and > > in the DeprecatedPaintLayer code) and file a bug to track the cleanup we'd > > ultimately like to do. > > There are code paths for hit testing that go directly into the LayoutView and > don't go via the EventHandler.cpp Right. There are other cases where I think various flags are being abused to trigger this fallback logic. For example |TreeScope::elementsFromPoint| is sets Active flag even though it does not make sense the or |WebDevToolsAgentImpl::inspectElementAt| is using the Move flag!! Perhaps we need a separate type for fall-back enum {none, document, document-if-in-frame} that is resolved in LayoutView and can be set by various hit-test clients such as EventHandler, or TreeScope. I updated the patch to do the simple fix here, perhaps Dave wants to take over the flag separation and moving the logic..?
PTAL. https://codereview.chromium.org/1210253003/diff/1/LayoutTests/fast/events/dis... File LayoutTests/fast/events/dispatch-mouse-events-to-window-always-expected.txt (right): https://codereview.chromium.org/1210253003/diff/1/LayoutTests/fast/events/dis... LayoutTests/fast/events/dispatch-mouse-events-to-window-always-expected.txt:9: FAIL dispatchCount[document.body][eventType] should be 2. Was 0. On 2015/06/29 16:58:40, majidvp wrote: > On 2015/06/26 12:34:48, Rick Byers wrote: > > You've got FAILs here > > Acknowledged. Done. https://codereview.chromium.org/1210253003/diff/1/LayoutTests/fast/events/dis... File LayoutTests/fast/events/dispatch-mouse-events-to-window-always.html (right): https://codereview.chromium.org/1210253003/diff/1/LayoutTests/fast/events/dis... LayoutTests/fast/events/dispatch-mouse-events-to-window-always.html:59: shouldBe('dispatchCount[window][eventType]', '4'); On 2015/06/29 16:58:40, majidvp wrote: > On 2015/06/26 12:34:49, Rick Byers wrote: > > This seems a little indirect - eg. hard to debug when it fails. Would be > better > > to have each call to generateEvents be a separate test case that validates the > > expected events are received. Then when it fails you know exactly what was > > missed. > > Acknowledged. Done. https://codereview.chromium.org/1210253003/diff/1/Source/core/input/EventHand... File Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1210253003/diff/1/Source/core/input/EventHand... Source/core/input/EventHandler.cpp:1767: node = m_frame->document(); On 2015/06/29 17:07:59, Rick Byers wrote: > On 2015/06/29 16:58:40, majidvp wrote: > > On 2015/06/26 12:34:49, Rick Byers wrote: > > > How did you decide to fall back to 'document' vs. 'window'? Ideally this > > > wouldn't be different for different types of events. Eg. what do touch > events > > > do? Any idea how other browsers behave? I assume this isn't spec'd, right? > > > > Hit test falls back to document for clicks and touch events. The code for that > > lives in |DeprecatedPaintLayer::hitTest| as a special case that is triggered > by > > specific hit testing flags (active|release). I didn't think it is a good idea > to > > add a new hit-testing flag for wheel event. Per our discussion offline it > maybe > > a good idea to move all of that special event specific fallback logic outside > > DeprecatedPaintLayer and into EventHandler. +dtapuska who may also be > interested > > in this. > > > > FF, and IE both behaves by falling back to document but Safari has the same > bug > > as Chromium. > > Great! That code in DeprecatedPaintLayer has seemed sketchy to me for awhile. > I think this is great evidence that we should be trying to move it out (hoist it > to the EventHandler layer above hit testing). Then we could at least share the > logic between mouse, touch and wheel events here. Falling back to dispatching > on the document sounds reasonable to me in all those cases. > > If, for some reason, it's too difficult to hoist this special case out, then I'm > also fine with a wheel-specific hack like this. But maybe add a TODO (here and > in the DeprecatedPaintLayer code) and file a bug to track the cleanup we'd > ultimately like to do. Created a bug (crbug.com/505825 owned by dtapuska@) to track that work and added TODO comments referring to it. I think we should land this temporary wheel fix while we wait for that to be done.
Thanks - re-using the existing hack like this definitely seems better to me than adding another copy of it. LGTM with nits. https://codereview.chromium.org/1210253003/diff/60001/LayoutTests/fast/events... File LayoutTests/fast/events/dispatch-mouse-events-to-window-always.html (right): https://codereview.chromium.org/1210253003/diff/60001/LayoutTests/fast/events... LayoutTests/fast/events/dispatch-mouse-events-to-window-always.html:92: shouldBe('actualReceivers', 'expectedReceivers'); nit: can you remove the quotes from expectedReceivers (and so also the window.expectedReceivers global) so that the test output actually contains the names of the elements receiving events? That would make it a little easier to verify correctness from the test output. https://codereview.chromium.org/1210253003/diff/60001/Source/core/input/Event... File Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1210253003/diff/60001/Source/core/input/Event... Source/core/input/EventHandler.cpp:1730: HitTestRequest request(HitTestRequest::ReadOnly | HitTestRequest::Active); I assume you've done a search to verify that this won't trigger other intended side effects inside hit testing somewhere, right? HitTestRequest::Move would be less of a lie if that part of the hack would work just as well for you (Active really means a mouse button is down). https://codereview.chromium.org/1210253003/diff/60001/Source/core/paint/Depre... File Source/core/paint/DeprecatedPaintLayer.cpp (right): https://codereview.chromium.org/1210253003/diff/60001/Source/core/paint/Depre... Source/core/paint/DeprecatedPaintLayer.cpp:1606: // TODO(majidvp): This should apply more consistently across different event types and we should not use nit: follow the wrapping of the rest of the comment (eg. about 100 characters max). No need for the extra space at the start of the next line.
https://codereview.chromium.org/1210253003/diff/60001/LayoutTests/fast/events... File LayoutTests/fast/events/dispatch-mouse-events-to-window-always.html (right): https://codereview.chromium.org/1210253003/diff/60001/LayoutTests/fast/events... LayoutTests/fast/events/dispatch-mouse-events-to-window-always.html:92: shouldBe('actualReceivers', 'expectedReceivers'); On 2015/06/30 20:14:49, Rick Byers wrote: > nit: can you remove the quotes from expectedReceivers (and so also the > window.expectedReceivers global) so that the test output actually contains the > names of the elements receiving events? That would make it a little easier to > verify correctness from the test output. ShouldBe does not like non-string arguments. I used a slightly different solution with the same end result. https://codereview.chromium.org/1210253003/diff/60001/Source/core/input/Event... File Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1210253003/diff/60001/Source/core/input/Event... Source/core/input/EventHandler.cpp:1730: HitTestRequest request(HitTestRequest::ReadOnly | HitTestRequest::Active); On 2015/06/30 20:14:49, Rick Byers wrote: > I assume you've done a search to verify that this won't trigger other intended > side effects inside hit testing somewhere, right? > > HitTestRequest::Move would be less of a lie if that part of the hack would work > just as well for you (Active really means a mouse button is down). Yes I have. It shouldn't have any other side effects when used in combination with ReadOnly. The only other effect of Active is to update hover and active state but that does not kicked in when ReadOnly is present. This is why "ReadOnly | Active" is used for this in few other places without causing side-effects. Move may also work but additionally it requires the point to be in the frame visible area rect. This should be fine for any user input generated events but may be problematic for script generated events. I have a test case specifically for mouse events that are outside visible frame rect which fails when Move is used. https://codereview.chromium.org/1210253003/diff/60001/Source/core/paint/Depre... File Source/core/paint/DeprecatedPaintLayer.cpp (right): https://codereview.chromium.org/1210253003/diff/60001/Source/core/paint/Depre... Source/core/paint/DeprecatedPaintLayer.cpp:1606: // TODO(majidvp): This should apply more consistently across different event types and we should not use On 2015/06/30 20:14:49, Rick Byers wrote: > nit: follow the wrapping of the rest of the comment (eg. about 100 characters > max). No need for the extra space at the start of the next line. Done.
Still LGTM https://codereview.chromium.org/1210253003/diff/60001/LayoutTests/fast/events... File LayoutTests/fast/events/dispatch-mouse-events-to-window-always.html (right): https://codereview.chromium.org/1210253003/diff/60001/LayoutTests/fast/events... LayoutTests/fast/events/dispatch-mouse-events-to-window-always.html:92: shouldBe('actualReceivers', 'expectedReceivers'); On 2015/06/30 22:48:31, majidvp wrote: > On 2015/06/30 20:14:49, Rick Byers wrote: > > nit: can you remove the quotes from expectedReceivers (and so also the > > window.expectedReceivers global) so that the test output actually contains the > > names of the elements receiving events? That would make it a little easier to > > verify correctness from the test output. > > ShouldBe does not like non-string arguments. I used a slightly different > solution > with the same end result. Oh right. Sure this looks fine (since you're only looking at one node of each type). https://codereview.chromium.org/1210253003/diff/60001/Source/core/input/Event... File Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1210253003/diff/60001/Source/core/input/Event... Source/core/input/EventHandler.cpp:1730: HitTestRequest request(HitTestRequest::ReadOnly | HitTestRequest::Active); On 2015/06/30 22:48:32, majidvp wrote: > On 2015/06/30 20:14:49, Rick Byers wrote: > > I assume you've done a search to verify that this won't trigger other intended > > > side effects inside hit testing somewhere, right? > > > > HitTestRequest::Move would be less of a lie if that part of the hack would > work > > just as well for you (Active really means a mouse button is down). > > Yes I have. It shouldn't have any other side effects when used in combination > with ReadOnly. > The only other effect of Active is to update hover and active state but that > does not kicked in when ReadOnly is present. This is why "ReadOnly | Active" is > used for this in few other places without causing side-effects. > > Move may also work but additionally it requires the point to be in the frame > visible area rect. This should be fine for any user input generated events but > may be problematic for script generated events. I have a test case > specifically for mouse events that are outside visible frame rect which fails > when Move is used. Alright, good enough for a temporary hack. Thanks!
The CQ bit was checked by majidvp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1210253003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=198210 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
