Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2521993003:
Send boundary events before gotpointercapture (Closed)
|
Created:
4 years ago by Navid Zolghadr Modified:
4 years ago CC:
chromium-reviews, shans, rjwright, blink-reviews-animation_chromium.org, Navid Zolghadr, dtapuska+blinkwatch_chromium.org, darktears, blink-reviews, Eric Willigers Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSend boundary events before gotpointercapture
This CL sends boundary events before gotpointercapture
as if gotpointercapture can trigger boundary events
like a move event. This kind of makes sure that
there will be no boundary events between got and lost
pointer capture events which might be more intuitive.
At this time Edge behavior differs from both the behavior in
this CL and before this cL as they were sending boundary
events regardless of the capture and using the hit-tested
target. However, they are planning to catch up here.
BUG=633679
Committed: https://crrev.com/eff55a14abadd957d1ebf8cfd897ce6c6892b3c3
Cr-Commit-Position: refs/heads/master@{#439609}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Removing the old redundant hack #Patch Set 3 : Fix the tests #Messages
Total messages: 30 (17 generated)
The CQ bit was checked by nzolghadr@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...
nzolghadr@chromium.org changed reviewers: + dtapuska@chromium.org, mustaq@chromium.org
LGTM. Please add a CL description that Edge currently differs here but has agreed to catch up.
Description was changed from ========== Send boundary events befere gotpointercapture This CL sends boundary events before gotpointercapture as if gotpointercapture can trigger boundary events like a move event. This kind of makes sure that there will be no boundary events between got and lost pointer capture events which might be more intuitive. BUG=633679 ========== to ========== Send boundary events befere gotpointercapture This CL sends boundary events before gotpointercapture as if gotpointercapture can trigger boundary events like a move event. This kind of makes sure that there will be no boundary events between got and lost pointer capture events which might be more intuitive. At this time Edge behavior differs from both the behavior in this CL and before this cL as they were sending boundary events regardless of the capture and using the hit-tested target. However, they are planning to catch up here. BUG=633679 ==========
Description was changed from ========== Send boundary events befere gotpointercapture This CL sends boundary events before gotpointercapture as if gotpointercapture can trigger boundary events like a move event. This kind of makes sure that there will be no boundary events between got and lost pointer capture events which might be more intuitive. At this time Edge behavior differs from both the behavior in this CL and before this cL as they were sending boundary events regardless of the capture and using the hit-tested target. However, they are planning to catch up here. BUG=633679 ========== to ========== Send boundary events befere gotpointercapture This CL sends boundary events before gotpointercapture as if gotpointercapture can trigger boundary events like a move event. This kind of makes sure that there will be no boundary events between got and lost pointer capture events which might be more intuitive. At this time Edge behavior differs from both the behavior in this CL and before this cL as they were sending boundary events regardless of the capture and using the hit-tested target. However, they are planning to catch up here. BUG=633679 ==========
Still LGTM but add these comments if applicable: https://codereview.chromium.org/2521993003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2521993003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:561: if (!RuntimeEnabledFeatures::pointerEventV1SpecCapturingEnabled()) { Wait...one last thing: - With pointerEventV1SpecCapturingEnabled(), we may see redundant enter/leave events. I don't care, we will remove the flag soon. But add a comment if this is the case. - This block and the setNodeUnderPointer() call below now /effective/ for only lostpointercapture, right? Please comment here to clarify. - Any chance we can/should move this to processPendingPointerCapture()?
Description was changed from ========== Send boundary events befere gotpointercapture This CL sends boundary events before gotpointercapture as if gotpointercapture can trigger boundary events like a move event. This kind of makes sure that there will be no boundary events between got and lost pointer capture events which might be more intuitive. At this time Edge behavior differs from both the behavior in this CL and before this cL as they were sending boundary events regardless of the capture and using the hit-tested target. However, they are planning to catch up here. BUG=633679 ========== to ========== Send boundary events before gotpointercapture This CL sends boundary events before gotpointercapture as if gotpointercapture can trigger boundary events like a move event. This kind of makes sure that there will be no boundary events between got and lost pointer capture events which might be more intuitive. At this time Edge behavior differs from both the behavior in this CL and before this cL as they were sending boundary events regardless of the capture and using the hit-tested target. However, they are planning to catch up here. BUG=633679 ==========
https://codereview.chromium.org/2521993003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2521993003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:561: if (!RuntimeEnabledFeatures::pointerEventV1SpecCapturingEnabled()) { On 2016/11/22 21:46:36, mustaq wrote: > Wait...one last thing: > > - With pointerEventV1SpecCapturingEnabled(), we may see redundant enter/leave > events. I don't care, we will remove the flag soon. But add a comment if this is > the case. > > - This block and the setNodeUnderPointer() call below now /effective/ for only > lostpointercapture, right? Please comment here to clarify. > > - Any chance we can/should move this to processPendingPointerCapture()? > - I noticed this problem before actually. I could have passed an extra parameter to avoid it. But I was not sure how important that is. Which one do you prefer? 1. Pass the extra parameter to fix the problem when the pointerEventV1SpecCapturingEnabled is on 2. Add the comment that a problem exists. 3. Just remove that flag by tomorrow if that is not necessary anymore. - That is correct. Sure. I'll add a comment - Which block? So you processPendingPointerCapture should somehow return a hitTestTarget or something?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2521993003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2521993003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:561: if (!RuntimeEnabledFeatures::pointerEventV1SpecCapturingEnabled()) { On 2016/11/22 21:58:11, Navid Zolghadr wrote: > On 2016/11/22 21:46:36, mustaq wrote: > > Wait...one last thing: > > > > - With pointerEventV1SpecCapturingEnabled(), we may see redundant enter/leave > > events. I don't care, we will remove the flag soon. But add a comment if this > is > > the case. > > > > - This block and the setNodeUnderPointer() call below now /effective/ for only > > lostpointercapture, right? Please comment here to clarify. > > > > - Any chance we can/should move this to processPendingPointerCapture()? > > > > - I noticed this problem before actually. I could have passed an extra parameter > to avoid it. But I was not sure how important that is. Which one do you prefer? > > 1. Pass the extra parameter to fix the problem when the > pointerEventV1SpecCapturingEnabled is on > 2. Add the comment that a problem exists. > 3. Just remove that flag by tomorrow if that is not necessary anymore. > I vote for #3. > - That is correct. Sure. I'll add a comment > > - Which block? So you processPendingPointerCapture should somehow return a > hitTestTarget or something? > I was thinking about the |if| plus the call to setNodeUnderPointer(). It seems a bit odd & error-prone that only a part of lostpointercapture is handled outside processPendingPointerCapture(). Does it look better if processPendingPointerCapture takes an optional hittestTarget, and returns the "effective" target?
https://codereview.chromium.org/2521993003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2521993003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/PointerEventManager.cpp:561: if (!RuntimeEnabledFeatures::pointerEventV1SpecCapturingEnabled()) { On 2016/11/23 15:38:47, mustaq wrote: > On 2016/11/22 21:58:11, Navid Zolghadr wrote: > > On 2016/11/22 21:46:36, mustaq wrote: > > > Wait...one last thing: > > > > > > - With pointerEventV1SpecCapturingEnabled(), we may see redundant > enter/leave > > > events. I don't care, we will remove the flag soon. But add a comment if > this > > is > > > the case. > > > > > > - This block and the setNodeUnderPointer() call below now /effective/ for > only > > > lostpointercapture, right? Please comment here to clarify. > > > > > > - Any chance we can/should move this to processPendingPointerCapture()? > > > > > > > - I noticed this problem before actually. I could have passed an extra > parameter > > to avoid it. But I was not sure how important that is. Which one do you > prefer? > > > > 1. Pass the extra parameter to fix the problem when the > > pointerEventV1SpecCapturingEnabled is on > > 2. Add the comment that a problem exists. > > 3. Just remove that flag by tomorrow if that is not necessary anymore. > > > > I vote for #3. Alright. I work on this before this lands. This change has to wait for web platform tests to update anyway. > > > - That is correct. Sure. I'll add a comment > > > > - Which block? So you processPendingPointerCapture should somehow return a > > hitTestTarget or something? > > > > I was thinking about the |if| plus the call to setNodeUnderPointer(). It seems a > bit odd & error-prone that only a part of lostpointercapture is handled outside > processPendingPointerCapture(). Does it look better if > processPendingPointerCapture takes an optional hittestTarget, and returns the > "effective" target? I thought a bit more about it and realized my previous answer was not very accurate. The setNodeUnderPointer outside of processPendingPointerCapture will only work when there is no capture including when this very event causes the lostpointercapture and it will have no effect when this very event sends a gotpointercapture. So if there is no capturing at all (no got or lost) this setNodeUnderPointer here is the only place that causes the boundary events. Regarding the if block not every place that processPendingPointerCapture is used that is needed. The only place that is needed to have the hit-test target overrided is processCaptureAndPositionOfPointerEvent that as the name suggests is a wrapper around processPendingPointerCapture that also deals with the target and position of the pointer. That is why I put that if block here in the first place.
The CQ bit was checked by nzolghadr@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...
ptal. I had to remove one of the old hacks which we added and after refactoring it was redundant. I noticed it had introduced a bug which is shown in the original versions of the tests.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
On 2016/12/07 19:29:42, Navid Zolghadr wrote: > ptal. > I had to remove one of the old hacks which we added and after refactoring it was > redundant. I noticed it had introduced a bug which is shown in the original > versions of the tests. Good catch. Still LGTM.
nzolghadr@chromium.org changed reviewers: + bokan@chromium.org
bokan@chromium.org: Please review changes in third_party/WebKit/Source/Core/input/*
rs lgtm
The CQ bit was checked by nzolghadr@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": 40001, "attempt_start_ts": 1482181314748900,
"parent_rev": "05b82a2f97b951d75a0c13d00d8ed31182a81933", "commit_rev":
"e546bb4d950697a60f43e2076b921f225984c603"}
Message was sent while issue was closed.
Description was changed from ========== Send boundary events before gotpointercapture This CL sends boundary events before gotpointercapture as if gotpointercapture can trigger boundary events like a move event. This kind of makes sure that there will be no boundary events between got and lost pointer capture events which might be more intuitive. At this time Edge behavior differs from both the behavior in this CL and before this cL as they were sending boundary events regardless of the capture and using the hit-tested target. However, they are planning to catch up here. BUG=633679 ========== to ========== Send boundary events before gotpointercapture This CL sends boundary events before gotpointercapture as if gotpointercapture can trigger boundary events like a move event. This kind of makes sure that there will be no boundary events between got and lost pointer capture events which might be more intuitive. At this time Edge behavior differs from both the behavior in this CL and before this cL as they were sending boundary events regardless of the capture and using the hit-tested target. However, they are planning to catch up here. BUG=633679 Review-Url: https://codereview.chromium.org/2521993003 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Send boundary events before gotpointercapture This CL sends boundary events before gotpointercapture as if gotpointercapture can trigger boundary events like a move event. This kind of makes sure that there will be no boundary events between got and lost pointer capture events which might be more intuitive. At this time Edge behavior differs from both the behavior in this CL and before this cL as they were sending boundary events regardless of the capture and using the hit-tested target. However, they are planning to catch up here. BUG=633679 Review-Url: https://codereview.chromium.org/2521993003 ========== to ========== Send boundary events before gotpointercapture This CL sends boundary events before gotpointercapture as if gotpointercapture can trigger boundary events like a move event. This kind of makes sure that there will be no boundary events between got and lost pointer capture events which might be more intuitive. At this time Edge behavior differs from both the behavior in this CL and before this cL as they were sending boundary events regardless of the capture and using the hit-tested target. However, they are planning to catch up here. BUG=633679 Committed: https://crrev.com/eff55a14abadd957d1ebf8cfd897ce6c6892b3c3 Cr-Commit-Position: refs/heads/master@{#439609} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/eff55a14abadd957d1ebf8cfd897ce6c6892b3c3 Cr-Commit-Position: refs/heads/master@{#439609} |
