Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(31)

Issue 1894253002: Prevent sending click event for non primary button (Closed)

Created:
4 years, 8 months ago by Navid Zolghadr
Modified:
4 years, 7 months ago
CC:
chromium-reviews, blink-reviews, dtapuska+blinkwatch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Prevent sending click event for non primary button Prevent sending click and dblclick events when the button in charge is a non-primary mouse button. As the result of this change there is no way to prevent the behavior of opening a new tap when middle clicking on a link. Previously, js could listen to click event of the middle button and call preventDefault on it. BUG=255 Committed: https://crrev.com/76fea00a18f75886ea649414393228180306e13d Cr-Commit-Position: refs/heads/master@{#390195}

Patch Set 1 #

Patch Set 2 : Fixing tests #

Total comments: 10

Patch Set 3 : Use stopPropagation #

Patch Set 4 : Remove empty line #

Total comments: 10

Patch Set 5 : Removing the test #

Total comments: 1

Patch Set 6 : Rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+19 lines, -27 lines) Patch
M chrome/browser/ui/browser_browsertest.cc View 1 2 3 4 5 1 chunk +0 lines, -22 lines 0 comments Download
M testing/buildbot/filters/browser-side-navigation.linux.browser_tests.filter View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/events/mouse-click-events-expected.txt View 1 2 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/fast/events/script-tests/mouse-click-events.js View 1 2 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/events/MouseEvent.cpp View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/input/EventHandler.cpp View 1 2 3 4 5 1 chunk +13 lines, -2 lines 0 comments Download

Messages

Total messages: 38 (12 generated)
Navid Zolghadr
This is a possible fix for the middle click issue. Note the complication of default ...
4 years, 8 months ago (2016-04-18 20:50:07 UTC) #2
Navid Zolghadr
ptal. Following our discussion from yesterday's standup I just removed the preventDefault behavior altogether.
4 years, 8 months ago (2016-04-22 18:24:39 UTC) #5
dtapuska
https://codereview.chromium.org/1894253002/diff/20001/third_party/WebKit/LayoutTests/fast/events/mouse-click-events-expected.txt File third_party/WebKit/LayoutTests/fast/events/mouse-click-events-expected.txt (right): https://codereview.chromium.org/1894253002/diff/20001/third_party/WebKit/LayoutTests/fast/events/mouse-click-events-expected.txt#newcode9 third_party/WebKit/LayoutTests/fast/events/mouse-click-events-expected.txt:9: FAIL eventLog should be mousedown(1) mouseup(1) click(1) mousedown(1) mouseup(1) ...
4 years, 8 months ago (2016-04-22 18:32:30 UTC) #6
Navid Zolghadr
https://codereview.chromium.org/1894253002/diff/20001/third_party/WebKit/LayoutTests/fast/events/mouse-click-events-expected.txt File third_party/WebKit/LayoutTests/fast/events/mouse-click-events-expected.txt (right): https://codereview.chromium.org/1894253002/diff/20001/third_party/WebKit/LayoutTests/fast/events/mouse-click-events-expected.txt#newcode9 third_party/WebKit/LayoutTests/fast/events/mouse-click-events-expected.txt:9: FAIL eventLog should be mousedown(1) mouseup(1) click(1) mousedown(1) mouseup(1) ...
4 years, 8 months ago (2016-04-22 19:09:58 UTC) #7
dtapuska
https://codereview.chromium.org/1894253002/diff/20001/third_party/WebKit/Source/core/events/EventDispatcher.cpp File third_party/WebKit/Source/core/events/EventDispatcher.cpp (left): https://codereview.chromium.org/1894253002/diff/20001/third_party/WebKit/Source/core/events/EventDispatcher.cpp#oldcode123 third_party/WebKit/Source/core/events/EventDispatcher.cpp:123: if (dispatchEventPreProcess(preDispatchEventHandlerResult) == ContinueDispatching) { On 2016/04/22 19:09:58, Navid ...
4 years, 8 months ago (2016-04-22 19:16:33 UTC) #8
mustaq
On 2016/04/22 19:16:33, dtapuska wrote: > https://codereview.chromium.org/1894253002/diff/20001/third_party/WebKit/Source/core/events/EventDispatcher.cpp > File third_party/WebKit/Source/core/events/EventDispatcher.cpp (left): > > https://codereview.chromium.org/1894253002/diff/20001/third_party/WebKit/Source/core/events/EventDispatcher.cpp#oldcode123 > ...
4 years, 8 months ago (2016-04-25 14:47:56 UTC) #9
Navid Zolghadr
ptal. I believe the stoppropagation fixed it and it is very clean. https://codereview.chromium.org/1894253002/diff/20001/third_party/WebKit/Source/core/events/EventDispatcher.cpp File third_party/WebKit/Source/core/events/EventDispatcher.cpp ...
4 years, 8 months ago (2016-04-25 18:37:49 UTC) #10
dtapuska
https://codereview.chromium.org/1894253002/diff/60001/third_party/WebKit/Source/core/events/MouseEvent.cpp File third_party/WebKit/Source/core/events/MouseEvent.cpp (right): https://codereview.chromium.org/1894253002/diff/60001/third_party/WebKit/Source/core/events/MouseEvent.cpp#newcode297 third_party/WebKit/Source/core/events/MouseEvent.cpp:297: // Do not send dblclick event for middle click ...
4 years, 8 months ago (2016-04-25 19:59:37 UTC) #11
dtapuska
On 2016/04/25 19:59:37, dtapuska wrote: > https://codereview.chromium.org/1894253002/diff/60001/third_party/WebKit/Source/core/events/MouseEvent.cpp > File third_party/WebKit/Source/core/events/MouseEvent.cpp (right): > > https://codereview.chromium.org/1894253002/diff/60001/third_party/WebKit/Source/core/events/MouseEvent.cpp#newcode297 > ...
4 years, 8 months ago (2016-04-25 20:00:51 UTC) #12
mustaq
https://codereview.chromium.org/1894253002/diff/60001/third_party/WebKit/Source/core/events/MouseEvent.cpp File third_party/WebKit/Source/core/events/MouseEvent.cpp (right): https://codereview.chromium.org/1894253002/diff/60001/third_party/WebKit/Source/core/events/MouseEvent.cpp#newcode294 third_party/WebKit/Source/core/events/MouseEvent.cpp:294: if (mouseEvent.type() != EventTypeNames::click || mouseEvent.detail() != 2) This ...
4 years, 8 months ago (2016-04-25 20:12:43 UTC) #13
Navid Zolghadr
https://codereview.chromium.org/1894253002/diff/60001/third_party/WebKit/Source/core/events/MouseEvent.cpp File third_party/WebKit/Source/core/events/MouseEvent.cpp (right): https://codereview.chromium.org/1894253002/diff/60001/third_party/WebKit/Source/core/events/MouseEvent.cpp#newcode294 third_party/WebKit/Source/core/events/MouseEvent.cpp:294: if (mouseEvent.type() != EventTypeNames::click || mouseEvent.detail() != 2) On ...
4 years, 8 months ago (2016-04-25 20:17:39 UTC) #14
mustaq
https://codereview.chromium.org/1894253002/diff/60001/third_party/WebKit/Source/core/events/MouseEvent.cpp File third_party/WebKit/Source/core/events/MouseEvent.cpp (right): https://codereview.chromium.org/1894253002/diff/60001/third_party/WebKit/Source/core/events/MouseEvent.cpp#newcode294 third_party/WebKit/Source/core/events/MouseEvent.cpp:294: if (mouseEvent.type() != EventTypeNames::click || mouseEvent.detail() != 2) On ...
4 years, 8 months ago (2016-04-25 20:47:35 UTC) #15
Navid Zolghadr
ptal. It seems that the test was doing an action on click action of middle ...
4 years, 8 months ago (2016-04-26 17:04:07 UTC) #17
dtapuska
lgtm https://codereview.chromium.org/1894253002/diff/80001/chrome/browser/ui/browser_browsertest.cc File chrome/browser/ui/browser_browsertest.cc (left): https://codereview.chromium.org/1894253002/diff/80001/chrome/browser/ui/browser_browsertest.cc#oldcode2639 chrome/browser/ui/browser_browsertest.cc:2639: // http://crbug.com/396347 Can you please ensure that crbug ...
4 years, 8 months ago (2016-04-26 17:11:56 UTC) #18
Navid Zolghadr
On 2016/04/26 17:11:56, dtapuska wrote: > lgtm > > https://codereview.chromium.org/1894253002/diff/80001/chrome/browser/ui/browser_browsertest.cc > File chrome/browser/ui/browser_browsertest.cc (left): > ...
4 years, 8 months ago (2016-04-26 17:13:56 UTC) #19
Navid Zolghadr
bokan@chromium.org: Please review changes in third_party/WebKit/Source/core/events/MouseEvent.cpp third_party/WebKit/Source/core/input/EventHandler.cpp
4 years, 8 months ago (2016-04-26 17:20:06 UTC) #21
bokan
WebKit core lgtm
4 years, 8 months ago (2016-04-26 20:44:08 UTC) #22
Navid Zolghadr
thestig@chromium.org: Please review changes in chrome/browser/ui/browser_browsertest.cc
4 years, 8 months ago (2016-04-26 20:53:08 UTC) #24
Lei Zhang
lgtm
4 years, 8 months ago (2016-04-26 21:38:26 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1894253002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1894253002/80001
4 years, 7 months ago (2016-04-27 13:44:43 UTC) #27
commit-bot: I haz the power
Try jobs failed on following builders: ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn/builds/25897)
4 years, 7 months ago (2016-04-27 13:48:58 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1894253002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1894253002/100001
4 years, 7 months ago (2016-04-27 20:01:06 UTC) #32
commit-bot: I haz the power
Committed patchset #6 (id:100001)
4 years, 7 months ago (2016-04-27 21:34:02 UTC) #34
vandebo (ex-Chrome)
If I read the description correctly, this change means that pages / extensions no longer ...
4 years, 7 months ago (2016-04-28 06:11:04 UTC) #35
dtapuska
On 2016/04/28 06:11:04, vandebo (ex-Chrome) wrote: > If I read the description correctly, this change ...
4 years, 7 months ago (2016-04-28 13:16:34 UTC) #36
commit-bot: I haz the power
4 years, 7 months ago (2016-04-30 17:13:24 UTC) #37
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/76fea00a18f75886ea649414393228180306e13d
Cr-Commit-Position: refs/heads/master@{#390195}

Powered by Google App Engine
This is Rietveld 408576698