Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2759523002:
Make sure all the touch events during an active fling are uncancelable (Closed)
DescriptionMake sure all the touch events during an active fling are uncancelable
After we enabled the fling intervention, all the touch events during an active fling should be uncancelable,
but now they are still cancelable. The reason is the layer returned by FindLayerThatIsHitByPointInTouchHandlerRegion
does not match with the current scrolling layer even we scroll on the same layer. So, we use FindLayerThatIsHitByPoint
to return the layer the touch start hits on and compare with the current scrolling layer.
BUG=700465
Review-Url: https://codereview.chromium.org/2759523002
Cr-Commit-Position: refs/heads/master@{#460415}
Committed: https://chromium.googlesource.com/chromium/src/+/9327ce925b77f722c31116f5e08cff23df70908d
Patch Set 1 : fling layer #
Total comments: 3
Patch Set 2 : fling layer #
Total comments: 9
Patch Set 3 : Check with active layer tree #Patch Set 4 : rebase master #
Messages
Total messages: 77 (56 generated)
Description was changed from ========== fling layer BUG= ========== to ========== fling layer BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== fling layer BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Make sure all the touch events during an active fling are uncancelable After we enabled the fling intervention, all the touch events during an active fling should be uncancelable, but now they are still cancelable. The reason is the layer returned by FindLayerThatIsHitByPointInTouchHandlerRegion does not match with the current scrolling layer even we scroll on the same layer. So, we use Check the layer the touch start hits on with the current scrolling layer BUG= ==========
Description was changed from ========== Make sure all the touch events during an active fling are uncancelable After we enabled the fling intervention, all the touch events during an active fling should be uncancelable, but now they are still cancelable. The reason is the layer returned by FindLayerThatIsHitByPointInTouchHandlerRegion does not match with the current scrolling layer even we scroll on the same layer. So, we use Check the layer the touch start hits on with the current scrolling layer BUG= ========== to ========== Make sure all the touch events during an active fling are uncancelable After we enabled the fling intervention, all the touch events during an active fling should be uncancelable, but now they are still cancelable. The reason is the layer returned by FindLayerThatIsHitByPointInTouchHandlerRegion does not match with the current scrolling layer even we scroll on the same layer. So, we use FindLayerThatIsHitByPoint to return the layer the touch start hits on and compare with the current scrolling layer. BUG=700465 ==========
lanwei@chromium.org changed reviewers: + dtapuska@chromium.org
Patchset #1 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dtapuska@chromium.org changed reviewers: + tdresser@chromium.org
Adding tdresser@ I've looked at the code and I don't see any issues. From what I understand is that we are hitting the layer with the touch event handler is higher up in the tree yet we are scrolling something else and so we check that the layer we hit right now is a descendant of the current scrolling layer. Seems reasonable to me.
mustaq@chromium.org changed reviewers: + mustaq@chromium.org
I think this behavior is now testable through a PointerAction script, right?
Yeah, general premise SGTM. This does need a test though. Ideally a unit test, potentially also a layout test. https://codereview.chromium.org/2759523002/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2759523002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:606: ScrollTree& scroll_tree = active_tree_->property_trees()->scroll_tree; I don't understand this change. In what context is this different? https://codereview.chromium.org/2759523002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:633: // descendant. Add a comment describing why we can't use the layer_impl_touch_handler.
Description was changed from ========== Make sure all the touch events during an active fling are uncancelable After we enabled the fling intervention, all the touch events during an active fling should be uncancelable, but now they are still cancelable. The reason is the layer returned by FindLayerThatIsHitByPointInTouchHandlerRegion does not match with the current scrolling layer even we scroll on the same layer. So, we use FindLayerThatIsHitByPoint to return the layer the touch start hits on and compare with the current scrolling layer. BUG=700465 ========== to ========== Make sure all the touch events during an active fling are uncancelable After we enabled the fling intervention, all the touch events during an active fling should be uncancelable, but now they are still cancelable. The reason is the layer returned by FindLayerThatIsHitByPointInTouchHandlerRegion does not match with the current scrolling layer even we scroll on the same layer. So, we use FindLayerThatIsHitByPoint to return the layer the touch start hits on and compare with the current scrolling layer. BUG=700465 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Make sure all the touch events during an active fling are uncancelable After we enabled the fling intervention, all the touch events during an active fling should be uncancelable, but now they are still cancelable. The reason is the layer returned by FindLayerThatIsHitByPointInTouchHandlerRegion does not match with the current scrolling layer even we scroll on the same layer. So, we use FindLayerThatIsHitByPoint to return the layer the touch start hits on and compare with the current scrolling layer. BUG=700465 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Make sure all the touch events during an active fling are uncancelable After we enabled the fling intervention, all the touch events during an active fling should be uncancelable, but now they are still cancelable. The reason is the layer returned by FindLayerThatIsHitByPointInTouchHandlerRegion does not match with the current scrolling layer even we scroll on the same layer. So, we use FindLayerThatIsHitByPoint to return the layer the touch start hits on and compare with the current scrolling layer. BUG=700465 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
lanwei@chromium.org changed reviewers: + pdr@chromium.org
pdr@ could you please take a look at the changes and unittests, thank you? https://codereview.chromium.org/2759523002/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2759523002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:606: ScrollTree& scroll_tree = active_tree_->property_trees()->scroll_tree; On 2017/03/20 20:15:45, tdresser wrote: > I don't understand this change. In what context is this different? It is the same. I just feel it is better to use active_tree.
lanwei@chromium.org changed reviewers: + pdr@google.com, weiliangc@chromium.org - pdr@chromium.org
LGTM
https://codereview.chromium.org/2759523002/diff/80001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2759523002/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:606: ScrollTree& scroll_tree = active_tree_->property_trees()->scroll_tree; Could you DCHECK that child is on active tree? as in child->layer_tree_impl() == active_tree_ https://codereview.chromium.org/2759523002/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:637: active_tree_->FindLayerThatIsHitByPoint(device_viewport_point); Consider the case where there are two divs, where the parent one has touch handler, and its child div is inside parent div and has a scrolling layer inside. The layer structure would look like: - parent div layer with touch handler - child div layer - child div content layer that is currently scrolling This would make the parent div touch handler uncancellable. Is this expected behavior? https://codereview.chromium.org/2759523002/diff/80001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl_unittest.cc (right): https://codereview.chromium.org/2759523002/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl_unittest.cc:10228: std::unique_ptr<LayerImpl> child = Could pdr@ confirm that this is what the "layer tree" would look like? As in we would have one layer with event handler, and another layer for scrolling.
pdr@chromium.org changed reviewers: + pdr@chromium.org
https://codereview.chromium.org/2759523002/diff/80001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2759523002/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:634: // is created inside it, which is the actual scrolling layer, and it does not If we have the following: <div style="border: 1px solid black; width: 400px; height: 400px; overflow: scroll; background: white; will-change: transform;"> content<div id=forcescroll style="height: 1000px;"></div> </div> I think this example will create two layers: one for the outer div, and one for the content. Is that correct? I don't think any blank layers are created. https://codereview.chromium.org/2759523002/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:637: active_tree_->FindLayerThatIsHitByPoint(device_viewport_point); On 2017/03/22 at 18:11:31, weiliangc wrote: > Consider the case where there are two divs, where the parent one has touch handler, and its child div is inside parent div and has a scrolling layer inside. > The layer structure would look like: > - parent div layer with touch handler > - child div layer > - child div content layer that is currently scrolling > > This would make the parent div touch handler uncancellable. Is this expected behavior? +1 to Wei's question I am also curious: this logic looks similar to LayerTreeHostImpl::ScrollBegin which is also called by input_handler_proxy. Are these codes doing similar work? Are these checks needed for other hit test cases?
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Description was changed from ========== Make sure all the touch events during an active fling are uncancelable After we enabled the fling intervention, all the touch events during an active fling should be uncancelable, but now they are still cancelable. The reason is the layer returned by FindLayerThatIsHitByPointInTouchHandlerRegion does not match with the current scrolling layer even we scroll on the same layer. So, we use FindLayerThatIsHitByPoint to return the layer the touch start hits on and compare with the current scrolling layer. BUG=700465 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Make sure all the touch events during an active fling are uncancelable After we enabled the fling intervention, all the touch events during an active fling should be uncancelable, but now they are still cancelable. The reason is the layer returned by FindLayerThatIsHitByPointInTouchHandlerRegion does not match with the current scrolling layer even we scroll on the same layer. So, we use FindLayerThatIsHitByPoint to return the layer the touch start hits on and compare with the current scrolling layer. BUG=700465 ==========
https://codereview.chromium.org/2759523002/diff/80001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/2759523002/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:606: ScrollTree& scroll_tree = active_tree_->property_trees()->scroll_tree; On 2017/03/22 18:11:31, weiliangc wrote: > Could you DCHECK that child is on active tree? as in child->layer_tree_impl() == > active_tree_ Done. https://codereview.chromium.org/2759523002/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:634: // is created inside it, which is the actual scrolling layer, and it does not On 2017/03/22 20:30:11, pdr. wrote: > If we have the following: > <div style="border: 1px solid black; width: 400px; height: 400px; overflow: > scroll; background: white; will-change: transform;"> > content<div id=forcescroll style="height: 1000px;"></div> > </div> > > I think this example will create two layers: one for the outer div, and one for > the content. Is that correct? I don't think any blank layers are created. I changed the comments, did not mention the blank layer, should cover all cases. https://codereview.chromium.org/2759523002/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:637: active_tree_->FindLayerThatIsHitByPoint(device_viewport_point); On 2017/03/22 18:11:31, weiliangc wrote: > Consider the case where there are two divs, where the parent one has touch > handler, and its child div is inside parent div and has a scrolling layer > inside. > The layer structure would look like: > - parent div layer with touch handler > - child div layer > - child div content layer that is currently scrolling > > This would make the parent div touch handler uncancellable. Is this expected > behavior? Good question. No matter we scroll on the parent div layer or the child div layer, we do not make the parent div touch handler uncancellable. We are actually making the touch events uncancellable, so that they are not blocked by the event handlers in the parent layer. We want the touch starts are uncancellable, if the layer the touch starts hit on is the same as the scroll layer or its descendant. https://codereview.chromium.org/2759523002/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_impl.cc:637: active_tree_->FindLayerThatIsHitByPoint(device_viewport_point); On 2017/03/22 20:30:11, pdr. wrote: > On 2017/03/22 at 18:11:31, weiliangc wrote: > > Consider the case where there are two divs, where the parent one has touch > handler, and its child div is inside parent div and has a scrolling layer > inside. > > The layer structure would look like: > > - parent div layer with touch handler > > - child div layer > > - child div content layer that is currently scrolling > > > > This would make the parent div touch handler uncancellable. Is this expected > behavior? > > +1 to Wei's question > > I am also curious: this logic looks similar to LayerTreeHostImpl::ScrollBegin > which is also called by input_handler_proxy. Are these codes doing similar work? > Are these checks needed for other hit test cases? This change is part of our touch event intervention, we make touch events unblock from the event handlers by forcing them uncancellable when there is an active fling. We are checking the layers when we are handling touch starts to make sure the touches hit on a scrolling layer or its descendant, so we can make them uncancellable. That is why we only check it in this class. Maybe the code is similar than ScrollBegin, but they are doing very different things.
pdr@chromium.org changed reviewers: + ajuma@chromium.org
LGTM, I like the latest patch but I don't feel like I know enough about cc's hit testing code to give this the final LGTM. Wei or Ali, would you be up for taking a quick look at this too?
LGTM
lanwei@chromium.org changed reviewers: - pdr@google.com
The CQ bit was checked by lanwei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2759523002/#ps100001 (title: "Check with active layer tree")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from ========== Make sure all the touch events during an active fling are uncancelable After we enabled the fling intervention, all the touch events during an active fling should be uncancelable, but now they are still cancelable. The reason is the layer returned by FindLayerThatIsHitByPointInTouchHandlerRegion does not match with the current scrolling layer even we scroll on the same layer. So, we use FindLayerThatIsHitByPoint to return the layer the touch start hits on and compare with the current scrolling layer. BUG=700465 ========== to ========== Make sure all the touch events during an active fling are uncancelable After we enabled the fling intervention, all the touch events during an active fling should be uncancelable, but now they are still cancelable. The reason is the layer returned by FindLayerThatIsHitByPointInTouchHandlerRegion does not match with the current scrolling layer even we scroll on the same layer. So, we use FindLayerThatIsHitByPoint to return the layer the touch start hits on and compare with the current scrolling layer. BUG=700465 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by lanwei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from weiliangc@chromium.org, pdr@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2759523002/#ps120001 (title: "rebase master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lanwei@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
CQ has no permission to schedule in bucket master.tryserver.chromium.linux
Description was changed from ========== Make sure all the touch events during an active fling are uncancelable After we enabled the fling intervention, all the touch events during an active fling should be uncancelable, but now they are still cancelable. The reason is the layer returned by FindLayerThatIsHitByPointInTouchHandlerRegion does not match with the current scrolling layer even we scroll on the same layer. So, we use FindLayerThatIsHitByPoint to return the layer the touch start hits on and compare with the current scrolling layer. BUG=700465 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Make sure all the touch events during an active fling are uncancelable After we enabled the fling intervention, all the touch events during an active fling should be uncancelable, but now they are still cancelable. The reason is the layer returned by FindLayerThatIsHitByPointInTouchHandlerRegion does not match with the current scrolling layer even we scroll on the same layer. So, we use FindLayerThatIsHitByPoint to return the layer the touch start hits on and compare with the current scrolling layer. BUG=700465 ==========
The CQ bit was checked by lanwei@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
CQ has no permission to schedule in bucket master.tryserver.chromium.linux
The CQ bit was checked by lanwei@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: CQ has no permission to schedule in bucket master.tryserver.chromium.linux
The CQ bit was checked by lanwei@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490802253828200,
"parent_rev": "f996646c9b3b4c8027e693c5a48c7fab3adbfe8c", "commit_rev":
"9327ce925b77f722c31116f5e08cff23df70908d"}
Message was sent while issue was closed.
Description was changed from ========== Make sure all the touch events during an active fling are uncancelable After we enabled the fling intervention, all the touch events during an active fling should be uncancelable, but now they are still cancelable. The reason is the layer returned by FindLayerThatIsHitByPointInTouchHandlerRegion does not match with the current scrolling layer even we scroll on the same layer. So, we use FindLayerThatIsHitByPoint to return the layer the touch start hits on and compare with the current scrolling layer. BUG=700465 ========== to ========== Make sure all the touch events during an active fling are uncancelable After we enabled the fling intervention, all the touch events during an active fling should be uncancelable, but now they are still cancelable. The reason is the layer returned by FindLayerThatIsHitByPointInTouchHandlerRegion does not match with the current scrolling layer even we scroll on the same layer. So, we use FindLayerThatIsHitByPoint to return the layer the touch start hits on and compare with the current scrolling layer. BUG=700465 Review-Url: https://codereview.chromium.org/2759523002 Cr-Commit-Position: refs/heads/master@{#460415} Committed: https://chromium.googlesource.com/chromium/src/+/9327ce925b77f722c31116f5e08c... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as https://chromium.googlesource.com/chromium/src/+/9327ce925b77f722c31116f5e08c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
