Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
727593003:
Implement MouseEvent buttons attribute. (Closed)
DescriptionImplement MouseEvent buttons attribute.
During any mouse events, buttons must be used to indicate which combination of
mouse buttons are currently being pressed, expressed as a bitmask.
Intent to implement and ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/tMvB-8CrE3M
Spec:
http://www.w3.org/TR/DOM-Level-3-Events/#widl-MouseEvent-buttons
Dependency in chromium side (for test):
https://codereview.chromium.org/732433002/
BUG=276941
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186490
Patch Set 1 : #
Total comments: 22
Patch Set 2 : #Patch Set 3 : #
Total comments: 24
Patch Set 4 : addressed comments #
Total comments: 16
Patch Set 5 : rebase #
Total comments: 2
Patch Set 6 : should be 2 in contextmenu event. #Patch Set 7 : should be 0 in contextmenu event #
Total comments: 19
Patch Set 8 : fixed #
Total comments: 6
Patch Set 9 : nits #Messages
Total messages: 40 (18 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
jinho.bang@samsung.com changed reviewers: + dglazkov@chromium.org, rbyers@chromium.org
jinho.bang@samsung.com changed reviewers: + kbr@chromium.org - dglazkov@chromium.org
jinho.bang@samsung.com changed reviewers: + tkent@chromium.org - kbr@chromium.org
Dear all, I've just sent to intent to blink-dev. I already know that we need 'lgtm x 3' from API owners before landing this patch. So, I'm waiting for responses. Anyway, please take a look at this. Thank you. (But not gotten lgtm yet.) Anyway, Please take a look at this.
Dear all, I've just sent to intent to blink-dev. I already know that we need 'lgtm x 3' from API owners before landing this patch. So, I'm waiting for responses. Anyway, please take a look at this. Thank you.
rbyers@chromium.org changed reviewers: - tkent@chromium.org
On 2014/11/14 07:35:40, zino wrote: > Dear all, > > I've just sent to intent to blink-dev. > I already know that we need 'lgtm x 3' from API owners before landing this > patch. > So, I'm waiting for responses. > > Anyway, please take a look at this. > > Thank you. Thanks for the super-fast turn around Jinho! I moved tkent to cc since it wasn't clear which of us you wanted to do the initial review of all files. I can add the necessary additional owners once I'm done reviewing.
On 2014/11/14 07:35:40, zino wrote: > Dear all, > > I've just sent to intent to blink-dev. > I already know that we need 'lgtm x 3' from API owners before landing this > patch. > So, I'm waiting for responses. Yep, we'll need to wait for the LGTMs before landing, but I don't anticipate any push back so I'm happy to review the CL ahead of time.
This is great, thank you! Unfortunately there are a couple details that make this a little more complicated than we both anticipated (but shouldn't be too bad I think). https://codereview.chromium.org/727593003/diff/100001/LayoutTests/fast/events... File LayoutTests/fast/events/mouse-event-buttons-attribute.html (right): https://codereview.chromium.org/727593003/diff/100001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:6: testRunner.waitUntilDone(); eventSender works synchronously, you can simplify the test by removing this async stuff. https://codereview.chromium.org/727593003/diff/100001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:8: window.addEventListener('mousedown', function() { Please add handlers for the other event types too and test that they include correct buttons values. At least: - mousemove - both before and after a button is depressed - click, dblclick, mouseenter, mouseleave, mouseover and mouseout - all of which blink synthesizes on and will need to propagate the button state explicitly for Also can you manually verify on at least one chrome platform what the behavior is for mouseup? The spec doesn't seem entirely clear on this, but I assumed it should be 0 when releasing a button. However testing Firefox on Linux I see the 'buttons' value represents the state BEFORE the button is released - perhaps I missed the explanation for this in the spec? Obviously this is a chromium-side bug/change, not directly applicable to this blink CL. Also you need a test for creating mouse events from javascript with non-zero buttons values. What should the behavior of 'buttons' be for MouseEvent objects created with the legacy initMouseEvent API? Eg. if you create a mousedown event with that, is it OK for buttons to still be 0? The spec seems silent on this, please check what IE and/or FireFox do here. https://codereview.chromium.org/727593003/diff/100001/Source/core/events/Whee... File Source/core/events/WheelEvent.cpp (right): https://codereview.chromium.org/727593003/diff/100001/Source/core/events/Whee... Source/core/events/WheelEvent.cpp:65: pageLocation.x(), pageLocation.y(), 0, 0, ctrlKey, altKey, shiftKey, metaKey, 0, 0, WheelEvent is a MouseEvent too, so I think you'll need to support and test the buttons value on it too (although I'd be curious to hear whether FireFox and IE set it properly for wheel). If that's going to take some non-trivial work (eg. maybe chromium isn't setting the modifiers on WebWheelEvent today), then feel free to add a FIXME here referencing your bug# and worry about that in follow-up CLs. We don't want to leave the API partially implemented for long though, we can put it behind a RuntimeEnabledFeature if it's likely to take more than a week or so. https://codereview.chromium.org/727593003/diff/100001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/727593003/diff/100001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:1643: 0, 0, nullptr, dataTransfer); This looks like a bug too. DragEvent is a MouseEvent (https://html.spec.whatwg.org/multipage/interaction.html#the-dragevent-interface), so technically the current mouse button state should be reflected there too. Again, feel free to add a FIXME and worry about this in a follow-up patch (it's a bit of an edge case). I'd also be interested to know if FF and IE do this properly (if neither do, then it's probably a low priority bug for us too). https://codereview.chromium.org/727593003/diff/100001/Source/platform/Platfor... File Source/platform/PlatformMouseEvent.h (right): https://codereview.chromium.org/727593003/diff/100001/Source/platform/Platfor... Source/platform/PlatformMouseEvent.h:85: , m_buttons(0) I think using 0 here is a bug. You probably need to let the caller supply the value (here and in the above overload - though maybe that overload can be removed). Eg. see EventHandler::handleGestureTap which creates mouse events for a tap. The events you create there will incorrectly report buttons=0. Please also add a layout test that verifies the buttons value of the mouse events generated for a GestureTap. https://codereview.chromium.org/727593003/diff/100001/Source/web/WebInputEven... File Source/web/WebInputEventConversion.cpp (right): https://codereview.chromium.org/727593003/diff/100001/Source/web/WebInputEven... Source/web/WebInputEventConversion.cpp:99: if (e.modifiers & WebInputEvent::LeftButtonDown) Oh cool, I didn't realize this data was already present in the WebInputEvent modifiers. Great! What platforms have (or will) you be able to validate that this is already set properly on? It looks like there's not much relying on these modifiers today, right? I'm just wondering if it might not be universally supported across platforms. That would be a chromium bug, and I'm OK landing the blink side before worrying too much about this but we shouldn't ship the API to beta without having validated all the platforms set the modifiers properly. https://codereview.chromium.org/727593003/diff/100001/Source/web/WebInputEven... Source/web/WebInputEventConversion.cpp:99: if (e.modifiers & WebInputEvent::LeftButtonDown) Please extend the unit tests in WebInputEventConversionTest.cpp to cover a couple buttons cases also.
@Rick Byers Thank you for detailed explanation. :) I've uploaded a new patch set. Could you please review again? https://codereview.chromium.org/727593003/diff/100001/LayoutTests/fast/events... File LayoutTests/fast/events/mouse-event-buttons-attribute.html (right): https://codereview.chromium.org/727593003/diff/100001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:6: testRunner.waitUntilDone(); On 2014/11/14 19:48:09, Rick Byers wrote: > eventSender works synchronously, you can simplify the test by removing this > async stuff. Done. https://codereview.chromium.org/727593003/diff/100001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:8: window.addEventListener('mousedown', function() { On 2014/11/14 19:48:09, Rick Byers wrote: > Please add handlers for the other event types too and test that they include > correct buttons values. At least: > - mousemove - both before and after a button is depressed > - click, dblclick, mouseenter, mouseleave, mouseover and mouseout - all of > which blink synthesizes on and will need to propagate the button state > explicitly for Done. > > Also can you manually verify on at least one chrome platform what the behavior > is for mouseup? The spec doesn't seem entirely clear on this, but I assumed it > should be 0 when releasing a button. However testing Firefox on Linux I see the > 'buttons' value represents the state BEFORE the button is released - perhaps I > missed the explanation for this in the spec? Obviously this is a chromium-side > bug/change, not directly applicable to this blink CL. > Sorry, I couldn't find your mentioned part. Could you let me know the explanation in the spec? Anyway, currently new implementation seems to have the same behavior. > Also you need a test for creating mouse events from javascript with non-zero > buttons values. What should the behavior of 'buttons' be for MouseEvent objects > created with the legacy initMouseEvent API? Eg. if you create a mousedown > event with that, is it OK for buttons to still be 0? The spec seems silent on > this, please check what IE and/or FireFox do here. I'm not sure but the current spec doesn't seem to say that. Firefox also doesn't support for them. Please see this: https://hg.mozilla.org/mozilla-central/file/7913c9392c5f/dom/events/MouseEven... https://codereview.chromium.org/727593003/diff/100001/Source/core/events/Whee... File Source/core/events/WheelEvent.cpp (right): https://codereview.chromium.org/727593003/diff/100001/Source/core/events/Whee... Source/core/events/WheelEvent.cpp:65: pageLocation.x(), pageLocation.y(), 0, 0, ctrlKey, altKey, shiftKey, metaKey, 0, 0, On 2014/11/14 19:48:09, Rick Byers wrote: > WheelEvent is a MouseEvent too, so I think you'll need to support and test the > buttons value on it too (although I'd be curious to hear whether FireFox and IE > set it properly for wheel). > > If that's going to take some non-trivial work (eg. maybe chromium isn't setting > the modifiers on WebWheelEvent today), then feel free to add a FIXME here > referencing your bug# and worry about that in follow-up CLs. We don't want to > leave the API partially implemented for long though, we can put it behind a > RuntimeEnabledFeature if it's likely to take more than a week or so. Done. https://codereview.chromium.org/727593003/diff/100001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/727593003/diff/100001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:1643: 0, 0, nullptr, dataTransfer); On 2014/11/14 19:48:09, Rick Byers wrote: > This looks like a bug too. DragEvent is a MouseEvent > (https://html.spec.whatwg.org/multipage/interaction.html#the-dragevent-interface), > so technically the current mouse button state should be reflected there too. > Again, feel free to add a FIXME and worry about this in a follow-up patch (it's > a bit of an edge case). I'd also be interested to know if FF and IE do this > properly (if neither do, then it's probably a low priority bug for us too). I left a FIXME comment. This seems to be non-trivial. Also, I've confirmed that FF works correctly. (Sorry about IE, I don't have a window PC..) https://codereview.chromium.org/727593003/diff/100001/Source/platform/Platfor... File Source/platform/PlatformMouseEvent.h (right): https://codereview.chromium.org/727593003/diff/100001/Source/platform/Platfor... Source/platform/PlatformMouseEvent.h:85: , m_buttons(0) On 2014/11/14 19:48:09, Rick Byers wrote: > I think using 0 here is a bug. You probably need to let the caller supply the > value (here and in the above overload - though maybe that overload can be > removed). Eg. see EventHandler::handleGestureTap which creates mouse events for > a tap. The events you create there will incorrectly report buttons=0. Please > also add a layout test that verifies the buttons value of the mouse events > generated for a GestureTap. Good point! Thank you. Done. https://codereview.chromium.org/727593003/diff/100001/Source/web/WebInputEven... File Source/web/WebInputEventConversion.cpp (right): https://codereview.chromium.org/727593003/diff/100001/Source/web/WebInputEven... Source/web/WebInputEventConversion.cpp:99: if (e.modifiers & WebInputEvent::LeftButtonDown) On 2014/11/14 19:48:09, Rick Byers wrote: > Oh cool, I didn't realize this data was already present in the WebInputEvent > modifiers. Great! > > What platforms have (or will) you be able to validate that this is already set > properly on? It looks like there's not much relying on these modifiers today, > right? I'm just wondering if it might not be universally supported across > platforms. That would be a chromium bug, and I'm OK landing the blink side > before worrying too much about this but we shouldn't ship the API to beta > without having validated all the platforms set the modifiers properly. Windows / Linux / Mac works well via layout tests on build bot. Android works well also. I've confirmed it manually. I couldn't check it in IOS. https://codereview.chromium.org/727593003/diff/100001/Source/web/WebInputEven... Source/web/WebInputEventConversion.cpp:99: if (e.modifiers & WebInputEvent::LeftButtonDown) On 2014/11/14 19:48:09, Rick Byers wrote: > Please extend the unit tests in WebInputEventConversionTest.cpp to cover a > couple buttons cases also. I don't think we may need it in new implementation.
Thanks, this looks close to being ready to land! https://codereview.chromium.org/727593003/diff/100001/LayoutTests/fast/events... File LayoutTests/fast/events/mouse-event-buttons-attribute.html (right): https://codereview.chromium.org/727593003/diff/100001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:8: window.addEventListener('mousedown', function() { On 2014/11/18 14:19:35, zino wrote: > On 2014/11/14 19:48:09, Rick Byers wrote: > > Please add handlers for the other event types too and test that they include > > correct buttons values. At least: > > - mousemove - both before and after a button is depressed > > - click, dblclick, mouseenter, mouseleave, mouseover and mouseout - all of > > which blink synthesizes on and will need to propagate the button state > > explicitly for > > Done. > > > > > Also can you manually verify on at least one chrome platform what the behavior > > is for mouseup? The spec doesn't seem entirely clear on this, but I assumed > it > > should be 0 when releasing a button. However testing Firefox on Linux I see > the > > 'buttons' value represents the state BEFORE the button is released - perhaps I > > missed the explanation for this in the spec? Obviously this is a > chromium-side > > bug/change, not directly applicable to this blink CL. > > > > Sorry, I couldn't find your mentioned part. > Could you let me know the explanation in the spec? The spec (http://www.w3.org/TR/DOM-Level-3-Events/#widl-MouseEvent-buttons) says that 'buttons' indicates which buttons "are currently being pressed". So what is the correct interpretation for a mouseup event where a button was just released? Eg. try clicking with the left mouse button at http://www.rbyers.net/eventTest.html on a couple browsers. In Firefox and IE on Windows I get: mousedown: ... button=0 buttons=1 ... mouseup: ... button=0 buttons=0 ... click: ... button=0 buttons=0 ... mousemove[28]: ... button=0 buttons=0 ... But in Firefox on Linux I get: mousedown: ... button=0 buttons=1 ... mouseup: ... button=0 buttons=1 ... click: ... button=0 buttons=1 ... mousemove[28]: ... button=0 buttons=0 ... The former seems correct according to the spec to me, the latter is probably a bug in Firefox. Which behavior does Chromium have? > Anyway, currently new implementation seems to have the same behavior. > > > Also you need a test for creating mouse events from javascript with non-zero > > buttons values. You still need this. Eg. 'var evt = new MouseEvent( {.... buttons: 2 }); shouldBeNumber("buttons", "evt.buttons"); > What should the behavior of 'buttons' be for MouseEvent > objects > > created with the legacy initMouseEvent API? Eg. if you create a mousedown > > event with that, is it OK for buttons to still be 0? The spec seems silent on > > this, please check what IE and/or FireFox do here. > > I'm not sure but the current spec doesn't seem to say that. > Firefox also doesn't support for them. > > Please see this: > https://hg.mozilla.org/mozilla-central/file/7913c9392c5f/dom/events/MouseEven... Yeah I think this is fine. I tried this on IE - see http://jsbin.com/tesopu/65/edit?js,output. On IE it displays: mousedown button=2 buttons=0 Technically this could confuse some code (buttons should never be 0 in a mousedown event), but it's probably not worth trying to do something about this. https://codereview.chromium.org/727593003/diff/100001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/727593003/diff/100001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:1643: 0, 0, nullptr, dataTransfer); On 2014/11/18 14:19:35, zino wrote: > On 2014/11/14 19:48:09, Rick Byers wrote: > > This looks like a bug too. DragEvent is a MouseEvent > > > (https://html.spec.whatwg.org/multipage/interaction.html#the-dragevent-interface), > > so technically the current mouse button state should be reflected there too. > > Again, feel free to add a FIXME and worry about this in a follow-up patch > (it's > > a bit of an edge case). I'd also be interested to know if FF and IE do this > > properly (if neither do, then it's probably a low priority bug for us too). > > I left a FIXME comment. This seems to be non-trivial. > Also, I've confirmed that FF works correctly. Ok. Thanks for adding the RuntimeEnabledFeature - I think that's a good idea (it's also a convenient way to be able to disable the feature without reverting your CLs if we uncover issues). Hopefully we can flip it to stable very soon (eg. before Jan 9 branch cut-off for M41). > (Sorry about IE, I don't have a > window PC..) No problem, comparison to Firefox is useful. It would be good to compare to one other major browser too though. Do you have a Mac (to compare with Safari)? Or if you have an Android or iOS device you could use https://remote.modern.ie to test IE. https://codereview.chromium.org/727593003/diff/100001/Source/web/WebInputEven... File Source/web/WebInputEventConversion.cpp (right): https://codereview.chromium.org/727593003/diff/100001/Source/web/WebInputEven... Source/web/WebInputEventConversion.cpp:99: if (e.modifiers & WebInputEvent::LeftButtonDown) On 2014/11/18 14:19:35, zino wrote: > On 2014/11/14 19:48:09, Rick Byers wrote: > > Oh cool, I didn't realize this data was already present in the WebInputEvent > > modifiers. Great! > > > > What platforms have (or will) you be able to validate that this is already set > > properly on? It looks like there's not much relying on these modifiers today, > > right? I'm just wondering if it might not be universally supported across > > platforms. That would be a chromium bug, and I'm OK landing the blink side > > before worrying too much about this but we shouldn't ship the API to beta > > without having validated all the platforms set the modifiers properly. > > Windows / Linux / Mac works well via layout tests on build bot. The behavior I'm worried about here isn't covered by the LayoutTest - it's whether chromium is setting the modifiers correctly. Arguably we should look for or add some test coverage in chromium for this. Can you please check manually on Linux that your behavior matches Firefox? > Android works well also. I've confirmed it manually. Great, thanks. Verifying Android and Linux is good enough for me, especially since you have the RuntimeEnabledFeature. Once it lands, I'll manually verify on Windows, Mac and ChromeOS canary builds before we flip the RuntimeEnabledFeature to stable. > I couldn't check it in IOS. iOS doesn't use blink so not relevant here. https://codereview.chromium.org/727593003/diff/100001/Source/web/WebInputEven... Source/web/WebInputEventConversion.cpp:99: if (e.modifiers & WebInputEvent::LeftButtonDown) On 2014/11/18 14:19:35, zino wrote: > On 2014/11/14 19:48:09, Rick Byers wrote: > > Please extend the unit tests in WebInputEventConversionTest.cpp to cover a > > couple buttons cases also. > > I don't think we may need it in new implementation. I guess your changes in WebInputConversion.cpp are covered by your layout test, so perhaps that's enough. Is that what you mean? WebInputConversionTest.cpp does add unit-test coverage for much of the logic in WebInputConversion.cpp. https://codereview.chromium.org/727593003/diff/140001/LayoutTests/fast/events... File LayoutTests/fast/events/mouse-event-buttons-attribute-expected.txt (right): https://codereview.chromium.org/727593003/diff/140001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute-expected.txt:1: MouseEvent::click test [leftButton,rightButton] this output looks great - thank you! https://codereview.chromium.org/727593003/diff/140001/LayoutTests/fast/events... File LayoutTests/fast/events/mouse-event-buttons-attribute.html (right): https://codereview.chromium.org/727593003/diff/140001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:2: <body> nit: remove <body> tag - see http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-h... https://codereview.chromium.org/727593003/diff/140001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:18: window.buttons = event.buttons; relying on the global 'event' object is discouraged I think (it's a little magical). Just add an argument to your function and use it instead. https://codereview.chromium.org/727593003/diff/140001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:35: add 'var buttons' to make it clear you're creating a new global variable. https://codereview.chromium.org/727593003/diff/140001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:44: document.body.insertBefore(div, document.body.firstChild); Is there a reason you want to create a new div for each test? It would be simpler to just define a single target div in HTML. https://codereview.chromium.org/727593003/diff/140001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:45: You should probably verify that your event handler is getting called at all (especially since you need to add a case where the expected value is 0 - see below). Perhaps set buttons to some illegal value (eg. 'null') right here? Then the fact that it's a number at all means the event handler got invoked (and also there's also no reason to reset it to 0 below). https://codereview.chromium.org/727593003/diff/140001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:55: shouldBe('buttons', expectedValue.toString()); why toString? Maybe use shouldBeEqualToNumber instead? https://codereview.chromium.org/727593003/diff/140001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:89: mouseTest('mousemove', [L], function(modifiers) { you should have one test where there are no modifiers - eg. for a mousemove event (to verify you get the 0 value). https://codereview.chromium.org/727593003/diff/140001/Source/platform/Platfor... File Source/platform/PlatformMouseEvent.h (right): https://codereview.chromium.org/727593003/diff/140001/Source/platform/Platfor... Source/platform/PlatformMouseEvent.h:98: unsigned short buttons() const And actually, it doesn't really make sense for PlatformMouseEvent to expose the buttons in two ways (via modifiers() and via buttons()) - especially when the values (1, 2, 4) of the latter are really defined by the DOM event spec (i.e. "4" has no definition here at the Platform layer - it's only within MouseEvent that it makes sense to reference the DOM events spec). So instead of modifying PlatformMouseEvent, please add a private function to MouseEvent which converts a PlatformEvent::Modifiers value into a 'buttons' value, and call it when creating MouseEvent objects from PlatformMouseEvent. Sorry I didn't suggest this earlier. https://codereview.chromium.org/727593003/diff/140001/Source/platform/Platfor... Source/platform/PlatformMouseEvent.h:101: return 1; This logic doesn't belong here, the caller should provide the correct modifiers for touch scenarios. In particular, for GestureLongPress we synthesize a contextmenu event, which is a MouseEvent where buttons should be 2 (right mouse button). Please add a test for contextmenu with both mouse and GestureLongPress input.
@Rick, Thank you for review and detailed explanation. I'll upload a new patch set soon. Thanks :)
Patchset #4 (id:160001) has been deleted
Thank you for review and sorry for delay.. https://codereview.chromium.org/727593003/diff/100001/LayoutTests/fast/events... File LayoutTests/fast/events/mouse-event-buttons-attribute.html (right): https://codereview.chromium.org/727593003/diff/100001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:8: window.addEventListener('mousedown', function() { On 2014/11/18 20:29:18, Rick Byers wrote: > The spec (http://www.w3.org/TR/DOM-Level-3-Events/#widl-MouseEvent-buttons) says > that 'buttons' indicates which buttons "are currently being pressed". So what > is the correct interpretation for a mouseup event where a button was just > released? Eg. try clicking with the left mouse button at > http://www.rbyers.net/eventTest.html on a couple browsers. In Firefox and IE on > Windows I get: > > mousedown: ... button=0 buttons=1 ... > mouseup: ... button=0 buttons=0 ... > click: ... button=0 buttons=0 ... > mousemove[28]: ... button=0 buttons=0 ... > > But in Firefox on Linux I get: > > mousedown: ... button=0 buttons=1 ... > mouseup: ... button=0 buttons=1 ... > click: ... button=0 buttons=1 ... > mousemove[28]: ... button=0 buttons=0 ... > > The former seems correct according to the spec to me, the latter is probably a > bug in Firefox. Which behavior does Chromium have? Okay, I've addressed the bug. > You still need this. Eg. 'var evt = new MouseEvent( {.... buttons: 2 }); > shouldBeNumber("buttons", "evt.buttons"); Done. https://codereview.chromium.org/727593003/diff/100001/Source/web/WebInputEven... File Source/web/WebInputEventConversion.cpp (right): https://codereview.chromium.org/727593003/diff/100001/Source/web/WebInputEven... Source/web/WebInputEventConversion.cpp:99: if (e.modifiers & WebInputEvent::LeftButtonDown) On 2014/11/18 20:29:18, Rick Byers wrote: > The behavior I'm worried about here isn't covered by the LayoutTest - it's > whether chromium is setting the modifiers correctly. Arguably we should look > for or add some test coverage in chromium for this. The modifiers value is passed from ui::EventFlagsFromNative() which is implemented on each platform. X(Chrome OS and Linux) and Cocoa(Mac) have already unit tests for it. - X : src/ui/events/x/events_x_unittest.cc - Cocoa : src/ui/events/cocoa/events_mac_unittest.mm Windows doesn't have a unit test but I'm not sure whether it is really necessary. (for |buttons| attribute) IMHO, we will have to do test manually to check whether each platform sets the modifiers correctly. If not, we will use simulated value instead of real value from system call and it will not be meaningful. (I mean eventSender is enough.) For example, please see EventFlagsFromNative test in events_mac_unittest.mm https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/cocoa/ev... According to the codes, using cocoa_test_event_utils::MouseEventWithType() to make a native mouse event and then checking its value via EventFlagsFromNative(). However, we know that EventFlagsFromNative() returns value which we passed to MouseEventWithType(). In order words, the value is simulated value. (not real value) Therefore I think we are enough as eventSender and layout test. > > Can you please check manually on Linux that your behavior matches Firefox? > Yep, I had already checked. But there is a bit of difference between new patch set for chromium and firefox. Because I've addressed mouse-up case per your comment. Other cases are the same. https://codereview.chromium.org/727593003/diff/140001/LayoutTests/fast/events... File LayoutTests/fast/events/mouse-event-buttons-attribute.html (right): https://codereview.chromium.org/727593003/diff/140001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:2: <body> On 2014/11/18 20:29:19, Rick Byers wrote: > nit: remove <body> tag - see > http://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-h... Done. https://codereview.chromium.org/727593003/diff/140001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:18: window.buttons = event.buttons; On 2014/11/18 20:29:18, Rick Byers wrote: > relying on the global 'event' object is discouraged I think (it's a little > magical). Just add an argument to your function and use it instead. Done. https://codereview.chromium.org/727593003/diff/140001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:35: On 2014/11/18 20:29:19, Rick Byers wrote: > add 'var buttons' to make it clear you're creating a new global variable. Done. https://codereview.chromium.org/727593003/diff/140001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:44: document.body.insertBefore(div, document.body.firstChild); On 2014/11/18 20:29:18, Rick Byers wrote: > Is there a reason you want to create a new div for each test? It would be > simpler to just define a single target div in HTML. Done. I'm not sure but shouldXXXX series in test.js seems to have some bug when using eventSender or event listener. https://codereview.chromium.org/727593003/diff/140001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:45: On 2014/11/18 20:29:19, Rick Byers wrote: > You should probably verify that your event handler is getting called at all > (especially since you need to add a case where the expected value is 0 - see > below). Perhaps set buttons to some illegal value (eg. 'null') right here? > Then the fact that it's a number at all means the event handler got invoked (and > also there's also no reason to reset it to 0 below). Done. https://codereview.chromium.org/727593003/diff/140001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:55: shouldBe('buttons', expectedValue.toString()); On 2014/11/18 20:29:19, Rick Byers wrote: > why toString? Maybe use shouldBeEqualToNumber instead? Done. Oh, I didn't know that. https://codereview.chromium.org/727593003/diff/140001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:89: mouseTest('mousemove', [L], function(modifiers) { On 2014/11/18 20:29:19, Rick Byers wrote: > you should have one test where there are no modifiers - eg. for a mousemove > event (to verify you get the 0 value). Done. https://codereview.chromium.org/727593003/diff/140001/Source/platform/Platfor... File Source/platform/PlatformMouseEvent.h (right): https://codereview.chromium.org/727593003/diff/140001/Source/platform/Platfor... Source/platform/PlatformMouseEvent.h:98: unsigned short buttons() const On 2014/11/18 20:29:19, Rick Byers wrote: > And actually, it doesn't really make sense for PlatformMouseEvent to expose the > buttons in two ways (via modifiers() and via buttons()) - especially when the > values (1, 2, 4) of the latter are really defined by the DOM event spec (i.e. > "4" has no definition here at the Platform layer - it's only within MouseEvent > that it makes sense to reference the DOM events spec). > > So instead of modifying PlatformMouseEvent, please add a private function to > MouseEvent which converts a PlatformEvent::Modifiers value into a 'buttons' > value, and call it when creating MouseEvent objects from PlatformMouseEvent. > > Sorry I didn't suggest this earlier. Done. https://codereview.chromium.org/727593003/diff/140001/Source/platform/Platfor... Source/platform/PlatformMouseEvent.h:101: return 1; On 2014/11/18 20:29:19, Rick Byers wrote: > This logic doesn't belong here, the caller should provide the correct modifiers > for touch scenarios. In particular, for GestureLongPress we synthesize a > contextmenu event, which is a MouseEvent where buttons should be 2 (right mouse > button). Please add a test for contextmenu with both mouse and GestureLongPress > input. Done.
This is looking create, but the changes raise a few other issues (sorry for the back and forth, the remaining issues should be easy to address I think). Rick https://codereview.chromium.org/727593003/diff/100001/LayoutTests/fast/events... File LayoutTests/fast/events/mouse-event-buttons-attribute.html (right): https://codereview.chromium.org/727593003/diff/100001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:8: window.addEventListener('mousedown', function() { On 2014/11/24 13:07:07, zino wrote: > On 2014/11/18 20:29:18, Rick Byers wrote: > > The spec (http://www.w3.org/TR/DOM-Level-3-Events/#widl-MouseEvent-buttons) > says > > that 'buttons' indicates which buttons "are currently being pressed". So what > > is the correct interpretation for a mouseup event where a button was just > > released? Eg. try clicking with the left mouse button at > > http://www.rbyers.net/eventTest.html on a couple browsers. In Firefox and IE > on > > Windows I get: > > > > mousedown: ... button=0 buttons=1 ... > > mouseup: ... button=0 buttons=0 ... > > click: ... button=0 buttons=0 ... > > mousemove[28]: ... button=0 buttons=0 ... > > > > But in Firefox on Linux I get: > > > > mousedown: ... button=0 buttons=1 ... > > mouseup: ... button=0 buttons=1 ... > > click: ... button=0 buttons=1 ... > > mousemove[28]: ... button=0 buttons=0 ... > > > > The former seems correct according to the spec to me, the latter is probably a > > bug in Firefox. Which behavior does Chromium have? > > Okay, I've addressed the bug. I was thinking this would be addressed in chromium, but adding additional protection in blink (so we're not sensitive to which way chromium behaves here) makes sense. Thanks! https://codereview.chromium.org/727593003/diff/100001/Source/web/WebInputEven... File Source/web/WebInputEventConversion.cpp (right): https://codereview.chromium.org/727593003/diff/100001/Source/web/WebInputEven... Source/web/WebInputEventConversion.cpp:99: if (e.modifiers & WebInputEvent::LeftButtonDown) On 2014/11/24 13:07:07, zino wrote: > On 2014/11/18 20:29:18, Rick Byers wrote: > > The behavior I'm worried about here isn't covered by the LayoutTest - it's > > whether chromium is setting the modifiers correctly. Arguably we should look > > for or add some test coverage in chromium for this. > > The modifiers value is passed from ui::EventFlagsFromNative() which is > implemented on each platform. > X(Chrome OS and Linux) and Cocoa(Mac) have already unit tests for it. > - X : src/ui/events/x/events_x_unittest.cc > - Cocoa : src/ui/events/cocoa/events_mac_unittest.mm > > Windows doesn't have a unit test but I'm not sure whether it is really > necessary. (for |buttons| attribute) > IMHO, we will have to do test manually to check whether each platform sets the > modifiers correctly. > If not, we will use simulated value instead of real value from system call and > it will not be meaningful. > (I mean eventSender is enough.) > > For example, please see EventFlagsFromNative test in events_mac_unittest.mm > https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/cocoa/ev... > > According to the codes, using cocoa_test_event_utils::MouseEventWithType() to > make a native mouse event and then checking its value via > EventFlagsFromNative(). > However, we know that EventFlagsFromNative() returns value which we passed to > MouseEventWithType(). In order words, the value is simulated value. (not real > value) > Therefore I think we are enough as eventSender and layout test. > > > > > Can you please check manually on Linux that your behavior matches Firefox? > > Sounds good, thanks for digging into this! > Yep, I had already checked. > But there is a bit of difference between new patch set for chromium and firefox. > Because I've addressed mouse-up case per your comment. Other cases are the same. Perfect, thanks! https://codereview.chromium.org/727593003/diff/140001/LayoutTests/fast/events... File LayoutTests/fast/events/mouse-event-buttons-attribute.html (right): https://codereview.chromium.org/727593003/diff/140001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:44: document.body.insertBefore(div, document.body.firstChild); On 2014/11/24 13:07:07, zino wrote: > On 2014/11/18 20:29:18, Rick Byers wrote: > > Is there a reason you want to create a new div for each test? It would be > > simpler to just define a single target div in HTML. > > Done. > > I'm not sure but shouldXXXX series in test.js seems to have some bug when using > eventSender or event listener. Perhaps what you were seeing was that the console by default gets created ABOVE your test div, so as shouldBe is generating output, the location of your div is changing so that it's no longer under the co-ordinates you've created. I've dealt with this in the past by explicitly adding a <div id=console></div> tag below my test element, or making my test element position:absolute over at the right side of the document. If you do either of those, then you should be able to combine your two loops into one again. https://codereview.chromium.org/727593003/diff/140001/Source/platform/Platfor... File Source/platform/PlatformMouseEvent.h (right): https://codereview.chromium.org/727593003/diff/140001/Source/platform/Platfor... Source/platform/PlatformMouseEvent.h:101: return 1; On 2014/11/24 13:07:08, zino wrote: > On 2014/11/18 20:29:19, Rick Byers wrote: > > This logic doesn't belong here, the caller should provide the correct > modifiers > > for touch scenarios. In particular, for GestureLongPress we synthesize a > > contextmenu event, which is a MouseEvent where buttons should be 2 (right > mouse > > button). Please add a test for contextmenu with both mouse and > GestureLongPress > > input. > > Done. I still don't see why we should change the PlatformMouseEvent here for 'fromTouch' cases instead of just fixing the callers to supply the right modifiers? I think there are just three callers to be trivially updated: EventHandler::HandleGestureTap, EventHandler::HandleGestureLongPress and EventHandler::sendContextMenuEventForGesture. That way PlatformMouseEvent will reflect exactly the modifiers it's been given without mucking with them in any way (we just adjust on the way in and way out as necessary). https://codereview.chromium.org/727593003/diff/180001/LayoutTests/fast/events... File LayoutTests/fast/events/constructors/mouse-event-constructor.html (right): https://codereview.chromium.org/727593003/diff/180001/LayoutTests/fast/events... LayoutTests/fast/events/constructors/mouse-event-constructor.html:146: shouldBe("new MouseEvent('eventType', { buttons: -1 }).buttons", "0"); what about other negative numbers? You special-cased -1, right? Should all negative values get converted to 0 (like other invalid values)? https://codereview.chromium.org/727593003/diff/180001/LayoutTests/fast/events... File LayoutTests/fast/events/mouse-event-buttons-attribute-expected.txt (right): https://codereview.chromium.org/727593003/diff/180001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute-expected.txt:2: PASS testSet[i].buttons is 1 this should be 0 (like mouseup). https://codereview.chromium.org/727593003/diff/180001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute-expected.txt:5: PASS testSet[i].buttons is 3 and this should be 2. https://codereview.chromium.org/727593003/diff/180001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute-expected.txt:32: PASS testSet[i].buttons is 2 Another example where I think this should be 0, not 2. I confirmed IE and Firefox on Windows. https://codereview.chromium.org/727593003/diff/180001/LayoutTests/fast/events... File LayoutTests/fast/events/mouse-event-buttons-attribute.html (right): https://codereview.chromium.org/727593003/diff/180001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:22: { type: ME, name: 'click', modifiers: [L, R], action: clickAction }, you're doing a click with the left mouse button, I think some or all of the modifiers should be cleared (since after the click, the button that was clicked is no longer down). https://codereview.chromium.org/727593003/diff/180001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:75: eventSender.mouseDown(0, modifiersDown); You should probably be testing the code you added to clear the modifier on mouseup. Rather than take a separate 'modifiersDown' how about you update your table to optionally supply a different 'expectedModifiers' array that omits the ones you expect to be cleared? https://codereview.chromium.org/727593003/diff/180001/Source/core/events/Mous... File Source/core/events/MouseEvent.cpp (right): https://codereview.chromium.org/727593003/diff/180001/Source/core/events/Mous... Source/core/events/MouseEvent.cpp:134: if (type == EventTypeNames::mouseup) I don't think checking for mouseup here is sufficient, this logic also applies to other events triggered as a result of mouseup (eg. click, dblclick). I suggest you do the modifier clearing stuff sooner on the PlatformMouseEvent - eg. at the top of EventHandler::handleMouseReleaseEvent (but that would require you to make a copy you can modify) or maybe best in PlatformMouseEventBuilder::PlatformMouseEventBuilder. Also please add a comment since this is a little subtle. Eg. something like: // The MouseEvent spec requires that buttons indicates the state immediately after the event takes place. To ensure consistency between platforms here, we explicitly clear the button that is in the process of being released. https://codereview.chromium.org/727593003/diff/200001/Source/core/events/Mous... File Source/core/events/MouseEvent.h (right): https://codereview.chromium.org/727593003/diff/200001/Source/core/events/Mous... Source/core/events/MouseEvent.h:63: bool ctrlKey, bool altKey, bool shiftKey, bool metaKey, unsigned short button, unsigned modifiers, I don't think you want to require all callers of 'create' to use PlatformEvent's notion of 'modifiers'. For one, this makes the 'modifiers' argument redundant with ctrlKey, altKey, etc. But it's a bit of a layering problem for code that's not using PlatformEvent at all (eg. synthetic events). What I was thinking was that you'd leave all the constructor/create signatures the same as you originally had them (eg. taking a 'buttons'). But change the implementation in the couple places we create a MouseEvent from a PlatformMouseEvent to also do the conversion from modifiers to buttons - similar to how it already converts from modifiers to ctrlKey, altKey, etc. I think this is just two places: MouseEvent::create(..., PlatformMouseEvent, ...) and WheelEventDispatchMediator::WheelEventDispatchMediator(const PlatformWheelEvent& event, ...). To avoid code duplication across these two places, you could add a protected method on MouseEvent called 'platformEventModifiersToButtons' which you could call from both places (if there's other uses, it's also fine for this to be public). Sorry I didn't explain this more thoroughly. Does it make sense?
Hi Rick, Thank you for detailed explanation and inputs. I have a question for addressing your comments. While I was making a new patch, I felt a little confused. If the |contextmenu| event is raised, Your first comment said |buttons| attribute should be 2. But your second comment said |buttons| attribute should be 0. (not 2) I also confirmed Firefox on Linux and the value was 2. (not 0) BTW, My result seem to be different from your result. (You said that you already checked it on Windows) Is the difference due to the difference between platforms? Which is correct? https://codereview.chromium.org/727593003/diff/140001/Source/platform/Platfor... File Source/platform/PlatformMouseEvent.h (right): https://codereview.chromium.org/727593003/diff/140001/Source/platform/Platfor... Source/platform/PlatformMouseEvent.h:101: return 1; On 2014/11/18 20:29:19, Rick Byers wrote: > This logic doesn't belong here, the caller should provide the correct modifiers > for touch scenarios. In particular, for GestureLongPress we synthesize a > contextmenu event, which is a MouseEvent where buttons should be 2 (right mouse > button). Please add a test for contextmenu with both mouse and GestureLongPress > input. Here https://codereview.chromium.org/727593003/diff/180001/LayoutTests/fast/events... File LayoutTests/fast/events/mouse-event-buttons-attribute-expected.txt (right): https://codereview.chromium.org/727593003/diff/180001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute-expected.txt:32: PASS testSet[i].buttons is 2 On 2014/11/25 17:44:27, Rick Byers wrote: > Another example where I think this should be 0, not 2. I confirmed IE and > Firefox on Windows. Here.
Hi Rick, I've just uploaded two patch sets. patch set 6 : if the contextmenu event happen, buttons will take 2. patch set 7 : if the contextmenu event happen, buttons will take 0. Could you please review again? https://codereview.chromium.org/727593003/diff/140001/LayoutTests/fast/events... File LayoutTests/fast/events/mouse-event-buttons-attribute.html (right): https://codereview.chromium.org/727593003/diff/140001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:44: document.body.insertBefore(div, document.body.firstChild); On 2014/11/25 17:44:27, Rick Byers wrote: > On 2014/11/24 13:07:07, zino wrote: > > On 2014/11/18 20:29:18, Rick Byers wrote: > > > Is there a reason you want to create a new div for each test? It would be > > > simpler to just define a single target div in HTML. > > > > Done. > > > > I'm not sure but shouldXXXX series in test.js seems to have some bug when > using > > eventSender or event listener. > > Perhaps what you were seeing was that the console by default gets created ABOVE > your test div, so as shouldBe is generating output, the location of your div is > changing so that it's no longer under the co-ordinates you've created. I've > dealt with this in the past by explicitly adding a <div id=console></div> tag > below my test element, or making my test element position:absolute over at the > right side of the document. > > If you do either of those, then you should be able to combine your two loops > into one again. Done. https://codereview.chromium.org/727593003/diff/140001/Source/platform/Platfor... File Source/platform/PlatformMouseEvent.h (right): https://codereview.chromium.org/727593003/diff/140001/Source/platform/Platfor... Source/platform/PlatformMouseEvent.h:101: return 1; On 2014/11/25 17:44:27, Rick Byers wrote: > I still don't see why we should change the PlatformMouseEvent here for > 'fromTouch' cases instead of just fixing the callers to supply the right > modifiers? I think there are just three callers to be trivially updated: > EventHandler::HandleGestureTap, EventHandler::HandleGestureLongPress and > EventHandler::sendContextMenuEventForGesture. That way PlatformMouseEvent will > reflect exactly the modifiers it's been given without mucking with them in any > way (we just adjust on the way in and way out as necessary). Done https://codereview.chromium.org/727593003/diff/180001/LayoutTests/fast/events... File LayoutTests/fast/events/constructors/mouse-event-constructor.html (right): https://codereview.chromium.org/727593003/diff/180001/LayoutTests/fast/events... LayoutTests/fast/events/constructors/mouse-event-constructor.html:146: shouldBe("new MouseEvent('eventType', { buttons: -1 }).buttons", "0"); On 2014/11/25 17:44:27, Rick Byers wrote: > what about other negative numbers? You special-cased -1, right? Should all > negative values get converted to 0 (like other invalid values)? Sorry, My thinking was incorrect. If the button is -1, it should be 65535. (This is not [Clamp] attribute.) I also confirmed Firefox on Linux. https://codereview.chromium.org/727593003/diff/180001/LayoutTests/fast/events... File LayoutTests/fast/events/mouse-event-buttons-attribute-expected.txt (right): https://codereview.chromium.org/727593003/diff/180001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute-expected.txt:2: PASS testSet[i].buttons is 1 On 2014/11/25 17:44:27, Rick Byers wrote: > this should be 0 (like mouseup). Done. https://codereview.chromium.org/727593003/diff/180001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute-expected.txt:5: PASS testSet[i].buttons is 3 On 2014/11/25 17:44:27, Rick Byers wrote: > and this should be 2. Done. https://codereview.chromium.org/727593003/diff/180001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute-expected.txt:32: PASS testSet[i].buttons is 2 On 2014/11/25 17:44:27, Rick Byers wrote: > Another example where I think this should be 0, not 2. I confirmed IE and > Firefox on Windows. If this is correct, you can see the patch set 7. But if the value should be 2, you will see the patch set 6. https://codereview.chromium.org/727593003/diff/180001/LayoutTests/fast/events... File LayoutTests/fast/events/mouse-event-buttons-attribute.html (right): https://codereview.chromium.org/727593003/diff/180001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:22: { type: ME, name: 'click', modifiers: [L, R], action: clickAction }, On 2014/11/25 17:44:27, Rick Byers wrote: > you're doing a click with the left mouse button, I think some or all of the > modifiers should be cleared (since after the click, the button that was clicked > is no longer down). Done. https://codereview.chromium.org/727593003/diff/180001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:75: eventSender.mouseDown(0, modifiersDown); On 2014/11/25 17:44:28, Rick Byers wrote: > You should probably be testing the code you added to clear the modifier on > mouseup. Rather than take a separate 'modifiersDown' how about you update your > table to optionally supply a different 'expectedModifiers' array that omits the > ones you expect to be cleared? Done. https://codereview.chromium.org/727593003/diff/180001/Source/core/events/Mous... File Source/core/events/MouseEvent.cpp (right): https://codereview.chromium.org/727593003/diff/180001/Source/core/events/Mous... Source/core/events/MouseEvent.cpp:134: if (type == EventTypeNames::mouseup) On 2014/11/25 17:44:28, Rick Byers wrote: > I don't think checking for mouseup here is sufficient, this logic also applies > to other events triggered as a result of mouseup (eg. click, dblclick). I > suggest you do the modifier clearing stuff sooner on the PlatformMouseEvent - > eg. at the top of EventHandler::handleMouseReleaseEvent (but that would require > you to make a copy you can modify) or maybe best in > PlatformMouseEventBuilder::PlatformMouseEventBuilder. > > Also please add a comment since this is a little subtle. Eg. something like: > // The MouseEvent spec requires that buttons indicates the state immediately > after the event takes place. To ensure consistency between platforms here, we > explicitly clear the button that is in the process of being released. Done. https://codereview.chromium.org/727593003/diff/200001/Source/core/events/Mous... File Source/core/events/MouseEvent.h (right): https://codereview.chromium.org/727593003/diff/200001/Source/core/events/Mous... Source/core/events/MouseEvent.h:63: bool ctrlKey, bool altKey, bool shiftKey, bool metaKey, unsigned short button, unsigned modifiers, On 2014/11/25 17:44:28, Rick Byers wrote: > I don't think you want to require all callers of 'create' to use PlatformEvent's > notion of 'modifiers'. For one, this makes the 'modifiers' argument redundant > with ctrlKey, altKey, etc. But it's a bit of a layering problem for code that's > not using PlatformEvent at all (eg. synthetic events). > > What I was thinking was that you'd leave all the constructor/create signatures > the same as you originally had them (eg. taking a 'buttons'). But change the > implementation in the couple places we create a MouseEvent from a > PlatformMouseEvent to also do the conversion from modifiers to buttons - similar > to how it already converts from modifiers to ctrlKey, altKey, etc. I think this > is just two places: MouseEvent::create(..., PlatformMouseEvent, ...) and > WheelEventDispatchMediator::WheelEventDispatchMediator(const PlatformWheelEvent& > event, ...). > > To avoid code duplication across these two places, you could add a protected > method on MouseEvent called 'platformEventModifiersToButtons' which you could > call from both places (if there's other uses, it's also fine for this to be > public). > > Sorry I didn't explain this more thoroughly. Does it make sense? Done.
On 2014/11/28 02:37:09, zino wrote: > Hi Rick, > > Thank you for detailed explanation and inputs. > > I have a question for addressing your comments. > > While I was making a new patch, I felt a little confused. > If the |contextmenu| event is raised, > Your first comment said |buttons| attribute should be 2. > But your second comment said |buttons| attribute should be 0. (not 2) Sorry for the confusion! > I also confirmed Firefox on Linux and the value was 2. (not 0) > BTW, My result seem to be different from your result. > (You said that you already checked it on Windows) > Is the difference due to the difference between platforms? > > Which is correct? The principle to apply here is that buttons should represent the state including the change described by the event. So for mouseup this means buttons=0 is correct and Firefox on Linux has a bug (which mozilla blames on GTK). I was thinking contextmenu always comes after mouseup (and so buttons must also be 0), but I just realized that's not necessarily correct. On windows the convention is to show the contextmenu after the RELEASE of the right mouse button (and indeed this is what Chrome, Firefox and IE all do on Windows), so buttons=0 is correct there. But on Linux the convention is to show the context menu after the button goes down (before any mouseup event), so buttons=2 is correct there. Hopefully you can mostly ignore this in your patch. The thing that matters for the purpose of your patch is that when a mouse release event first comes into blink, you consistently set the buttons value to exclude the lifting buttin for all events derived from that PlatformMouseEvent. Presumably then we'll do the right thing for contextmenu depending on whether it's triggered by MousePressed or MouseReleased. > > https://codereview.chromium.org/727593003/diff/140001/Source/platform/Platfor... > File Source/platform/PlatformMouseEvent.h (right): > > https://codereview.chromium.org/727593003/diff/140001/Source/platform/Platfor... > Source/platform/PlatformMouseEvent.h:101: return 1; > On 2014/11/18 20:29:19, Rick Byers wrote: > > This logic doesn't belong here, the caller should provide the correct > modifiers > > for touch scenarios. In particular, for GestureLongPress we synthesize a > > contextmenu event, which is a MouseEvent where buttons should be 2 (right > mouse > > button). Please add a test for contextmenu with both mouse and > GestureLongPress > > input. > > Here > > https://codereview.chromium.org/727593003/diff/180001/LayoutTests/fast/events... > File LayoutTests/fast/events/mouse-event-buttons-attribute-expected.txt (right): > > https://codereview.chromium.org/727593003/diff/180001/LayoutTests/fast/events... > LayoutTests/fast/events/mouse-event-buttons-attribute-expected.txt:32: PASS > testSet[i].buttons is 2 > On 2014/11/25 17:44:27, Rick Byers wrote: > > Another example where I think this should be 0, not 2. I confirmed IE and > > Firefox on Windows. > > Here.
This is looking great to me, thank you! Just a couple minor things left. https://codereview.chromium.org/727593003/diff/180001/LayoutTests/fast/events... File LayoutTests/fast/events/constructors/mouse-event-constructor.html (right): https://codereview.chromium.org/727593003/diff/180001/LayoutTests/fast/events... LayoutTests/fast/events/constructors/mouse-event-constructor.html:146: shouldBe("new MouseEvent('eventType', { buttons: -1 }).buttons", "0"); On 2014/11/28 12:29:28, zino wrote: > On 2014/11/25 17:44:27, Rick Byers wrote: > > what about other negative numbers? You special-cased -1, right? Should all > > negative values get converted to 0 (like other invalid values)? > > Sorry, My thinking was incorrect. > If the button is -1, it should be 65535. (This is not [Clamp] attribute.) > I also confirmed Firefox on Linux. Ok, thanks for checking - I'll take your word on whatever you believe the correct behavior is here :-) https://codereview.chromium.org/727593003/diff/240001/LayoutTests/fast/events... File LayoutTests/fast/events/mouse-event-buttons-attribute.html (right): https://codereview.chromium.org/727593003/diff/240001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:38: { type: GE, name: 'contextmenu', modifiers: [R], expectedModifiers: [], action: longTapAction }, please add a longTapAction 'mousedown' test (since that appears broken at the moment). https://codereview.chromium.org/727593003/diff/240001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:76: eventSender.mouseUp(1, modifiersUp); This says to press the left mouse button (0) then raise the middle mouse button (1). That's pretty confusing - maybe just pass '0' instead of '1' here so you're pressing and releasing the same mouse button? https://codereview.chromium.org/727593003/diff/240001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:92: eventSender.gestureLongPress(50, 50); This is a little confusing. LongPress and LongTap are actually two different gestures. LongPress is what happens when you hold your finger (eg. can trigger drang and drop on ChromeOS), and LongTap is what happens after holding your finger and lifting (eg. can trigger a context menu). You need two different actions to test both of them. https://codereview.chromium.org/727593003/diff/240001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:106: testSet[n].action(testSet[n].modifiers, testSet[n].expectedModifiers); you shouldn't have to pass your 'expected modifiers' to the action. The only difference between your desired modifiers and expected modifiers should be the clearing you do for MouseReleased. https://codereview.chromium.org/727593003/diff/240001/Source/core/events/Mous... File Source/core/events/MouseEvent.cpp (right): https://codereview.chromium.org/727593003/diff/240001/Source/core/events/Mous... Source/core/events/MouseEvent.cpp:62: if (eventType != EventTypeNames::contextmenu) why do you need to special case contextmenu here? I don't think it should have to be special in any way (it's just like click as an event that might be generated as part of a MouseReleased platform event). https://codereview.chromium.org/727593003/diff/240001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/727593003/diff/240001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:2229: static PlatformEvent::Modifiers platformModifiersFrom(unsigned modifierFlags) nit: this function is simple enough you should just write the cast inline rather than use a helper function. https://codereview.chromium.org/727593003/diff/240001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:2234: static unsigned modifierFlagsFrom(const PlatformGestureEvent& event) Isn't this function pretty much equivalent to just 'return event.modifiers()'? You should just be able to copy the modifiers directly without needing this function at all, right? I'm not sure why the original code was doing this - maybe just old... https://codereview.chromium.org/727593003/diff/240001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:2855: PlatformEvent::Type eventType = PlatformEvent::MousePressed; Looks like I was wrong and this OS behavior difference will effect your CL for the gesture long tap case only. You need to add RightButtonDown to the modifiers for the MousePressed (non-windows) case. Sadly I guess this means you'll need a platform-specific expectations file for windows (and need to detect windows from your layout test to change your expected button for this case). Really the right fix here is to add a WebPreference for 'show contextmenu on mouseup' which chrome sets to true only on Windows (then we could test both in the same layout tests on all platforms). This might be a nice follow-up CL if you're interested. If you want to do this, then feel free to disable the test on Windows for now against that bug (or better - just land an expectation that has a FAIL in it), rather than trying to detect Windows and making it pass cleanly... https://codereview.chromium.org/727593003/diff/240001/Source/platform/Platfor... File Source/platform/PlatformEvent.h (right): https://codereview.chromium.org/727593003/diff/240001/Source/platform/Platfor... Source/platform/PlatformEvent.h:84: RightButtonDown = 1 << 8, By the way, with the addition of these, I believe it should now be possible to remove the (redundant and confusing) PlatformMouseEvent::m_modifierFlags. Can you please add a FIXME there saying it's redundant and should be removed? Let me know if you're interested in that CL too - otherwise I can do it (I _think_ it should be easy, but I haven't traced all the code to verify).
On 2014/11/28 17:36:02, Rick Byers wrote: > https://codereview.chromium.org/727593003/diff/240001/Source/core/page/EventH... > Source/core/page/EventHandler.cpp:2855: PlatformEvent::Type eventType = > PlatformEvent::MousePressed; > Looks like I was wrong and this OS behavior difference will effect your CL for > the gesture long tap case only. You need to add RightButtonDown to the > modifiers for the MousePressed (non-windows) case. > > Sadly I guess this means you'll need a platform-specific expectations file for > windows (and need to detect windows from your layout test to change your > expected button for this case). Really the right fix here is to add a > WebPreference for 'show contextmenu on mouseup' which chrome sets to true only > on Windows (then we could test both in the same layout tests on all platforms). > This might be a nice follow-up CL if you're interested. If you want to do this, > then feel free to disable the test on Windows for now against that bug (or > better - just land an expectation that has a FAIL in it), rather than trying to > detect Windows and making it pass cleanly... > Dear Rick, Thank you for review! I have a question about your comment. If I fix as per your comment, is the following way correct? For example.. /* We should have only one layout test */ if (internals.settings.enabledShowContextMenuOnMouseUp()) shouldBe("buttons", "0"); // windows else shouldBe("buttons", "2"); // other platforms /* We should have two expectations */ platform/win/expected.txt contextmenu.. buttons should be 0. other-platforms/expected.txt contextmenu.. buttons should be 2. Sorry for my poor understanding..
On 2014/12/01 17:13:20, zino wrote: > On 2014/11/28 17:36:02, Rick Byers wrote: > > > https://codereview.chromium.org/727593003/diff/240001/Source/core/page/EventH... > > Source/core/page/EventHandler.cpp:2855: PlatformEvent::Type eventType = > > PlatformEvent::MousePressed; > > Looks like I was wrong and this OS behavior difference will effect your CL for > > the gesture long tap case only. You need to add RightButtonDown to the > > modifiers for the MousePressed (non-windows) case. > > > > Sadly I guess this means you'll need a platform-specific expectations file for > > windows (and need to detect windows from your layout test to change your > > expected button for this case). Really the right fix here is to add a > > WebPreference for 'show contextmenu on mouseup' which chrome sets to true only > > on Windows (then we could test both in the same layout tests on all > platforms). > > This might be a nice follow-up CL if you're interested. If you want to do > this, > > then feel free to disable the test on Windows for now against that bug (or > > better - just land an expectation that has a FAIL in it), rather than trying > to > > detect Windows and making it pass cleanly... > > > > Dear Rick, > > Thank you for review! > > I have a question about your comment. > If I fix as per your comment, is the following way correct? > > For example.. > > /* We should have only one layout test */ > if (internals.settings.enabledShowContextMenuOnMouseUp()) > shouldBe("buttons", "0"); // windows > else > shouldBe("buttons", "2"); // other platforms > > /* We should have two expectations */ > platform/win/expected.txt > contextmenu.. buttons should be 0. > > other-platforms/expected.txt > contextmenu.. buttons should be 2. > > Sorry for my poor understanding.. No problem. No, this would still be a problem since the output would be different. Instead we want to insulate blink to different default settings Chrome might pick on different platforms by testing both. Eg maybe: // Test Linux style context menus: { type: GE, name: 'contextmenu', modifiers: [R], action: longTapAction, showContextMenuOnMouseUp: false }, // Test windows style context menus: { type: GE, name: 'contextmenu', modifiers: [R], expectedModifiers: [], action: longTapAction, showContextMenuOnMouseUp: true }, // Then something like this: if ('showContextMenuOnMouseUp' in testSet[i]) internals.settings.setShowContextMenuOnMouseUp(testSet[i].showContextMenuOnMouseUp) Adding this setting is a bit of additional work however - probably belongs in it's own CL (and needs chrome to change to set it to true only on Windows). In fact it's probably 3 CLs due to the blink/chromium repo split: 1) add the setting to blink but don't use it 2) then change chrome to set it 3) then change blink to use the value of the setting instead of the #ifdef. Feel free to hack around this for now in any of the ways we've discussed, or if you prefer land such a set of CLs first.
Patchset #8 (id:260001) has been deleted
Patchset #8 (id:280001) has been deleted
Patchset #8 (id:300001) has been deleted
Patchset #8 (id:320001) has been deleted
Patchset #8 (id:340001) has been deleted
Patchset #8 (id:360001) has been deleted
Dear @Rick, Could you review again? https://codereview.chromium.org/727593003/diff/240001/LayoutTests/fast/events... File LayoutTests/fast/events/mouse-event-buttons-attribute.html (right): https://codereview.chromium.org/727593003/diff/240001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:38: { type: GE, name: 'contextmenu', modifiers: [R], expectedModifiers: [], action: longTapAction }, On 2014/11/28 17:36:02, Rick Byers wrote: > please add a longTapAction 'mousedown' test (since that appears broken at the > moment). Done. https://codereview.chromium.org/727593003/diff/240001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:76: eventSender.mouseUp(1, modifiersUp); On 2014/11/28 17:36:02, Rick Byers wrote: > This says to press the left mouse button (0) then raise the middle mouse button > (1). That's pretty confusing - maybe just pass '0' instead of '1' here so you're > pressing and releasing the same mouse button? Done. https://codereview.chromium.org/727593003/diff/240001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:92: eventSender.gestureLongPress(50, 50); On 2014/11/28 17:36:02, Rick Byers wrote: > This is a little confusing. LongPress and LongTap are actually two different > gestures. LongPress is what happens when you hold your finger (eg. can trigger > drang and drop on ChromeOS), and LongTap is what happens after holding your > finger and lifting (eg. can trigger a context menu). You need two different > actions to test both of them. Thank you for the information. I added contextmenu cases for two gestures fist. But we still need dragNdrop cases for distinction between LongTap and LongPress. I was thinking I was able to add the tests in the follow-up CL for dragNdrop because this CL ignores the cases for now. (I've already left a FIXME comment for dragNdrop) I was wondering what you think about that. https://codereview.chromium.org/727593003/diff/240001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:106: testSet[n].action(testSet[n].modifiers, testSet[n].expectedModifiers); On 2014/11/28 17:36:02, Rick Byers wrote: > you shouldn't have to pass your 'expected modifiers' to the action. The only > difference between your desired modifiers and expected modifiers should be the > clearing you do for MouseReleased. Done. https://codereview.chromium.org/727593003/diff/240001/Source/core/events/Mous... File Source/core/events/MouseEvent.cpp (right): https://codereview.chromium.org/727593003/diff/240001/Source/core/events/Mous... Source/core/events/MouseEvent.cpp:62: if (eventType != EventTypeNames::contextmenu) On 2014/11/28 17:36:02, Rick Byers wrote: > why do you need to special case contextmenu here? I don't think it should have > to be special in any way (it's just like click as an event that might be > generated as part of a MouseReleased platform event). This was for asking question to you. Done. https://codereview.chromium.org/727593003/diff/240001/Source/core/page/EventH... File Source/core/page/EventHandler.cpp (right): https://codereview.chromium.org/727593003/diff/240001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:2229: static PlatformEvent::Modifiers platformModifiersFrom(unsigned modifierFlags) On 2014/11/28 17:36:02, Rick Byers wrote: > nit: this function is simple enough you should just write the cast inline rather > than use a helper function. Done. https://codereview.chromium.org/727593003/diff/240001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:2234: static unsigned modifierFlagsFrom(const PlatformGestureEvent& event) On 2014/11/28 17:36:02, Rick Byers wrote: > Isn't this function pretty much equivalent to just 'return event.modifiers()'? > You should just be able to copy the modifiers directly without needing this > function at all, right? I'm not sure why the original code was doing this - > maybe just old... Yep.. you're right :) Thanks. Done. https://codereview.chromium.org/727593003/diff/240001/Source/core/page/EventH... Source/core/page/EventHandler.cpp:2855: PlatformEvent::Type eventType = PlatformEvent::MousePressed; On 2014/11/28 17:36:02, Rick Byers wrote: > Looks like I was wrong and this OS behavior difference will effect your CL for > the gesture long tap case only. You need to add RightButtonDown to the > modifiers for the MousePressed (non-windows) case. > > Sadly I guess this means you'll need a platform-specific expectations file for > windows (and need to detect windows from your layout test to change your > expected button for this case). Really the right fix here is to add a > WebPreference for 'show contextmenu on mouseup' which chrome sets to true only > on Windows (then we could test both in the same layout tests on all platforms). > This might be a nice follow-up CL if you're interested. If you want to do this, > then feel free to disable the test on Windows for now against that bug (or > better - just land an expectation that has a FAIL in it), rather than trying to > detect Windows and making it pass cleanly... Thank you for your guide. As you know, these works are in review. https://codereview.chromium.org/727593003/diff/240001/Source/platform/Platfor... File Source/platform/PlatformEvent.h (right): https://codereview.chromium.org/727593003/diff/240001/Source/platform/Platfor... Source/platform/PlatformEvent.h:84: RightButtonDown = 1 << 8, On 2014/11/28 17:36:02, Rick Byers wrote: > By the way, with the addition of these, I believe it should now be possible to > remove the (redundant and confusing) PlatformMouseEvent::m_modifierFlags. Can > you please add a FIXME there saying it's redundant and should be removed? Let > me know if you're interested in that CL too - otherwise I can do it (I _think_ > it should be easy, but I haven't traced all the code to verify). Done.
rbyers@chromium.org changed reviewers: + haraken@chromium.org
This looks great, thanks for all the related clean-up CLs! LGTM with a couple minor nits. +haraken for Source/web and Source/platform OWNERS https://codereview.chromium.org/727593003/diff/240001/LayoutTests/fast/events... File LayoutTests/fast/events/mouse-event-buttons-attribute.html (right): https://codereview.chromium.org/727593003/diff/240001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:92: eventSender.gestureLongPress(50, 50); On 2014/12/03 15:47:15, zino wrote: > On 2014/11/28 17:36:02, Rick Byers wrote: > > This is a little confusing. LongPress and LongTap are actually two different > > gestures. LongPress is what happens when you hold your finger (eg. can > trigger > > drang and drop on ChromeOS), and LongTap is what happens after holding your > > finger and lifting (eg. can trigger a context menu). You need two different > > actions to test both of them. > > Thank you for the information. > > I added contextmenu cases for two gestures fist. > But we still need dragNdrop cases for distinction between LongTap and LongPress. > > I was thinking I was able to add the tests in the follow-up CL for dragNdrop > because this CL ignores the cases for now. (I've already left a FIXME comment > for dragNdrop) > > I was wondering what you think about that. Yes, ignore drag and drop here. Touch drag and drop is disabled by default (only ChromeOS enables it at the moment), so it shouldn't affect your testing. For the purposes of this CL LongTap and LongPress are probably the same (but need to be tested separately since they hit different code paths). What you've got looks great, thanks! https://codereview.chromium.org/727593003/diff/380001/LayoutTests/fast/events... File LayoutTests/fast/events/mouse-event-buttons-attribute.html (right): https://codereview.chromium.org/727593003/diff/380001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:39: { type: GE, name: 'mousedown', modifiers: [R], expectedModifiers: [R], action: longPressAction }, nit: you can remove the expectedModifiers here since it's the same as the requested modifiers. https://codereview.chromium.org/727593003/diff/380001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:136: expectedModifiers = ' -> shouldBe([' + testSet[i].expectedModifiers + '])'; nit: avoid duplicating your strings here. Eg. you can simplify the above lines with something like: var modifiers = testSet[i].expectedModifiers || testSet[i].modifiers var expectedModifiers = ' -> shouldBe([' + modifiers + '])'; https://codereview.chromium.org/727593003/diff/380001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:145: var showContextMenuOnMouseUp; Note that comparing to undefined in javascript is a tricky thing you can simplify this and the below by saying: internals.settings.setShowContextMenuOnMouseUp(Boolean(testSet[i].showContextMenuOnMouseUp)) that'll convert undefined to false.
web/ and platform/ LGTM
Thanks :) https://codereview.chromium.org/727593003/diff/380001/LayoutTests/fast/events... File LayoutTests/fast/events/mouse-event-buttons-attribute.html (right): https://codereview.chromium.org/727593003/diff/380001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:39: { type: GE, name: 'mousedown', modifiers: [R], expectedModifiers: [R], action: longPressAction }, On 2014/12/03 16:58:09, Rick Byers wrote: > nit: you can remove the expectedModifiers here since it's the same as the > requested modifiers. Done. https://codereview.chromium.org/727593003/diff/380001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:136: expectedModifiers = ' -> shouldBe([' + testSet[i].expectedModifiers + '])'; On 2014/12/03 16:58:09, Rick Byers wrote: > nit: avoid duplicating your strings here. Eg. you can simplify the above lines > with something like: > var modifiers = testSet[i].expectedModifiers || testSet[i].modifiers > var expectedModifiers = ' -> shouldBe([' + modifiers + '])'; Done. https://codereview.chromium.org/727593003/diff/380001/LayoutTests/fast/events... LayoutTests/fast/events/mouse-event-buttons-attribute.html:145: var showContextMenuOnMouseUp; On 2014/12/03 16:58:09, Rick Byers wrote: > Note that comparing to undefined in javascript is a tricky thing > you can simplify this and the below by saying: > internals.settings.setShowContextMenuOnMouseUp(Boolean(testSet[i].showContextMenuOnMouseUp)) > > that'll convert undefined to false. Done.
The CQ bit was checked by jinho.bang@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/727593003/400001
Message was sent while issue was closed.
Committed patchset #9 (id:400001) as https://src.chromium.org/viewvc/blink?view=rev&revision=186490 |
