Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2011653003:
Only allow link clicks on single click. (Closed)
|
Created:
4 years, 7 months ago by dtapuska Modified:
4 years, 7 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOnly allow link clicks on single click.
When double clicking a link it would cause a double load. Ensure
link click only occurs on when the detail is 1.
BUG=614467
Committed: https://crrev.com/e46f8fbfd5d2d3408d09d4262fb0f4f99c93a98f
Cr-Commit-Position: refs/heads/master@{#396116}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add test and adjust comment #
Total comments: 3
Messages
Total messages: 22 (8 generated)
Description was changed from ========== Only allow link clicks on single click. When double clicking a link it would cause a double load. Ensure link click only occurs on when the detail is 1. BUG=614467 ========== to ========== Only allow link clicks on single click. When double clicking a link it would cause a double load. Ensure link click only occurs on when the detail is 1. BUG=614467 ==========
dtapuska@chromium.org changed reviewers: + rbyers@chromium.org, tkent@chromium.org
On 2016/05/25 14:13:42, dtapuska wrote: > mailto:dtapuska@chromium.org changed reviewers: > + mailto:rbyers@chromium.org, mailto:tkent@chromium.org tkent@ this is what we discussed on the bug. rbyers@ wdyt?
On 2016/05/25 14:14:12, dtapuska wrote: > On 2016/05/25 14:13:42, dtapuska wrote: > > mailto:dtapuska@chromium.org changed reviewers: > > + mailto:rbyers@chromium.org, mailto:tkent@chromium.org > > tkent@ this is what we discussed on the bug. > rbyers@ wdyt? Sounds like a great idea to me, thanks! Can you please check what other browsers do? Should we be updating any specs for this? CL needs a test of course.
On 2016/05/25 14:30:06, Rick Byers wrote: > On 2016/05/25 14:14:12, dtapuska wrote: > > On 2016/05/25 14:13:42, dtapuska wrote: > > > mailto:dtapuska@chromium.org changed reviewers: > > > + mailto:rbyers@chromium.org, mailto:tkent@chromium.org > > > > tkent@ this is what we discussed on the bug. > > rbyers@ wdyt? > > Sounds like a great idea to me, thanks! > > Can you please check what other browsers do? Should we be updating any specs > for this? CL needs a test of course. Seems FireFox has the same behaviour as us. IE; doesn't populate the details field so my test case is kind of invalid. But Edge does populate it and they have the desired behaviour this CL does. http://output.jsbin.com/mizicukixo
(I forgot to publish this nit before, sorry) https://codereview.chromium.org/2011653003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/2011653003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:376: // Allow detail <= 1 so that synthetic clicks work. They have detail == 0. nit: "they may have", right? Presumably JS should be passing detail:1 to the MouseEvent constructor.
On 2016/05/25 14:57:21, dtapuska wrote: > On 2016/05/25 14:30:06, Rick Byers wrote: > > On 2016/05/25 14:14:12, dtapuska wrote: > > > On 2016/05/25 14:13:42, dtapuska wrote: > > > > mailto:dtapuska@chromium.org changed reviewers: > > > > + mailto:rbyers@chromium.org, mailto:tkent@chromium.org > > > > > > tkent@ this is what we discussed on the bug. > > > rbyers@ wdyt? > > > > Sounds like a great idea to me, thanks! > > > > Can you please check what other browsers do? Should we be updating any specs > > for this? CL needs a test of course. > > Seems FireFox has the same behaviour as us. IE; doesn't populate the details > field so my test case is kind of invalid. But Edge does populate it and they > have the desired behaviour this CL does. Great, if Edge already behaves this way then the compat risk is minimal. I'm fine calling this a bug / "trivial" platform change.
PTAL https://codereview.chromium.org/2011653003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right): https://codereview.chromium.org/2011653003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:376: // Allow detail <= 1 so that synthetic clicks work. They have detail == 0. On 2016/05/25 17:25:14, Rick Byers wrote: > nit: "they may have", right? Presumably JS should be passing detail:1 to the > MouseEvent constructor. Done.
LGTM
The CQ bit was checked by dtapuska@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2011653003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2011653003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dtapuska@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2011653003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2011653003/20001
lgtm https://codereview.chromium.org/2011653003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/html/a-click.html (right): https://codereview.chromium.org/2011653003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/html/a-click.html:2: <body/> nit: Closing body tag here looks weird :)
Message was sent while issue was closed.
Description was changed from ========== Only allow link clicks on single click. When double clicking a link it would cause a double load. Ensure link click only occurs on when the detail is 1. BUG=614467 ========== to ========== Only allow link clicks on single click. When double clicking a link it would cause a double load. Ensure link click only occurs on when the detail is 1. BUG=614467 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Only allow link clicks on single click. When double clicking a link it would cause a double load. Ensure link click only occurs on when the detail is 1. BUG=614467 ========== to ========== Only allow link clicks on single click. When double clicking a link it would cause a double load. Ensure link click only occurs on when the detail is 1. BUG=614467 Committed: https://crrev.com/e46f8fbfd5d2d3408d09d4262fb0f4f99c93a98f Cr-Commit-Position: refs/heads/master@{#396116} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e46f8fbfd5d2d3408d09d4262fb0f4f99c93a98f Cr-Commit-Position: refs/heads/master@{#396116}
Message was sent while issue was closed.
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2011653003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/html/a-click.html (right): https://codereview.chromium.org/2011653003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/html/a-click.html:2: <body/> On 2016/05/25 at 22:57:45, tkent wrote: > nit: Closing body tag here looks weird :) Dave: Can you fix this? the / doesn't do anything. You should actually just remove the body entirely. The parser will insert one for you when it hits the <a> below. https://codereview.chromium.org/2011653003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/loader/repeat-same-document-navigation.html (left): https://codereview.chromium.org/2011653003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/loader/repeat-same-document-navigation.html:13: eventSender.mouseUp(); why did you change this? ::click() is different than going through eventSender. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
