Issue metadata
Sign in to add a comment
|
Clicks stopped working in unstable |
||||||||||||||||||||||
Issue descriptionChrome Version: Linux 58.0.3029.6 (Official Build) dev (64-bit) Chrome Version: Mac 59.0.3036.0 (Official Build) canary (64-bit) (didn't try this on Windows) What steps will reproduce the problem? (1) open fleurop.ch (2) click on the image of one of the 3 products shown What is the expected result? Navigate to the product page What happens instead? Nothing. The javascript console shows: "[Violation] Added non-passive event listener to a scroll-blocking 'touchstart' event. Consider marking event handler as 'passive' to make the page more responsive." Works as expected in chrome stable [56.0.2924.87 (Official Build) (64-bit)]
,
Mar 9 2017
This page uses a setPointerCapture and I believe this is caused by Navid's change to dispatching the click target as the captured target it set to the viewport. Works fine in Edge, both mouse and touch. https://codereview.chromium.org/2681443003/
,
Mar 9 2017
,
Mar 10 2017
,
Mar 10 2017
Just to give an update here. The target element in the page was never receiving mouseup event even though it was receiving a mousedown. So based on the spec it should not have expected the click event either. I will add some metrics to see how many pages are affected.
,
Mar 15 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/18a3165ddb7326845a6028b1e3f93f7d347bde95 commit 18a3165ddb7326845a6028b1e3f93f7d347bde95 Author: nzolghadr <nzolghadr@chromium.org> Date: Wed Mar 15 01:10:20 2017 Add use count for click retargeting due to capture Since pointer capture changes the target of the following events for that pointer it potentially changes the target of the click. Considering the changed target for calculating the click target may cause some websites to break. So this CL adds a usecount for that case. Note this is an upper bound on the breakage as lots of pages may not have any listener for these click events or just not break due to this retargetting. BUG= 699933 Review-Url: https://codereview.chromium.org/2747763002 Cr-Commit-Position: refs/heads/master@{#456933} [modify] https://crrev.com/18a3165ddb7326845a6028b1e3f93f7d347bde95/third_party/WebKit/Source/core/frame/UseCounter.h [modify] https://crrev.com/18a3165ddb7326845a6028b1e3f93f7d347bde95/third_party/WebKit/Source/core/input/MouseEventManager.cpp [modify] https://crrev.com/18a3165ddb7326845a6028b1e3f93f7d347bde95/third_party/WebKit/Source/core/input/MouseEventManager.h [modify] https://crrev.com/18a3165ddb7326845a6028b1e3f93f7d347bde95/third_party/WebKit/Source/core/input/PointerEventManager.cpp [modify] https://crrev.com/18a3165ddb7326845a6028b1e3f93f7d347bde95/tools/metrics/histograms/histograms.xml
,
Mar 16 2017
,
Mar 16 2017
Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 20 2017
Adding a me-too - We have a web-app that uses click and pointerdown events. If 'preventDefault()' is called inside a pointerdown event, it also kills any related click events - This is not true of mousedown events, and is not true prior to Chrome-57. Simple example file attached. Click both body and the button in Chrome 56 and Chrome 57+ to see the difference. Results go to JS console.
,
Mar 20 2017
Just to clarify things a little: 1. davies...@ what you are mentioning is a bug we introduced which should not have happened and will certainly be fixed. 2. The original problem in the reporter's website is caused by the fact that on pointerdown they capture the event to another node and so that new node will receive all the following events including pointerup and mouseup regardless of where you release the mouse. As per definition of click event the target should be the first common ancestor of mousedown and up targets. So when you click on one element in their page that same element does not receive the mouseup and then it shouldn't have received the click event either. But that is not what they expect. We are considering keeping the second paragraph behavior I explained depending on what we get from the metrics but the first paragraph will be definitely solved soon.
,
Mar 20 2017
Thank you for the clarification. FWIW I also agree with your assessment that the 2nd paragraph (new behaviour) is completely justified. In fact a common UI metaphor is that if you 'down + move-away + up', you are indicating cancellation of the click.
,
Mar 20 2017
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ddb951c0cc2912acd57e4238677b82e6360ac1ad commit ddb951c0cc2912acd57e4238677b82e6360ac1ad Author: nzolghadr <nzolghadr@chromium.org> Date: Mon Mar 20 16:45:46 2017 Ignore pointer capture target while calculating click target After https://codereview.chromium.org/2681443003 it turns out we are creating some regressions. So this change keeps the essence of the original change for the purpose of keeping the metric but ignores the target that is calculated from the captured target and use the original click target regardless of the capturing. BUG= 701599 , 699933 Review-Url: https://codereview.chromium.org/2755753003 Cr-Commit-Position: refs/heads/master@{#458091} [add] https://crrev.com/ddb951c0cc2912acd57e4238677b82e6360ac1ad/third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerevent_click_during_capture-manual-expected.txt [modify] https://crrev.com/ddb951c0cc2912acd57e4238677b82e6360ac1ad/third_party/WebKit/Source/core/input/MouseEventManager.cpp [modify] https://crrev.com/ddb951c0cc2912acd57e4238677b82e6360ac1ad/third_party/WebKit/Source/core/input/PointerEventManager.cpp
,
Mar 20 2017
Was a separate bug filed for #c9 or is that also being tracked here?
,
Mar 20 2017
I assume it is being tracked here as what I did was kind of reverting the original change and adding the metric for it. It was all part of the change in #c13 which went it today to master. I'll wait a few days and test the change in Canary. Please feel free to do the same thing in case you are interested. Then I'll merge it to beta.
,
Mar 20 2017
Awesome, thanks. I'll share another observation here then, for the record: the click event is only broken when the cancelled pointerdown was generated by mouse, NOT touch. https://jsfiddle.net/2j5aktmq/7/
,
Mar 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/38052ae353aee8c943b570b5a2c10be82a48c1ae commit 38052ae353aee8c943b570b5a2c10be82a48c1ae Author: johnme <johnme@chromium.org> Date: Tue Mar 21 12:45:46 2017 Revert of Ignore pointer capture target while calculating click target (patchset #1 id:1 of https://codereview.chromium.org/2755753003/ ) Reason for revert: Tentative revert. Hit testing broke in the following tests, and this patch looks the most closely related: AccessibilityHitTestingBrowserTest.HitTestingInIframes AccessibilityHitTestingBrowserTest.CachingAsyncHitTestingInIframes Original issue's description: > Ignore pointer capture target while calculating click target > > After https://codereview.chromium.org/2681443003 > it turns out we are creating some regressions. > So this change keeps the essence of the original > change for the purpose of keeping the metric but > ignores the target that is calculated from the > captured target and use the original click target > regardless of the capturing. > > BUG= 701599 , 699933 > > Review-Url: https://codereview.chromium.org/2755753003 > Cr-Commit-Position: refs/heads/master@{#458091} > Committed: https://chromium.googlesource.com/chromium/src/+/ddb951c0cc2912acd57e4238677b82e6360ac1ad TBR=dtapuska@chromium.org,mustaq@chromium.org,bokan@chromium.org,nzolghadr@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 701599 , 699933 Review-Url: https://codereview.chromium.org/2764813004 Cr-Commit-Position: refs/heads/master@{#458382} [delete] https://crrev.com/c5521157c6a33f93c544d45f5db15dbbe14ea8cd/third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerevent_click_during_capture-manual-expected.txt [modify] https://crrev.com/38052ae353aee8c943b570b5a2c10be82a48c1ae/third_party/WebKit/Source/core/input/MouseEventManager.cpp [modify] https://crrev.com/38052ae353aee8c943b570b5a2c10be82a48c1ae/third_party/WebKit/Source/core/input/PointerEventManager.cpp
,
Mar 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4b4080856b160e1abf3c3a21cacdd165847d90d8 commit 4b4080856b160e1abf3c3a21cacdd165847d90d8 Author: johnme <johnme@chromium.org> Date: Tue Mar 21 16:23:37 2017 Reland of Ignore pointer capture target while calculating click target (patchset #1 id:1 of https://codereview.chromium.org/2764813004/ ) Reason for revert: Reverting didn't fix https://build.chromium.org/p/chromium.android/builders/Lollipop%20Tablet%20Tester/builds/7242 so this patch seems innocent. Original issue's description: > Revert of Ignore pointer capture target while calculating click target (patchset #1 id:1 of https://codereview.chromium.org/2755753003/ ) > > Reason for revert: > Tentative revert. Hit testing broke in the following tests, and this patch looks the most closely related: > > AccessibilityHitTestingBrowserTest.HitTestingInIframes > AccessibilityHitTestingBrowserTest.CachingAsyncHitTestingInIframes > > Original issue's description: > > Ignore pointer capture target while calculating click target > > > > After https://codereview.chromium.org/2681443003 > > it turns out we are creating some regressions. > > So this change keeps the essence of the original > > change for the purpose of keeping the metric but > > ignores the target that is calculated from the > > captured target and use the original click target > > regardless of the capturing. > > > > BUG= 701599 , 699933 > > > > Review-Url: https://codereview.chromium.org/2755753003 > > Cr-Commit-Position: refs/heads/master@{#458091} > > Committed: https://chromium.googlesource.com/chromium/src/+/ddb951c0cc2912acd57e4238677b82e6360ac1ad > > TBR=dtapuska@chromium.org,mustaq@chromium.org,bokan@chromium.org,nzolghadr@chromium.org > # Skipping CQ checks because original CL landed less than 1 days ago. > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG= 701599 , 699933 > > Review-Url: https://codereview.chromium.org/2764813004 > Cr-Commit-Position: refs/heads/master@{#458382} > Committed: https://chromium.googlesource.com/chromium/src/+/38052ae353aee8c943b570b5a2c10be82a48c1ae TBR=dtapuska@chromium.org,mustaq@chromium.org,bokan@chromium.org,nzolghadr@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 701599 , 699933 Review-Url: https://codereview.chromium.org/2765683003 Cr-Commit-Position: refs/heads/master@{#458431} [add] https://crrev.com/4b4080856b160e1abf3c3a21cacdd165847d90d8/third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerevent_click_during_capture-manual-expected.txt [modify] https://crrev.com/4b4080856b160e1abf3c3a21cacdd165847d90d8/third_party/WebKit/Source/core/input/MouseEventManager.cpp [modify] https://crrev.com/4b4080856b160e1abf3c3a21cacdd165847d90d8/third_party/WebKit/Source/core/input/PointerEventManager.cpp
,
Mar 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1b708802860053bad35e8eaad2508f87701ec18b commit 1b708802860053bad35e8eaad2508f87701ec18b Author: nzolghadr <nzolghadr@chromium.org> Date: Tue Mar 21 19:22:57 2017 Revert of Ignore pointer capture target while calculating click target (patchset #1 id:1 of https://codereview.chromium.org/2755753003/ ) Reason for revert: Reverting all the changes related to the click target in capturing as they are splitted across multiple commits. Original issue's description: > Ignore pointer capture target while calculating click target > > After https://codereview.chromium.org/2681443003 > it turns out we are creating some regressions. > So this change keeps the essence of the original > change for the purpose of keeping the metric but > ignores the target that is calculated from the > captured target and use the original click target > regardless of the capturing. > > BUG= 701599 , 699933 > > Review-Url: https://codereview.chromium.org/2755753003 > Cr-Commit-Position: refs/heads/master@{#458091} > Committed: https://chromium.googlesource.com/chromium/src/+/ddb951c0cc2912acd57e4238677b82e6360ac1ad TBR=dtapuska@chromium.org,mustaq@chromium.org,bokan@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 701599 , 699933 Review-Url: https://codereview.chromium.org/2762913003 Cr-Commit-Position: refs/heads/master@{#458506} [delete] https://crrev.com/5de6f5cb0ef513f204df00f0d13d4da3f6a0fd86/third_party/WebKit/LayoutTests/external/wpt/pointerevents/pointerevent_click_during_capture-manual-expected.txt [modify] https://crrev.com/1b708802860053bad35e8eaad2508f87701ec18b/third_party/WebKit/Source/core/input/MouseEventManager.cpp [modify] https://crrev.com/1b708802860053bad35e8eaad2508f87701ec18b/third_party/WebKit/Source/core/input/PointerEventManager.cpp
,
Mar 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3d84b135e186d7d22d9597aa752ddc8f345be102 commit 3d84b135e186d7d22d9597aa752ddc8f345be102 Author: nzolghadr <nzolghadr@chromium.org> Date: Tue Mar 21 21:20:51 2017 Revert of Add use count for click retargeting due to capture Reason for revert: Reverting all the changes related to the click target in capturing as they are splitted across multiple commits. This reverts commit 18a3165ddb7326845a6028b1e3f93f7d347bde95. Original issue's description Since pointer capture changes the target of the following events for that pointer it potentially changes the target of the click. Considering the changed target for calculating the click target may cause some websites to break. So this CL adds a usecount for that case. Note this is an upper bound on the breakage as lots of pages may not have any listener for these click events or just not break due to this retargetting. BUG= 699933 TBR=dtapuska@chromium.org,mustaq@chromium.org,bokan@chromium.org Review-Url: https://codereview.chromium.org/2769433002 Cr-Commit-Position: refs/heads/master@{#458550} [modify] https://crrev.com/3d84b135e186d7d22d9597aa752ddc8f345be102/third_party/WebKit/Source/core/input/MouseEventManager.cpp [modify] https://crrev.com/3d84b135e186d7d22d9597aa752ddc8f345be102/third_party/WebKit/Source/core/input/MouseEventManager.h [modify] https://crrev.com/3d84b135e186d7d22d9597aa752ddc8f345be102/third_party/WebKit/Source/core/input/PointerEventManager.cpp
,
Mar 22 2017
Our team is still able to reproduce the issue in 58.0.3029.33, nzolghadr@ please take a look. Adding RBS for tracking purpose.
,
Mar 22 2017
I'm waiting a day or two before applying these last two changes c#19 and c#20 to 58 branch. Then everything should be good to go.
,
Mar 22 2017
Great, Thanks for the update.
,
Mar 24 2017
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e306c2eec0414707bf998990b6598c85e0efdb9c commit e306c2eec0414707bf998990b6598c85e0efdb9c Author: Navid Zolghadr <nzolghadr@chromium.org> Date: Mon Mar 27 17:57:33 2017 Revert of Add use count for click retargeting due to capture Reason for revert: Reverting all the changes related to the click target in capturing as they are splitted across multiple commits. This reverts commit 18a3165ddb7326845a6028b1e3f93f7d347bde95. Original issue's description Since pointer capture changes the target of the following events for that pointer it potentially changes the target of the click. Considering the changed target for calculating the click target may cause some websites to break. So this CL adds a usecount for that case. Note this is an upper bound on the breakage as lots of pages may not have any listener for these click events or just not break due to this retargetting. BUG= 699933 TBR=dtapuska@chromium.org,mustaq@chromium.org,bokan@chromium.org Review-Url: https://codereview.chromium.org/2769433002 Cr-Commit-Position: refs/heads/master@{#458550} (cherry picked from commit 3d84b135e186d7d22d9597aa752ddc8f345be102) (cherry picked from commit e96cb455bcb1979f239b242a2f81f5ed50572b28) Review-Url: https://codereview.chromium.org/2777213002 . Cr-Commit-Position: refs/branch-heads/3029@{#429} Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471} [modify] https://crrev.com/e306c2eec0414707bf998990b6598c85e0efdb9c/third_party/WebKit/Source/core/input/MouseEventManager.cpp [modify] https://crrev.com/e306c2eec0414707bf998990b6598c85e0efdb9c/third_party/WebKit/Source/core/input/MouseEventManager.h [modify] https://crrev.com/e306c2eec0414707bf998990b6598c85e0efdb9c/third_party/WebKit/Source/core/input/PointerEventManager.cpp
,
Mar 27 2017
,
Mar 29 2017
Tested the issue on Mac 10.12.3 and Ubuntu 14.04 using chrome beta version #58.0.3029.41 as per comment #9. Attached a screencast for reference. Steps followed are as follows: 1. Opened clicktest.html in chrome. 2. Click both body and the button and checked in console. 3. Observed that in console it was written as body click and button click. nzolghadr@ - Could you please verify the screencast and please let us know if it is the expected behaviour. Thanks...!!
,
Mar 29 2017
#27 Yes, your screencast looks correct to me - I can also confirm that in the latest unstable 59.0.3053.3, the issue has been resolved. When it was "broken" the "click" events were being lost.
,
Mar 29 2017
davies147@ - Thanks for your confirmation...!! As per comment #28 adding the verified labels. Thanks...!! |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by joaodasilva@chromium.org
, Mar 9 2017