Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2807433003:
No pointer captured when the pointer lock is applied (Closed)
|
Created:
3 years, 8 months ago by lanwei Modified:
3 years, 6 months ago CC:
chromium-reviews, dtapuska+blinkwatch_chromium.org, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, Navid Zolghadr, blink-reviews, kinuko+watch, rwlbuis Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNo pointer captured when the pointer lock is applied
We want to make the pointer lock always take priority over the capture.
There are three cases:
1. If we set a pointer lock, at the same time we want to set the pointer
capture as well, we should not set pointer capture and send out 'gotpointercapture'
event.
2. After we set a pointer lock, we should throw an error when trying to set
any pointer capture.
3. If there is a pointer capture and when trying to request a pointer lock,
we should release the pointer capture.
Testing pages:
http://jsbin.com/zasesu/quiet
http://jsbin.com/qabopik/quiet
http://jsbin.com/fucewup/quiet
BUG=697581
Review-Url: https://codereview.chromium.org/2807433003
Cr-Commit-Position: refs/heads/master@{#475572}
Committed: https://chromium.googlesource.com/chromium/src/+/3d0d129b2cbc4e9547e34ffbf67a5e872b7d006f
Patch Set 1 : pointer lock #
Total comments: 7
Patch Set 2 : pointer lock #
Total comments: 18
Patch Set 3 : pointer lock #
Total comments: 5
Patch Set 4 : Change the tests and move code to WebFrameWidgetBase #
Total comments: 13
Patch Set 5 : Merge tests and rename functions #
Total comments: 4
Patch Set 6 : pointer lock #Messages
Total messages: 84 (65 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...
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...
Description was changed from ========== pointer lock BUG= ========== to ========== When the pointer is about to be locked or already locked, there is no pointer captured When we set the pointer lock, we should not set any pointer capture, and we should release the pointer capture if there is any. Testing pages: http://jsbin.com/zasesu/edit?html,output http://jsbin.com/qabopik/7/edit?html,output BUG=697581 ==========
lanwei@chromium.org changed reviewers: + nzolghadr@chromium.org
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== When the pointer is about to be locked or already locked, there is no pointer captured When we set the pointer lock, we should not set any pointer capture, and we should release the pointer capture if there is any. Testing pages: http://jsbin.com/zasesu/edit?html,output http://jsbin.com/qabopik/7/edit?html,output BUG=697581 ========== to ========== When the pointer is about to be locked or already locked, there is no pointer captured When we set the pointer lock, we should not set any pointer capture, and we should release the pointer capture if there is any. Testing pages: http://jsbin.com/zasesu/quiet http://jsbin.com/qabopik/quiet http://jsbin.com/fucewup/quiet BUG=697581 ==========
Overall sounds good. Although you need to put up a PR for the spec and call out how to handle these cases first. After that lands we can proceed with landing this. Also we need to have -manual wpt tests similar to other pointer event tests for this. You can include them in the same CL and they get upstream later. I believe the best place for these tests are under wpt/pointerevents/pointerlock/. https://codereview.chromium.org/2807433003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2807433003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:3039: InvalidStateError, "Cannot capture when the mouse is locked"); I believe we need to put the error in pointer event spec. Maybe using the same invalidStateError? https://codereview.chromium.org/2807433003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2807433003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.cpp:1196: void EventHandler::releasePointerCapture() { Can we use a mouse specific name for this function as it only releases the capture for mouse? Something like releaseMousePointerCapture? https://codereview.chromium.org/2807433003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.h (right): https://codereview.chromium.org/2807433003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.h:65: void releasePointerCapture(int); I suggest having the same releaseMousePointerCapture here instead of making this function public. https://codereview.chromium.org/2807433003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2807433003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:2250: LocalFrame* focusedFrame = focusedLocalFrameInWidget(); Please make sure scheib@ knows about this change in case he knows other places that we are missing for this. Maybe have the behavior in different cases documented in a one pager document in case he has any questions regarding this.
Description was changed from ========== When the pointer is about to be locked or already locked, there is no pointer captured When we set the pointer lock, we should not set any pointer capture, and we should release the pointer capture if there is any. Testing pages: http://jsbin.com/zasesu/quiet http://jsbin.com/qabopik/quiet http://jsbin.com/fucewup/quiet BUG=697581 ========== to ========== No pointer captured when the pointer lock is applied When we set the pointer lock, we should not set any pointer capture, and we should release the pointer capture if there is any. Testing pages: http://jsbin.com/zasesu/quiet http://jsbin.com/qabopik/quiet http://jsbin.com/fucewup/quiet BUG=697581 ==========
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.
lanwei@chromium.org changed reviewers: + scheib@chromium.org
Description was changed from ========== No pointer captured when the pointer lock is applied When we set the pointer lock, we should not set any pointer capture, and we should release the pointer capture if there is any. Testing pages: http://jsbin.com/zasesu/quiet http://jsbin.com/qabopik/quiet http://jsbin.com/fucewup/quiet BUG=697581 ========== to ========== No pointer captured when the pointer lock is applied We want to make the pointer lock always take priority over the capture. There are three cases: If we set a pointer lock, at the same time we want to set the pointer capture as well, we should not set pointer capture and send out 'gotpointercapture' event. After we set a pointer lock, we should throw an error when trying to set any pointer capture. If there is a pointer capture and when trying to request a pointer lock, we should release the pointer capture. Testing pages: http://jsbin.com/zasesu/quiet http://jsbin.com/qabopik/quiet http://jsbin.com/fucewup/quiet BUG=697581 ==========
Description was changed from ========== No pointer captured when the pointer lock is applied We want to make the pointer lock always take priority over the capture. There are three cases: If we set a pointer lock, at the same time we want to set the pointer capture as well, we should not set pointer capture and send out 'gotpointercapture' event. After we set a pointer lock, we should throw an error when trying to set any pointer capture. If there is a pointer capture and when trying to request a pointer lock, we should release the pointer capture. Testing pages: http://jsbin.com/zasesu/quiet http://jsbin.com/qabopik/quiet http://jsbin.com/fucewup/quiet BUG=697581 ========== to ========== No pointer captured when the pointer lock is applied We want to make the pointer lock always take priority over the capture. There are three cases: 1. If we set a pointer lock, at the same time we want to set the pointer capture as well, we should not set pointer capture and send out 'gotpointercapture' event. 2. After we set a pointer lock, we should throw an error when trying to set any pointer capture. 3. If there is a pointer capture and when trying to request a pointer lock, we should release the pointer capture. Testing pages: http://jsbin.com/zasesu/quiet http://jsbin.com/qabopik/quiet http://jsbin.com/fucewup/quiet BUG=697581 ==========
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 checked by lanwei@chromium.org to run a CQ dry run
Patchset #3 (id:60001) 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 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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:80001) has been deleted
https://codereview.chromium.org/2807433003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2807433003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:3039: InvalidStateError, "Cannot capture when the mouse is locked"); On 2017/04/10 17:16:14, Navid Zolghadr wrote: > I believe we need to put the error in pointer event spec. Maybe using the same > invalidStateError? Done. https://codereview.chromium.org/2807433003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2807433003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.cpp:1196: void EventHandler::releasePointerCapture() { On 2017/04/10 17:16:14, Navid Zolghadr wrote: > Can we use a mouse specific name for this function as it only releases the > capture for mouse? Something like releaseMousePointerCapture? Done. https://codereview.chromium.org/2807433003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.h (right): https://codereview.chromium.org/2807433003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.h:65: void releasePointerCapture(int); On 2017/04/10 17:16:14, Navid Zolghadr wrote: > I suggest having the same releaseMousePointerCapture here instead of making this > function public. Done.
https://codereview.chromium.org/2807433003/diff/100001/content/browser/render... File content/browser/renderer_host/input/synthetic_mouse_driver.cc (right): https://codereview.chromium.org/2807433003/diff/100001/content/browser/render... content/browser/renderer_host/input/synthetic_mouse_driver.cc:22: mouse_event_.SetType(blink::WebInputEvent::kUndefined); We set the type to kUndefined again if it is already kUndefined? https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock-manual.html (right): https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock-manual.html:47: assert_equals(capture_count, 0, "no capture"); Maybe a better error message here. Like "There shouldn't be any capture events fired" or something similar. https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock-manual.html:61: <li>Press left button down on the blue rectangle and hold it.</li> Do you want to add a little move here as well? Also there is no blue div in these tests. You need to update instructions for all the tests. https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock_after_pointercapture-manual.html (right): https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock_after_pointercapture-manual.html:28: on_event(div2, 'mousedown', function(event) { I believe we shouldn't rely on mouse events here. Can we just listen to the pointermove here instead and add a little move to the test instructions after pointerdown? https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock_after_pointercapture-manual.html:36: lost_capture = true; I believe we need to make sure we get this lostpointercapture before div1 gets any move after lock is active. Can you also test this in this file? That should catch that problem I mentioned in the code as well. https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock_after_pointercapture-manual.html:52: <li>Press down on the blue rectangle and hold it.</li> ditto https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock_no_pointercapture-manual.html (right): https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock_no_pointercapture-manual.html:31: assert_true(false, "DOMException: InvalidStateError should have been thrown."); There is another function called assert_unreached for this usecase. https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock_no_pointercapture-manual.html:62: <li>Press left button down on the blue rectangle and hold it.</li> ditto https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt_automation/pointerevents/pointerevent_common_input.js (right): https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt_automation/pointerevents/pointerevent_common_input.js:220: {name: 'pointerDown', x: xPosition, y: yPosition, button: 'left'}, Can you add a little move after the left pressed as well? https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:2204: focusedFrame->GetEventHandler().DispatchLostPointerCaptureEvent( This doesn't seem correct. We should have called PointerEventManager::ProcessPendingPointerCapture before the pointerevent is sent out instead of this. So basically there are two points here: 1. We added another function (i.e. DispatchLostPointerCaptureEvent) which is in charge of lostpointercapture instead of reusing what we had PointerEventManager::ProcessPendingPointerCapture. 2. It seems that this sends out lostpointercapture after the first pointermove that is sent to the lock element. The lostpointercapture should be sent before the pointermove. https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:2299: focusedFrame->GetEventHandler().ReleaseMousePointerCapture(); Why do we need to release mouse capture when capture is lost?
I agree with the intent of the change. Tests are shaping up, I agree with comments already there, add a small note: https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock-manual.html (right): https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock-manual.html:61: <li>Press left button down on the blue rectangle and hold it.</li> On 2017/05/17 16:30:10, Navid Zolghadr wrote: > Do you want to add a little move here as well? Also there is no blue div in > these tests. You need to update instructions for all the tests. The middle button press seems to be a distraction as well. This test only uses 'pointerdown' repeatedly -- just click two times, right?
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 checked by lanwei@chromium.org to run a CQ dry run
Patchset #3 (id:120001) 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2807433003/diff/100001/content/browser/render... File content/browser/renderer_host/input/synthetic_mouse_driver.cc (right): https://codereview.chromium.org/2807433003/diff/100001/content/browser/render... content/browser/renderer_host/input/synthetic_mouse_driver.cc:22: mouse_event_.SetType(blink::WebInputEvent::kUndefined); On 2017/05/17 16:30:10, Navid Zolghadr wrote: > We set the type to kUndefined again if it is already kUndefined? Done. https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock-manual.html (right): https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock-manual.html:47: assert_equals(capture_count, 0, "no capture"); On 2017/05/17 16:30:10, Navid Zolghadr wrote: > Maybe a better error message here. Like "There shouldn't be any capture events > fired" or something similar. Done. https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock-manual.html:61: <li>Press left button down on the blue rectangle and hold it.</li> On 2017/05/17 16:30:10, Navid Zolghadr wrote: > Do you want to add a little move here as well? Also there is no blue div in > these tests. You need to update instructions for all the tests. Done. https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock_no_pointercapture-manual.html (right): https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock_no_pointercapture-manual.html:31: assert_true(false, "DOMException: InvalidStateError should have been thrown."); On 2017/05/17 16:30:10, Navid Zolghadr wrote: > There is another function called assert_unreached for this usecase. Acknowledged. https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt_automation/pointerevents/pointerevent_common_input.js (right): https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt_automation/pointerevents/pointerevent_common_input.js:220: {name: 'pointerDown', x: xPosition, y: yPosition, button: 'left'}, On 2017/05/17 16:30:10, Navid Zolghadr wrote: > Can you add a little move after the left pressed as well? Done. https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2807433003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:2299: focusedFrame->GetEventHandler().ReleaseMousePointerCapture(); On 2017/05/17 16:30:11, Navid Zolghadr wrote: > Why do we need to release mouse capture when capture is lost? This is for the case that we call the setPointerCapture and requestPointerLock, we should release the pointer capture when we release the pointer lock, so no gotpointercapture is sent.
https://codereview.chromium.org/2807433003/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock-manual.html (right): https://codereview.chromium.org/2807433003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock-manual.html:28: on_event(div1, 'pointermove', function(event) { why not pointerup? If using move, I'm concerned that you need to wait to register the move only after pointerlock has been entered (similar to other test's got_capture) https://codereview.chromium.org/2807433003/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock_no_pointercapture-manual.html (right): https://codereview.chromium.org/2807433003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock_no_pointercapture-manual.html:26: on_event(div1, 'pointermove', function(event) { ditto pointermove concerns
mustaq@chromium.org changed reviewers: + mustaq@chromium.org
https://codereview.chromium.org/2807433003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2807433003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:2206: MainFrameImpl()->FrameWidget()->PointerLockMouseEvent(input_event); I believe we need a similar fix before another direct dispatch call which in WebFrameWidgetImpl::HandleInputEvent. But same fix in two places would be ugly. Perhaps move WebFrameWidgetBase::PointerLockMouseEvent into EventHandler and call ProcessPendingPointerCapture there? Another related thought: these calls ultimately reach PointerLockController::DispatchLockedMouseEvent where we synthesize a "click" after a "mouseup". Can we reuse the existing code around EventHandler for this, perhaps by replacing calls to element_->DispatchMouseEvent by calls to EventHandler methods? This would also fix the PointerLock part of crbug.com/665924.
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 checked by lanwei@chromium.org to run a 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 #4 (id:160001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2807433003/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock-manual.html (right): https://codereview.chromium.org/2807433003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock-manual.html:28: on_event(div1, 'pointermove', function(event) { On 2017/05/19 19:54:17, scheib wrote: > why not pointerup? > > If using move, I'm concerned that you need to wait to register the move only > after pointerlock has been entered (similar to other test's got_capture) We want to use the same actions for all three test cases. The pointerup will release the pointer capture, so we use pointermove instead. I added some condition checks to make sure we are exiting the pointer lock at the appropriate pointermove. https://codereview.chromium.org/2807433003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/2807433003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:2206: MainFrameImpl()->FrameWidget()->PointerLockMouseEvent(input_event); On 2017/05/19 20:05:33, mustaq wrote: > I believe we need a similar fix before another direct dispatch call which in > WebFrameWidgetImpl::HandleInputEvent. > > But same fix in two places would be ugly. Perhaps move > WebFrameWidgetBase::PointerLockMouseEvent into EventHandler and call > ProcessPendingPointerCapture there? > > Another related thought: these calls ultimately reach > PointerLockController::DispatchLockedMouseEvent where we synthesize a "click" > after a "mouseup". Can we reuse the existing code around EventHandler for this, > perhaps by replacing calls to > element_->DispatchMouseEvent > by calls to EventHandler methods? This would also fix the PointerLock part of > crbug.com/665924. You are right, I moved the code to the WebFrameWidgetBase class, and add a todo.
lgtm https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/WebFrameWidgetBase.cpp (right): https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/WebFrameWidgetBase.cpp:258: const WebCoalescedInputEvent& coalesced_event) { I love the idea that you changed this to WebCoalescedInputEvent. We should be using it in pointermoves being sent during pointerlock later to put the coalesced events in there. So definitely keep this one. nit: But there is no need to create and pass transformed_coalesced_events to focusedFrame->GetEventHandler().ProcessPendingPointerCapture(). In there you can just pass an empty vector (i.e. Vector<WebMouseEvent>()) to the pointer_event_factory_.Create(...) as got/lostpointercapture events will never have coalecsed events.
LGTM Suggested filename change. Added a note where I'm not an effective reviewer. https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock-manual.html (right): https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock-manual.html:1: <!doctype html> Filename should be improved. e.g. ...pointerlock_supercedes_capture https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/WebFrameWidgetBase.cpp (right): https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/WebFrameWidgetBase.cpp:257: void WebFrameWidgetBase::PointerLockMouseEvent( NOTE: I'm not familiar enough with this code, and complexity is high, so I can't vouch for it.
Looks good overall but I would ask for a few cleanup. Also: I didn't quite understand the 3 cases in the CL description until I looked into the tests. For case 1, I think you meant pointer lock request while there is a pending pointer capture, right? https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock_no_pointercapture-manual.html (right): https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock_no_pointercapture-manual.html:1: <!doctype html> This test should be easily done inside the first test above, right? Just call |setPointerCapture| & check the exception right before the |exitPointerLock| call. https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:3082: } else if (GetDocument().GetPage() && GetDocument() Nit: Combine with the above |if| with an OR, since both throw the same exception. https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.cpp:1237: void EventHandler::ProcessPendingPointerCapture( Please rename to make the API usage clear, otherwise the ME param look meaningless. See my comment in PointerEventManager::ProcessPendingPointerCapture(). https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/PointerEventManager.cpp:557: bool send_mouse_event) { A clean-up request: the last two parameters should be replaced by a single WebMouseEvent pointer which defaults to null. https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/PointerEventManager.cpp:611: void PointerEventManager::ProcessPendingPointerCapture( The existing method with the same name is wrapped here for a particular purpose, so let's make it clear from the name here. May be |ProcessPendingPointerCaptureForPointerLock|? Also, as Navid suggested, drop the |coalesced_events| parameter and create an empty vector when calling |Create()| here.
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...
lanwei@chromium.org changed reviewers: + dtapuska@chromium.org
lanwei@chromium.org changed reviewers: + bokan@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) 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_...)
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 #5 (id:200001) 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...
Patchset #5 (id:220001) has been deleted
https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock-manual.html (right): https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerlock/pointerevent_pointerlock-manual.html:1: <!doctype html> On 2017/05/26 05:44:12, scheib wrote: > Filename should be improved. > e.g. ...pointerlock_supercedes_capture Done. https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:3082: } else if (GetDocument().GetPage() && GetDocument() On 2017/05/26 14:56:18, mustaq wrote: > Nit: Combine with the above |if| with an OR, since both throw the same > exception. Done. https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/WebFrameWidgetBase.cpp (right): https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/WebFrameWidgetBase.cpp:258: const WebCoalescedInputEvent& coalesced_event) { On 2017/05/25 16:59:15, Navid Zolghadr wrote: > I love the idea that you changed this to WebCoalescedInputEvent. We should be > using it in pointermoves being sent during pointerlock later to put the > coalesced events in there. So definitely keep this one. > > nit: But there is no need to create and pass transformed_coalesced_events to > focusedFrame->GetEventHandler().ProcessPendingPointerCapture(). In there you can > just pass an empty vector (i.e. Vector<WebMouseEvent>()) to the > pointer_event_factory_.Create(...) as got/lostpointercapture events will never > have coalecsed events. Done. https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/EventHandler.cpp:1237: void EventHandler::ProcessPendingPointerCapture( On 2017/05/26 14:56:19, mustaq wrote: > Please rename to make the API usage clear, otherwise the ME param look > meaningless. See my comment in > PointerEventManager::ProcessPendingPointerCapture(). Done. https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2807433003/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/PointerEventManager.cpp:611: void PointerEventManager::ProcessPendingPointerCapture( On 2017/05/26 14:56:19, mustaq wrote: > The existing method with the same name is wrapped here for a particular purpose, > so let's make it clear from the name here. May be > |ProcessPendingPointerCaptureForPointerLock|? > > Also, as Navid suggested, drop the |coalesced_events| parameter and create an > empty vector when calling |Create()| here. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2807433003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2807433003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/PointerEventManager.cpp:552: // TODO(727333): Replace the last two parameters by a single WebMouseEvent Nit: Let's stick to the official style: TODO(lanwei): ... crbug.com/bugNo.
rs lgtm based on other reviewers.
lgtm % please fix mustaq's comment and my nit https://codereview.chromium.org/2807433003/diff/240001/content/browser/render... File content/browser/renderer_host/input/synthetic_mouse_driver.cc (right): https://codereview.chromium.org/2807433003/diff/240001/content/browser/render... content/browser/renderer_host/input/synthetic_mouse_driver.cc:22: mouse_event_.SetType(blink::WebInputEvent::kUndefined); Can we avoid the calls to SetType() and just mouse_event_ = blink::WebMouseEvent(); to clear all state?
The CQ bit was checked by lanwei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org, nzolghadr@chromium.org, mustaq@chromium.org, bokan@chromium.org, dtapuska@chromium.org Link to the patchset: https://codereview.chromium.org/2807433003/#ps260001 (title: "pointer lock")
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": 260001, "attempt_start_ts": 1496156369990810,
"parent_rev": "1c4bde5b5820211c8954b82648bbe089ca117336", "commit_rev":
"3d0d129b2cbc4e9547e34ffbf67a5e872b7d006f"}
Message was sent while issue was closed.
Description was changed from ========== No pointer captured when the pointer lock is applied We want to make the pointer lock always take priority over the capture. There are three cases: 1. If we set a pointer lock, at the same time we want to set the pointer capture as well, we should not set pointer capture and send out 'gotpointercapture' event. 2. After we set a pointer lock, we should throw an error when trying to set any pointer capture. 3. If there is a pointer capture and when trying to request a pointer lock, we should release the pointer capture. Testing pages: http://jsbin.com/zasesu/quiet http://jsbin.com/qabopik/quiet http://jsbin.com/fucewup/quiet BUG=697581 ========== to ========== No pointer captured when the pointer lock is applied We want to make the pointer lock always take priority over the capture. There are three cases: 1. If we set a pointer lock, at the same time we want to set the pointer capture as well, we should not set pointer capture and send out 'gotpointercapture' event. 2. After we set a pointer lock, we should throw an error when trying to set any pointer capture. 3. If there is a pointer capture and when trying to request a pointer lock, we should release the pointer capture. Testing pages: http://jsbin.com/zasesu/quiet http://jsbin.com/qabopik/quiet http://jsbin.com/fucewup/quiet BUG=697581 Review-Url: https://codereview.chromium.org/2807433003 Cr-Commit-Position: refs/heads/master@{#475572} Committed: https://chromium.googlesource.com/chromium/src/+/3d0d129b2cbc4e9547e34ffbf67a... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:260001) as https://chromium.googlesource.com/chromium/src/+/3d0d129b2cbc4e9547e34ffbf67a...
Message was sent while issue was closed.
https://codereview.chromium.org/2807433003/diff/240001/content/browser/render... File content/browser/renderer_host/input/synthetic_mouse_driver.cc (right): https://codereview.chromium.org/2807433003/diff/240001/content/browser/render... content/browser/renderer_host/input/synthetic_mouse_driver.cc:22: mouse_event_.SetType(blink::WebInputEvent::kUndefined); On 2017/05/29 20:57:40, dtapuska wrote: > Can we avoid the calls to SetType() and just mouse_event_ = > blink::WebMouseEvent(); > > to clear all state? We still need other states, like button, click count. https://codereview.chromium.org/2807433003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/2807433003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/input/PointerEventManager.cpp:552: // TODO(727333): Replace the last two parameters by a single WebMouseEvent On 2017/05/29 18:37:44, mustaq wrote: > Nit: Let's stick to the official style: > TODO(lanwei): ... crbug.com/bugNo. Done. |
