Adding release blocker label for this issue.Please reduce priority or remove if not the case.

Thank You!

dpapad@, do you know what the best owner for this bug is? It seems that the toggle buttons in the settings are relying on the unspecified Chrome behavior and the CL that makes Chrome match the spec and other browsers break this usecase. I don't know where the code is for those components in Chrome. Could you find a better owner here?

Removing stable blocker as users can still easily toggle the button with just a normal click rather than a drag.

@nzolghadr: Can you explain what is the unspecified behavior? 

Also can you please take a look at the comments at [1] and [2], which also refer to issue 689158 (which you are currently the owner)?

IIUC, your change is breaking that behavior? Even if the new behavior is more spec compliant, I would like to find a solution first, before breaking current WebUI behaviors. WDYT?

Finally, potentially duplicate issue 864413.

[1] https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_toggle/cr_toggle.html?l=95-100
[2] https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_checkbox/cr_checkbox.html?l=125-127

Sure. In the spec there is no condition on the boundary detection to find the parent element of down/up events and we should always fire it. There used to be some websites relying on this behavior of Chrome and they fixed their issues. So we decided to revisit this interoperability issue.

Related section of the spec:
https://www.w3.org/TR/uievents/#events-mouseevent-event-order
... fire click and dblclick events on the nearest common inclusive ancestor when the	associated mousedown and mouseup event targets are different.

Obviously since the settings UI lives in Chrome only it is not used on other browser so this is not very recognizable that it doesn't work in other browsers.
Anyhow, regarding the capture issue that is assigned to me. I'm not sure how this is related to that. Do you use pointer capture in the settings page? If not that shouldn't affect you at all. That is also a spec compliant issue that we tried and we broke some websites and we had to revert it. But we are going to fix that as well as soon as we see less compat issues.

Do you think this action of dragging a toggle element in the settings common? If not I prefer to keep the change in the Chrome as we haven't heard any other website breaking due to this issue so far. WDYT?

> Do you think this action of dragging a toggle element in the settings common? 

I think it is important. Especially because this can also have privacy implications. If the user thinks they disabled some toggle, but the toggle accidentally re-enables itself, it can be very very confusing (at the very least).

> Do you use pointer capture in the settings page?

Yes. Please see https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_toggle/cr_toggle.js?g=0&l=127

Also see https://chromium.googlesource.com/chromium/src/+/79985f94e043bd4de3f3652a19f4b7698291cdd7, where we were using a different workaround for the same use case, but we simplified it by using a wrapping <button> element, because it worked at the time.

Shouldn't we just always ignore click event when you start toggling as the result of drag? Does that work in both cases (like capturing click retargetting and this one as well.

> Shouldn't we just always ignore click event when you start toggling as the result of drag?

The problem is explained in more detail at https://bugs.chromium.org/p/chromium/issues/detail?id=768555 (and there is a minimal repro as well).

The problem is that the toggle resides in a clickable parent, and clicking on the parent should also cause the toggle change state. When the drag gesture ends within the parent's boundaries, the parent thinks that it should re-toggle the toggle.

I know. That is why I was suggesting that way. Basically if the drag starts (and moved the button) neither the button nor the parent should toggle the button as the result of the click event. Is that fair?

The previous workaround (before r555944), stored a timestamp of the pointerup event. The parent (which received the unwanted 'click' event), would call a method to compare whether the 'click' event has the same timestamp as the 'pointerup' and if so it would not change the toggle's state).

After rr555944, that was no longer necessary, because wrapping the toggle with a <button> eliminated the unwanted 'click' event on the parent (which is no longer the case after r580554).

> I know. That is why I was suggesting that way. Basically if the drag starts (and moved the button) neither the button nor the parent should toggle the button as the result of the click event. Is that fair?

I am confused by this. By the time the unwanted 'click' event fires, the drag gesture has finished. The parent has no way of knowing that a gesture preceded (except for the previous timestamp workaround I described).

Yeah. Basically the workaround that you suggested was the way to go here again. 
Another question. What happens when I release my pointer on the button itself? I mean say I drag it from on to off but without moving the mouse further I release it. Now the element is getting the click as well. How do you handle that not to toggle the button again?

> What happens when I release my pointer on the button itself?

That case is easy to handle, see logic at https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/cr_toggle/cr_toggle.js?l=135-138,147-152

The unwanted "click" event on the parent, unfortunately is fired directly on the parent. So every user of cr-toggle would have to know that they need to check with the child first to decide whether they should toggle. This is fairly inconvenient as you probably guess, which is why the <button> solution seemed much more scalable (users of the element did not need to know anything special).

Could we revert the change for now, until we can fix cr-toggle to not rely on the previosu behavior, and then potentially re-land your change?

Reverting back to the previous "timestamp" based workaround is not trivial, since all users of cr-toggle need to be audited on whether they are supposed to be clickable.

Actually I really don't like that workaround either.

So if I fix the problem with targeting click while the pointer is captured (i.e. in your case since you set the pointercapture on down you are going to get the rest of move and up events and also "click" event for that pointer with fixing issue 689158) then you wouldn't need this workaround and this issue is prevented even with the current change in place. Is that correct?

Yes, that is correct. Issue 689158 is the root of the problem, so fixing it would be great, and it  would eliminate the need for either of the two workarounds (<button> wrapper, timestamp comparison).

I can help verifying any candidate fix locally, if that helps.

No. That other issue itself is going to be quite a task to land. But I can see the usecase here and I don't like to land a fix while there is a valid usecase that cannot be addressed without the current Chrome behavior. I'm going to revert this change for now and make sure the other issue with pointercapture is fixed first before this patch lands again

> I'm going to revert this change for now and make sure the other issue with pointercapture is fixed first before this patch lands again

Thank you!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6c9478fffb43e0411078a846fc10f9f455d8a432

commit 6c9478fffb43e0411078a846fc10f9f455d8a432
Author: Navid Zolghadr <nzolghadr@chromium.org>
Date: Tue Oct 02 18:52:32 2018

Revert of 'Send click event to the nearest common ancestor'

This is a partial revert of the click target calculation
CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1161164

which puts https://crbug.com/869919 back on the table.

It still keeps the wpt test though to make sure the test is
there for the final desired behavior.

Bug:  890711 , 869919
Change-Id: I3ea000a8f62b08b7222f91d27a6957d5a94e8388
Reviewed-on: https://chromium-review.googlesource.com/1256377
Commit-Queue: Navid Zolghadr <nzolghadr@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595932}
[modify] https://crrev.com/6c9478fffb43e0411078a846fc10f9f455d8a432/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/6c9478fffb43e0411078a846fc10f9f455d8a432/third_party/WebKit/LayoutTests/fast/dom/shadow/select-in-shadowdom-expected.txt
[modify] https://crrev.com/6c9478fffb43e0411078a846fc10f9f455d8a432/third_party/WebKit/LayoutTests/fast/dom/shadow/select-in-shadowdom.html
[modify] https://crrev.com/6c9478fffb43e0411078a846fc10f9f455d8a432/third_party/WebKit/LayoutTests/fast/events/click-over-descendant-elements-expected.txt
[modify] https://crrev.com/6c9478fffb43e0411078a846fc10f9f455d8a432/third_party/WebKit/LayoutTests/fast/events/click-over-descendant-elements.html
[modify] https://crrev.com/6c9478fffb43e0411078a846fc10f9f455d8a432/third_party/blink/renderer/core/input/event_handling_util.cc
[modify] https://crrev.com/6c9478fffb43e0411078a846fc10f9f455d8a432/third_party/blink/renderer/core/input/mouse_event_manager.cc

I reverted the change and as far as this use case works again. Can someone else also verify this?

This bug requires manual review: We are only 12 days from stable.
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Re #19: I tested it locally and seems to work fine after the revert.

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

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0276fa1a88e6ca061ac6f7a7379475fdfac55b07

commit 0276fa1a88e6ca061ac6f7a7379475fdfac55b07
Author: Navid Zolghadr <nzolghadr@chromium.org>
Date: Tue Oct 09 13:15:06 2018

Revert of 'Send click event to the nearest common ancestor'

This is a partial revert of the click target calculation
CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1161164

which puts https://crbug.com/869919 back on the table.

It still keeps the wpt test though to make sure the test is
there for the final desired behavior.

Bug:  890711 , 869919
Change-Id: I3ea000a8f62b08b7222f91d27a6957d5a94e8388
Reviewed-on: https://chromium-review.googlesource.com/1256377
Commit-Queue: Navid Zolghadr <nzolghadr@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#595932}(cherry picked from commit 6c9478fffb43e0411078a846fc10f9f455d8a432)
Reviewed-on: https://chromium-review.googlesource.com/c/1270526
Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#915}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/0276fa1a88e6ca061ac6f7a7379475fdfac55b07/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/0276fa1a88e6ca061ac6f7a7379475fdfac55b07/third_party/WebKit/LayoutTests/fast/dom/shadow/select-in-shadowdom-expected.txt
[modify] https://crrev.com/0276fa1a88e6ca061ac6f7a7379475fdfac55b07/third_party/WebKit/LayoutTests/fast/dom/shadow/select-in-shadowdom.html
[modify] https://crrev.com/0276fa1a88e6ca061ac6f7a7379475fdfac55b07/third_party/WebKit/LayoutTests/fast/events/click-over-descendant-elements-expected.txt
[modify] https://crrev.com/0276fa1a88e6ca061ac6f7a7379475fdfac55b07/third_party/WebKit/LayoutTests/fast/events/click-over-descendant-elements.html
[modify] https://crrev.com/0276fa1a88e6ca061ac6f7a7379475fdfac55b07/third_party/blink/renderer/core/input/event_handling_util.cc
[modify] https://crrev.com/0276fa1a88e6ca061ac6f7a7379475fdfac55b07/third_party/blink/renderer/core/input/mouse_event_manager.cc

I just merged this into branch. So we should be good for now.

The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/0276fa1a88e6ca061ac6f7a7379475fdfac55b07

Commit: 0276fa1a88e6ca061ac6f7a7379475fdfac55b07
Author: nzolghadr@chromium.org
Commiter: nzolghadr@chromium.org
Date: 2018-10-09 13:15:06 +0000 UTC

Revert of 'Send click event to the nearest common ancestor'

This is a partial revert of the click target calculation
CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1161164

which puts https://crbug.com/869919 back on the table.

It still keeps the wpt test though to make sure the test is
there for the final desired behavior.

Bug:  890711 , 869919
Change-Id: I3ea000a8f62b08b7222f91d27a6957d5a94e8388
Reviewed-on: https://chromium-review.googlesource.com/1256377
Commit-Queue: Navid Zolghadr <nzolghadr@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#595932}(cherry picked from commit 6c9478fffb43e0411078a846fc10f9f455d8a432)
Reviewed-on: https://chromium-review.googlesource.com/c/1270526
Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#915}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}