Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1602443006:
Set the correct buttons for long press action (Closed)
|
Created:
4 years, 11 months ago by Navid Zolghadr Modified:
4 years, 11 months ago CC:
chromium-reviews, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSet the correct buttons for long press action
Set "2" in buttons to indicate right button pressed in both
mousedown and contextmenu originiated from a long press action.
BUG=576281
Committed: https://crrev.com/6a9b6b4394511b90600b6af16e9a01b43a07a479
Cr-Commit-Position: refs/heads/master@{#370714}
Patch Set 1 #
Total comments: 8
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Messages
Total messages: 16 (4 generated)
nzolghadr@chromium.org changed reviewers: + mustaq@chromium.org, rbyers@chromium.org
https://codereview.chromium.org/1602443006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1602443006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:3015: PlatformEvent::NoModifiers, PlatformMouseEvent::RealOrIndistinguishable, Please use the same logic here too. https://codereview.chromium.org/1602443006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:3036: eventType = PlatformEvent::MouseReleased; This seems to be firing mouseup when showContextMenuOnMouseUp is true but the page is not seeing this because I believe the context menu code hides it. Please add a comment here to explain this, and make the "else" part below unconditional.
https://codereview.chromium.org/1602443006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1602443006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:3015: PlatformEvent::NoModifiers, PlatformMouseEvent::RealOrIndistinguishable, On 2016/01/19 19:33:41, mustaq wrote: > Please use the same logic here too. No. I don't think that is correct. This is different. We have not sent any mouse down before and this just sends the context menu event having right click mouse being the cause of it (for compatibility reason). Or do you mean to also send the mouse up/down in this case as well? https://codereview.chromium.org/1602443006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:3036: eventType = PlatformEvent::MouseReleased; On 2016/01/19 19:33:41, mustaq wrote: > This seems to be firing mouseup when showContextMenuOnMouseUp is true but the > page is not seeing this because I believe the context menu code hides it. Please > add a comment here to explain this, and make the "else" part below > unconditional. That is not the case. Although we set the eventType to MouseReleased but since we call handleMousePressEvent it eventually sends a mousedown event. Should we send a mouse up and mouse down if we show context on mouse up in this case? If we send them both I can understand to have buttons=0 in contextmenu event as well. Any thoughts?
https://codereview.chromium.org/1602443006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1602443006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:3015: PlatformEvent::NoModifiers, PlatformMouseEvent::RealOrIndistinguishable, On 2016/01/19 20:41:40, Navid Zolghadr wrote: > On 2016/01/19 19:33:41, mustaq wrote: > > Please use the same logic here too. > > No. I don't think that is correct. This is different. We have not sent any mouse > down before and this just sends the context menu event having right click mouse > being the cause of it (for compatibility reason). Or do you mean to also send > the mouse up/down in this case as well? I didn't mean sending extra events here. With button=RightButton, we are still pretending that the right button is down if sending MousePressed, right? PlatformEvent::NoModifiers is therefore inconsistent here. Should really depend on eventType: none if mouseRelease, rightButton if mousePressed. The spec is a bit cryptic here: https://html.spec.whatwg.org/multipage/forms.html#context-menus but the MDN page is clear: https://developer.mozilla.org/en-US/docs/Web/Events/contextmenu https://codereview.chromium.org/1602443006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:3036: eventType = PlatformEvent::MouseReleased; On 2016/01/19 20:41:40, Navid Zolghadr wrote: > On 2016/01/19 19:33:41, mustaq wrote: > > This seems to be firing mouseup when showContextMenuOnMouseUp is true but the > > page is not seeing this because I believe the context menu code hides it. > Please > > add a comment here to explain this, and make the "else" part below > > unconditional. > > That is not the case. Although we set the eventType to MouseReleased but since > we call handleMousePressEvent it eventually sends a mousedown event. Should we > send a mouse up and mouse down if we show context on mouse up in this case? If > we send them both I can understand to have buttons=0 in contextmenu event as > well. Any thoughts? The down vs up mix up here seems non-trivial for sure. Calling handleMousePresEvent() on mouseReleased seems bad here, but I agree it's not easy to resolve. The method seems to have hit-testing related branching on event-type. Let's leave it as is. But the "else" part is effectively unconditional because you are ORing with RBDown anyway, right? If I am not missing anything, please move it out of "else" with a comment that we are sending mousedown, and remove the ORing below.
https://codereview.chromium.org/1602443006/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1602443006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:3015: PlatformEvent::NoModifiers, PlatformMouseEvent::RealOrIndistinguishable, On 2016/01/19 21:50:13, mustaq wrote: > On 2016/01/19 20:41:40, Navid Zolghadr wrote: > > On 2016/01/19 19:33:41, mustaq wrote: > > > Please use the same logic here too. > > > > No. I don't think that is correct. This is different. We have not sent any > mouse > > down before and this just sends the context menu event having right click > mouse > > being the cause of it (for compatibility reason). Or do you mean to also send > > the mouse up/down in this case as well? > > I didn't mean sending extra events here. With button=RightButton, we are still > pretending that the right button is down if sending MousePressed, right? > PlatformEvent::NoModifiers is therefore inconsistent here. Should really depend > on eventType: none if mouseRelease, rightButton if mousePressed. > > The spec is a bit cryptic here: > https://html.spec.whatwg.org/multipage/forms.html#context-menus > but the MDN page is clear: > https://developer.mozilla.org/en-US/docs/Web/Events/contextmenu I can definitely do that. The only point is that this is not what FF itself does. It always sets the buttons field to 0 if I press menu key on FF/Linux (which shows context menu on mouse down). https://codereview.chromium.org/1602443006/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:3036: eventType = PlatformEvent::MouseReleased; On 2016/01/19 21:50:13, mustaq wrote: > On 2016/01/19 20:41:40, Navid Zolghadr wrote: > > On 2016/01/19 19:33:41, mustaq wrote: > > > This seems to be firing mouseup when showContextMenuOnMouseUp is true but > the > > > page is not seeing this because I believe the context menu code hides it. > > Please > > > add a comment here to explain this, and make the "else" part below > > > unconditional. > > > > That is not the case. Although we set the eventType to MouseReleased but since > > we call handleMousePressEvent it eventually sends a mousedown event. Should we > > send a mouse up and mouse down if we show context on mouse up in this case? If > > we send them both I can understand to have buttons=0 in contextmenu event as > > well. Any thoughts? > > The down vs up mix up here seems non-trivial for sure. Calling > handleMousePresEvent() on mouseReleased seems bad here, but I agree it's not > easy to resolve. The method seems to have hit-testing related branching on > event-type. Let's leave it as is. > > But the "else" part is effectively unconditional because you are ORing with > RBDown anyway, right? If I am not missing anything, please move it out of "else" > with a comment that we are sending mousedown, and remove the ORing below. Sure. I will remove the else.
I fixed the points with the solutions that we discussed. ptal
lgtm https://codereview.chromium.org/1602443006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1602443006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.cpp:3041: // TODO(nzolghadr): crbug.com/579564 Maybe we should not send mouse down at all Please consider using "TODO(crbug.com/579564)" since the TODO is "equivalent" to the bug fix here. https://google.github.io/styleguide/cppguide.html#TODO_Comments
https://codereview.chromium.org/1602443006/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/1602443006/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandler.cpp:3041: // TODO(nzolghadr): crbug.com/579564 Maybe we should not send mouse down at all On 2016/01/20 18:10:26, mustaq wrote: > Please consider using "TODO(crbug.com/579564)" since the TODO is "equivalent" to > the bug fix here. > > https://google.github.io/styleguide/cppguide.html#TODO_Comments Done.
LGTM
The CQ bit was checked by nzolghadr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org Link to the patchset: https://codereview.chromium.org/1602443006/#ps40001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1602443006/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1602443006/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Set the correct buttons for long press action Set "2" in buttons to indicate right button pressed in both mousedown and contextmenu originiated from a long press action. BUG=576281 ========== to ========== Set the correct buttons for long press action Set "2" in buttons to indicate right button pressed in both mousedown and contextmenu originiated from a long press action. BUG=576281 Committed: https://crrev.com/6a9b6b4394511b90600b6af16e9a01b43a07a479 Cr-Commit-Position: refs/heads/master@{#370714} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/6a9b6b4394511b90600b6af16e9a01b43a07a479 Cr-Commit-Position: refs/heads/master@{#370714} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
