Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2701553002:
Touchmoves should not be suppressed if the touchstart is consumed (Closed)
DescriptionTouchmoves should not be suppressed if the touchstart is consumed
In a touch sequence starting with a touchstart, if the touchstart is consumed
(prevent-defaulted), we will not suppress the following touchmoves even if
they are in the slop regions.
BUG=692702
Review-Url: https://codereview.chromium.org/2701553002
Cr-Commit-Position: refs/heads/master@{#451796}
Committed: https://chromium.googlesource.com/chromium/src/+/be77e3f3b19b056f8ef9c5464ebadcc2d5bf25ac
Patch Set 1 #
Total comments: 5
Patch Set 2 : slop region #
Total comments: 4
Patch Set 3 : slop region #
Total comments: 2
Patch Set 4 : remove comments #
Messages
Total messages: 47 (31 generated)
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 ========== slop region slop region slop region BUG= ========== to ========== Touchmoves should not be suppressed if the touchstart is consumed In a touch sequence starting with a touchstart, if the touchstart or any previous touch moves are consumed, we will not suppress the touch moves even if they are in the slop regions. BUG=692702 ==========
lanwei@chromium.org changed reviewers: + dtapuska@chromium.org, mustaq@chromium.org
Description was changed from ========== Touchmoves should not be suppressed if the touchstart is consumed In a touch sequence starting with a touchstart, if the touchstart or any previous touch moves are consumed, we will not suppress the touch moves even if they are in the slop regions. BUG=692702 ========== to ========== Touchmoves should not be suppressed if the touchstart is consumed In a touch sequence starting with a touchstart, if the touchstart or any previous touch moves are consumed, we will not suppress the following touch moves even if they are in the slop regions. BUG=692702 ==========
On 2017/02/15 22:02:15, lanwei wrote: lgtm
lanwei@chromium.org changed reviewers: + rbyers@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM % a clarification around touchstart to prevent future misunderstanding. https://codereview.chromium.org/2701553002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/pointerevents/pointer-event-consumed-touchstart-in-slop-region.html (right): https://codereview.chromium.org/2701553002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/pointerevents/pointer-event-consumed-touchstart-in-slop-region.html:56: { name: "pointerMove", x: x, y: y + 10 }, Please add a single pixel drag to make sure that even smallest possible movements are not hidden. Perhaps we should do the same in multi-pointer-event-in-slop-region.html too: have a single pixel move after a touchstart for every touchstart. This didn't came to me before, sorry. https://codereview.chromium.org/2701553002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2701553002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/TouchEventManager.cpp:296: eventResult != WebInputEventResult::NotHandled) { This "!=NotHandled " applies only to touchstarts because in suppressed-state, touchmoves can't be prevent-defaulted. Please add a comment here that this applies only to touchstarts. Also update the CL description accordingly.
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 ========== Touchmoves should not be suppressed if the touchstart is consumed In a touch sequence starting with a touchstart, if the touchstart or any previous touch moves are consumed, we will not suppress the following touch moves even if they are in the slop regions. BUG=692702 ========== to ========== Touchmoves should not be suppressed if the touchstart is consumed In a touch sequence starting with a touchstart, if the touchstart is consumed(prevent-defaulted), we will not suppress the following touch moves even if they are in the slop regions. BUG=692702 ==========
Description was changed from ========== Touchmoves should not be suppressed if the touchstart is consumed In a touch sequence starting with a touchstart, if the touchstart is consumed(prevent-defaulted), we will not suppress the following touch moves even if they are in the slop regions. BUG=692702 ========== to ========== Touchmoves should not be suppressed if the touchstart is consumed In a touch sequence starting with a touchstart, if the touchstart is consumed(prevent-defaulted), we will not suppress the following touch moves even if they are in the slop regions. BUG=692702 ==========
Rick, could you please take a look, thank you? https://codereview.chromium.org/2701553002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/pointerevents/pointer-event-consumed-touchstart-in-slop-region.html (right): https://codereview.chromium.org/2701553002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/pointerevents/pointer-event-consumed-touchstart-in-slop-region.html:56: { name: "pointerMove", x: x, y: y + 10 }, On 2017/02/16 17:06:26, mustaq wrote: > Please add a single pixel drag to make sure that even smallest possible > movements are not hidden. > > Perhaps we should do the same in multi-pointer-event-in-slop-region.html too: > have a single pixel move after a touchstart for every touchstart. This didn't > came to me before, sorry. Done. https://codereview.chromium.org/2701553002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2701553002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/TouchEventManager.cpp:296: eventResult != WebInputEventResult::NotHandled) { On 2017/02/16 17:06:26, mustaq wrote: > This "!=NotHandled " applies only to touchstarts because in suppressed-state, > touchmoves can't be prevent-defaulted. Please add a comment here that this > applies only to touchstarts. > > Also update the CL description accordingly. Done.
https://codereview.chromium.org/2701553002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2701553002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/TouchEventManager.cpp:296: eventResult != WebInputEventResult::NotHandled) { On 2017/02/16 19:48:59, lanwei wrote: > On 2017/02/16 17:06:26, mustaq wrote: > > This "!=NotHandled " applies only to touchstarts because in suppressed-state, > > touchmoves can't be prevent-defaulted. Please add a comment here that this > > applies only to touchstarts. > > > > Also update the CL description accordingly. > > Done. Let me clarify a bit: this condition is met only for touchstarts, so we don't need a new condition. Let's just add an ASSERT(event.type() == WebInputEvent::TouchStart) w/o any comments.
Description was changed from ========== Touchmoves should not be suppressed if the touchstart is consumed In a touch sequence starting with a touchstart, if the touchstart is consumed(prevent-defaulted), we will not suppress the following touch moves even if they are in the slop regions. BUG=692702 ========== to ========== Touchmoves should not be suppressed if the touchstart is consumed In a touch sequence starting with a touchstart, if the touchstart is consumed (prevent-defaulted), we will not suppress the following touchmoves even if they are in the slop regions. BUG=692702 ==========
https://codereview.chromium.org/2701553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2701553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:295: // Do not suppress any touchmoves if the touchstart is consumed. nit: it seems like it would be a small clean-up to move the "m_suppressingTouchmovesWithinSlop = true;" line down from 131 into here. I.e. instead of setting it and then clearing it, we could just ONLY set it after a touchstart has been dispatched that wasn't cancelled.
https://codereview.chromium.org/2701553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2701553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:295: // Do not suppress any touchmoves if the touchstart is consumed. On 2017/02/16 21:21:57, Rick Byers wrote: > nit: it seems like it would be a small clean-up to move the > "m_suppressingTouchmovesWithinSlop = true;" line down from 131 into here. I.e. > instead of setting it and then clearing it, we could just ONLY set it after a > touchstart has been dispatched that wasn't cancelled. The "true" value is used in Line 138, so we have to leave things up there anyways I think.
https://codereview.chromium.org/2701553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2701553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:295: // Do not suppress any touchmoves if the touchstart is consumed. On 2017/02/16 21:41:26, mustaq wrote: > On 2017/02/16 21:21:57, Rick Byers wrote: > > nit: it seems like it would be a small clean-up to move the > > "m_suppressingTouchmovesWithinSlop = true;" line down from 131 into here. > I.e. > > instead of setting it and then clearing it, we could just ONLY set it after a > > touchstart has been dispatched that wasn't cancelled. > > The "true" value is used in Line 138, so we have to leave things up there > anyways I think. But it's used only if we're currently processing a TouchMove, and here we're talking about setting it only after a TouchStart. I.e. "am I suppressing slop moves" is relevant between the time AFTER a the first touchstart has been dispatched up until just BEFORE the last touchend/cancel has been dispatched. This bit doesn't really make sense DURING the dispatch of the first touchstart - we don't actually know yet if we should be suppressing slop movement or not (and it's irrelevant until we get the first move anyway).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
On 2017/02/16 22:09:19, Rick Byers wrote: > https://codereview.chromium.org/2701553002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): > > https://codereview.chromium.org/2701553002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/input/TouchEventManager.cpp:295: // Do not > suppress any touchmoves if the touchstart is consumed. > On 2017/02/16 21:41:26, mustaq wrote: > > On 2017/02/16 21:21:57, Rick Byers wrote: > > > nit: it seems like it would be a small clean-up to move the > > > "m_suppressingTouchmovesWithinSlop = true;" line down from 131 into here. > > I.e. > > > instead of setting it and then clearing it, we could just ONLY set it after > a > > > touchstart has been dispatched that wasn't cancelled. > > > > The "true" value is used in Line 138, so we have to leave things up there > > anyways I think. > > But it's used only if we're currently processing a TouchMove, and here we're > talking about setting it only after a TouchStart. I.e. "am I suppressing slop > moves" is relevant between the time AFTER a the first touchstart has been > dispatched up until just BEFORE the last touchend/cancel has been dispatched. > This bit doesn't really make sense DURING the dispatch of the first touchstart - > we don't actually know yet if we should be suppressing slop movement or not (and > it's irrelevant until we get the first move anyway). Yes you are right. And it would be much cleaner.
Rick, could you please take another look, thank you? https://codereview.chromium.org/2701553002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2701553002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:295: // Do not suppress any touchmoves if the touchstart is consumed. On 2017/02/16 22:09:19, Rick Byers wrote: > On 2017/02/16 21:41:26, mustaq wrote: > > On 2017/02/16 21:21:57, Rick Byers wrote: > > > nit: it seems like it would be a small clean-up to move the > > > "m_suppressingTouchmovesWithinSlop = true;" line down from 131 into here. > > I.e. > > > instead of setting it and then clearing it, we could just ONLY set it after > a > > > touchstart has been dispatched that wasn't cancelled. > > > > The "true" value is used in Line 138, so we have to leave things up there > > anyways I think. > > But it's used only if we're currently processing a TouchMove, and here we're > talking about setting it only after a TouchStart. I.e. "am I suppressing slop > moves" is relevant between the time AFTER a the first touchstart has been > dispatched up until just BEFORE the last touchend/cancel has been dispatched. > This bit doesn't really make sense DURING the dispatch of the first touchstart - > we don't actually know yet if we should be suppressing slop movement or not (and > it's irrelevant until we get the first move anyway). Acknowledged. Yes, we can merge line 131 here, thank you.
One more nit, still LGTM. https://codereview.chromium.org/2701553002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2701553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:129: // Suppress the touch moves in the slop region when Lines 129-130 are meaningless now. Please delete.
LGTM
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_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
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.
The CQ bit was checked by lanwei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtapuska@chromium.org, mustaq@chromium.org, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/2701553002/#ps60001 (title: "remove comments")
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": 60001, "attempt_start_ts": 1487697956370440,
"parent_rev": "7390e5594fb6f8468359b519580dacb29fdf269b", "commit_rev":
"be77e3f3b19b056f8ef9c5464ebadcc2d5bf25ac"}
Message was sent while issue was closed.
Description was changed from ========== Touchmoves should not be suppressed if the touchstart is consumed In a touch sequence starting with a touchstart, if the touchstart is consumed (prevent-defaulted), we will not suppress the following touchmoves even if they are in the slop regions. BUG=692702 ========== to ========== Touchmoves should not be suppressed if the touchstart is consumed In a touch sequence starting with a touchstart, if the touchstart is consumed (prevent-defaulted), we will not suppress the following touchmoves even if they are in the slop regions. BUG=692702 Review-Url: https://codereview.chromium.org/2701553002 Cr-Commit-Position: refs/heads/master@{#451796} Committed: https://chromium.googlesource.com/chromium/src/+/be77e3f3b19b056f8ef9c5464eba... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/be77e3f3b19b056f8ef9c5464eba...
Message was sent while issue was closed.
https://codereview.chromium.org/2701553002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/TouchEventManager.cpp (right): https://codereview.chromium.org/2701553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/TouchEventManager.cpp:129: // Suppress the touch moves in the slop region when On 2017/02/17 15:24:01, mustaq wrote: > Lines 129-130 are meaningless now. Please delete. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
