New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 699933 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Clicks stopped working in unstable

Project Member Reported by joaodasilva@chromium.org, Mar 9 2017

Issue description

Chrome 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)]
 
Cc: tmarek@google.com
Cc: rbyers@chromium.org mustaq@chromium.org
Labels: -Type-Bug Hotlist-Input-Dev Type-Bug-Regression
Owner: nzolghadr@chromium.org
Status: Assigned (was: Untriaged)
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/ 
Labels: PointerEvent
Status: Started (was: Assigned)

Comment 4 by jochen@chromium.org, Mar 10 2017

Cc: -eisinger@chromium.org
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.
Project Member

Comment 6 by bugdroid1@chromium.org, 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

Labels: Merge-Request-58
Project Member

Comment 8 by sheriffbot@chromium.org, Mar 16 2017

Labels: -Merge-Request-58 Hotlist-Merge-Approved Merge-Approved-58
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

Comment 9 by davies...@gmail.com, 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.

clicktest.html
599 bytes View Download
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. 
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.
Project Member

Comment 12 by sheriffbot@chromium.org, Mar 20 2017

Cc: keta...@chromium.org bhthompson@google.com ketakid@google.com
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
Project Member

Comment 13 by bugdroid1@chromium.org, 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

Was a separate bug filed for #c9 or is that also being tracked here?
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.
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/
Project Member

Comment 17 by bugdroid1@chromium.org, 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

Project Member

Comment 18 by bugdroid1@chromium.org, 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

Project Member

Comment 19 by bugdroid1@chromium.org, 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

Project Member

Comment 20 by bugdroid1@chromium.org, 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

Cc: ligim...@chromium.org
Labels: ReleaseBlock-Stable
Our team is still able to reproduce the issue in 58.0.3029.33, nzolghadr@ please take a look.

Adding RBS for tracking purpose.
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.
Great, Thanks for the update.
Project Member

Comment 24 by sheriffbot@chromium.org, 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
Project Member

Comment 25 by bugdroid1@chromium.org, Mar 27 2017

Labels: -merge-approved-58 merge-merged-3029
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

Status: Fixed (was: Started)
Cc: krajshree@chromium.org
Labels: Needs-Feedback
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...!!

699933.mp4
725 KB View Download
#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.

Labels: TE-Verified-M58 TE-Verified-58.0.3029.41
davies147@ - Thanks for your confirmation...!!

As per comment #28 adding the verified labels.

Thanks...!!

Sign in to add a comment