Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2831933002:
Make sure PointerEvents's isPrimary set correctly for stylus (Closed)
DescriptionMake sure PointerEvents's isPrimary set correctly for stylus
When a pen enters and leaves a tablet's proximity and enters again, the
PointerEvents' isPrimary should always be true.
Testing page: https://patrickhlauke.github.io/touch/tracker/multi-touch-tracker-pointer-hud.html
Webplatform test pointerevent_pointerleave_pen-manual.html does not work
right now because of no support of PointerLeave events when pen leaves
the range of the tablet in PointerActionSequence API. I will add it soon.
BUG=713745
Review-Url: https://codereview.chromium.org/2831933002
Cr-Commit-Position: refs/heads/master@{#467997}
Committed: https://chromium.googlesource.com/chromium/src/+/c007949541f4d2238c9653a4e3b8dbc0987bc752
Patch Set 1 : isprimary #
Total comments: 4
Patch Set 2 : primaray #Patch Set 3 : isprimary #
Total comments: 4
Patch Set 4 : isprimary #
Messages
Total messages: 63 (53 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 ========== isprimary BUG= ========== to ========== Make sure PointerEvents's isPrimary set correctly for stylus When a pen enters and leaves a tablet's proximity and enters again, the PointerEvents' isPrimary is always true. BUG=713745 ==========
lanwei@chromium.org changed reviewers: + dtapuska@chromium.org, nzolghadr@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_tsan_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 to run a CQ dry run
Patchset #1 (id:1) has been deleted
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...
Patchset #1 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2831933002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerevent_pointerleave_pen-manual.html (right): https://codereview.chromium.org/2831933002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerevent_pointerleave_pen-manual.html:33: assert_equals(event.isPrimary, true, "The " + event.type + ".isPrimary is true"); You'll need to fix the automation scripts. https://codereview.chromium.org/2831933002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2831933002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:163: if (event_type == EventTypeNames::pointerleave) Do you want to do this after the pointerleave is dispatched? So that if anything queries states that it is correct while the leave is being dispatched?
https://codereview.chromium.org/2831933002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerevent_pointerleave_pen-manual.html (right): https://codereview.chromium.org/2831933002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerevent_pointerleave_pen-manual.html:50: When a pointing device that supports hover (pen stylus) leaves the range of the digitizer while over an element, the pointerleave event must be dispatched. Can you also update the test description? https://codereview.chromium.org/2831933002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2831933002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:163: if (event_type == EventTypeNames::pointerleave) On 2017/04/20 21:04:14, dtapuska wrote: > Do you want to do this after the pointerleave is dispatched? So that if anything > queries states that it is correct while the leave is being dispatched? I don't think doing this here is correct. This would remove the pointer regardless of the type of the event. So if we send a pointerleave to an element while mouse/pen/tuoch is leaving boundaries of an element inside the page they will get removed from our data structures.
Sorry, this patch is still under implementation, not ready for review. I should not add both of you as the reviewers at this time.
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_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: 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 PointerEvents's isPrimary set correctly for stylus When a pen enters and leaves a tablet's proximity and enters again, the PointerEvents' isPrimary is always true. BUG=713745 ========== to ========== Make sure PointerEvents's isPrimary set correctly for stylus When a pen enters and leaves a tablet's proximity and enters again, the PointerEvents' isPrimary is always true. BUG=713745 ==========
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 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...
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Description was changed from ========== Make sure PointerEvents's isPrimary set correctly for stylus When a pen enters and leaves a tablet's proximity and enters again, the PointerEvents' isPrimary is always true. BUG=713745 ========== to ========== Make sure PointerEvents's isPrimary set correctly for stylus When a pen enters and leaves a tablet's proximity and enters again, the PointerEvents' isPrimary is always true. Testing page: https://patrickhlauke.github.io/touch/tracker/multi-touch-tracker-pointer-hud... BUG=713745 ==========
Description was changed from ========== Make sure PointerEvents's isPrimary set correctly for stylus When a pen enters and leaves a tablet's proximity and enters again, the PointerEvents' isPrimary is always true. Testing page: https://patrickhlauke.github.io/touch/tracker/multi-touch-tracker-pointer-hud... BUG=713745 ========== to ========== Make sure PointerEvents's isPrimary set correctly for stylus When a pen enters and leaves a tablet's proximity and enters again, the PointerEvents' isPrimary should always be true. Testing page: https://patrickhlauke.github.io/touch/tracker/multi-touch-tracker-pointer-hud... pointerevents/pointerevent_pointerleave_pen-manual.html does not work right because of missing support of PointerLeave events when pen leaves the range of the tablet. I will add it soon. BUG=713745 ==========
Description was changed from ========== Make sure PointerEvents's isPrimary set correctly for stylus When a pen enters and leaves a tablet's proximity and enters again, the PointerEvents' isPrimary should always be true. Testing page: https://patrickhlauke.github.io/touch/tracker/multi-touch-tracker-pointer-hud... pointerevents/pointerevent_pointerleave_pen-manual.html does not work right because of missing support of PointerLeave events when pen leaves the range of the tablet. I will add it soon. BUG=713745 ========== to ========== Make sure PointerEvents's isPrimary set correctly for stylus When a pen enters and leaves a tablet's proximity and enters again, the PointerEvents' isPrimary should always be true. Testing page: https://patrickhlauke.github.io/touch/tracker/multi-touch-tracker-pointer-hud... pointerevents/pointerevent_pointerleave_pen-manual.html does not work right because of missing support of PointerLeave events when pen leaves the range of the tablet. I will add it soon. BUG=713745 ==========
Description was changed from ========== Make sure PointerEvents's isPrimary set correctly for stylus When a pen enters and leaves a tablet's proximity and enters again, the PointerEvents' isPrimary should always be true. Testing page: https://patrickhlauke.github.io/touch/tracker/multi-touch-tracker-pointer-hud... pointerevents/pointerevent_pointerleave_pen-manual.html does not work right because of missing support of PointerLeave events when pen leaves the range of the tablet. I will add it soon. BUG=713745 ========== to ========== Make sure PointerEvents's isPrimary set correctly for stylus When a pen enters and leaves a tablet's proximity and enters again, the PointerEvents' isPrimary should always be true. Testing page: https://patrickhlauke.github.io/touch/tracker/multi-touch-tracker-pointer-hud... Webplatform test pointerevent_pointerleave_pen-manual.html does not work right now because of no support of PointerLeave events when pen leaves the range of the tablet in PointerActionSequence API. I will add it soon. BUG=713745 ==========
lgtm https://codereview.chromium.org/2831933002/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt_automation/pointerevents/pointerevent_pointerleave_pen-manual-automation.js (right): https://codereview.chromium.org/2831933002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt_automation/pointerevents/pointerevent_pointerleave_pen-manual-automation.js:6: }).then(function() { This one does nothing anyway as we are skipping this test due to the lack of "screen leave" action. But having it here doesn't hurt.
https://codereview.chromium.org/2831933002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2831933002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/PointerEventManager.cpp:527: pointer_event->pointerType() == "pen") { I think this could probably be: mouse_event.pointer_type == kPen which would avoid the string compare.
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: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2831933002/diff/160001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt_automation/pointerevents/pointerevent_pointerleave_pen-manual-automation.js (right): https://codereview.chromium.org/2831933002/diff/160001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt_automation/pointerevents/pointerevent_pointerleave_pen-manual-automation.js:6: }).then(function() { On 2017/04/27 16:04:34, Navid Zolghadr wrote: > This one does nothing anyway as we are skipping this test due to the lack of > "screen leave" action. But having it here doesn't hurt. Yes, this is just remind me that I need to change this test, moving the pen back and forth. https://codereview.chromium.org/2831933002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2831933002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/PointerEventManager.cpp:527: pointer_event->pointerType() == "pen") { On 2017/04/27 16:57:39, dtapuska wrote: > I think this could probably be: > mouse_event.pointer_type == kPen > > which would avoid the string compare. Done.
On 2017/04/27 20:18:20, lanwei wrote: > https://codereview.chromium.org/2831933002/diff/160001/third_party/WebKit/Lay... > File > third_party/WebKit/LayoutTests/external/wpt_automation/pointerevents/pointerevent_pointerleave_pen-manual-automation.js > (right): > > https://codereview.chromium.org/2831933002/diff/160001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/external/wpt_automation/pointerevents/pointerevent_pointerleave_pen-manual-automation.js:6: > }).then(function() { > On 2017/04/27 16:04:34, Navid Zolghadr wrote: > > This one does nothing anyway as we are skipping this test due to the lack of > > "screen leave" action. But having it here doesn't hurt. > > Yes, this is just remind me that I need to change this test, moving the pen back > and forth. > > https://codereview.chromium.org/2831933002/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): > > https://codereview.chromium.org/2831933002/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/input/PointerEventManager.cpp:527: > pointer_event->pointerType() == "pen") { > On 2017/04/27 16:57:39, dtapuska wrote: > > I think this could probably be: > > mouse_event.pointer_type == kPen > > > > which would avoid the string compare. > > Done. lgtm
The CQ bit was checked by lanwei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nzolghadr@chromium.org Link to the patchset: https://codereview.chromium.org/2831933002/#ps180001 (title: "isprimary")
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": 180001, "attempt_start_ts": 1493388773569270,
"parent_rev": "50cd18bc75066ab5f78d8b1469b69d8d621ed9b8", "commit_rev":
"c007949541f4d2238c9653a4e3b8dbc0987bc752"}
Message was sent while issue was closed.
Description was changed from ========== Make sure PointerEvents's isPrimary set correctly for stylus When a pen enters and leaves a tablet's proximity and enters again, the PointerEvents' isPrimary should always be true. Testing page: https://patrickhlauke.github.io/touch/tracker/multi-touch-tracker-pointer-hud... Webplatform test pointerevent_pointerleave_pen-manual.html does not work right now because of no support of PointerLeave events when pen leaves the range of the tablet in PointerActionSequence API. I will add it soon. BUG=713745 ========== to ========== Make sure PointerEvents's isPrimary set correctly for stylus When a pen enters and leaves a tablet's proximity and enters again, the PointerEvents' isPrimary should always be true. Testing page: https://patrickhlauke.github.io/touch/tracker/multi-touch-tracker-pointer-hud... Webplatform test pointerevent_pointerleave_pen-manual.html does not work right now because of no support of PointerLeave events when pen leaves the range of the tablet in PointerActionSequence API. I will add it soon. BUG=713745 Review-Url: https://codereview.chromium.org/2831933002 Cr-Commit-Position: refs/heads/master@{#467997} Committed: https://chromium.googlesource.com/chromium/src/+/c007949541f4d2238c9653a4e3b8... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:180001) as https://chromium.googlesource.com/chromium/src/+/c007949541f4d2238c9653a4e3b8... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
