Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Click event while a pointer event is captured #75

Open
NavidZ opened this issue Jun 1, 2016 · 28 comments

Comments

Projects
None yet
9 participants
@NavidZ
Copy link
Member

commented Jun 1, 2016

Consider these scenarios and their outputs in the following page.

The output of Edge (and it is the same in current Chrome canary with pointer event flags on):

1. Mousedown on blue. Mouseup on blue.
blue pointerdown
Set pointer capture to blue div
blue gotpointercapture
blue pointerup
blue click
blue lostpointercapture

2. Mousedown on green. Mouseup on green.
green pointerdown
Set pointer capture to blue div
blue gotpointercapture
blue pointerup
green click
blue lostpointercapture

3. Mousedown on blue. Move to green. Mouseup on green.
blue pointerdown
Set pointer capture to blue div
blue gotpointercapture
blue pointerup
grey click
blue lostpointercapture

4. Mousedown on green. Move to blue. Mouseup on blue.
green pointerdown
Set pointer capture to blue div
blue gotpointercapture
blue pointerup
grey click
blue lostpointercapture

The question here is about the click event. In scenarios 2, 3, and 4 they all go to an element that doesn't have capture. Is that expected? Do we like to also direct click/contextmenu event to the captured element or suppress it altogether if it is not targeted at the capturing element? Or do we want to maybe call out that click or context menu events are not considered pointer events so they don't need to be redirected like capturing?

I have a feeling that sending the click event as above is incorrect with the capturing scenarios as it is creating an inconsistent flow of events. I think maybe suppressing is the most reasonable action. what do you guys think?

@scottgonzalez

This comment has been minimized.

Copy link
Member

commented Jun 3, 2016

Only scenario 2 was unexpected to me.

The relative order of pointerup and lostpointercapture is defined in the spec, but the relative order of lostpointercapture and click is undefined, so I'm inclined to ignore the fact that this specific implementation fires lostpointercapture after click. With that in mind, here's why only scenario 2 was unexpected:

A click occurs when a button is pressed and released on the same element. Because pressing either the green or blue element inherently means pressing the grey element, I expect the grey element to receive a click in all scenarios. Only the first scenario should have a target other than grey, because a descendant of grey was the target of both the press and release.

There were also comments about this yesterday starting at #61 (comment).

@NavidZ

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2016

Why scenario 3 is correct? blue is getting pointerdown as well as pointerup. So how come grey is getting the click?

Also in scenario 4 which is related to your first paragraph, I think either click

  1. should be suppressed
  2. be sent to grey after the lostpointercapture
  3. be excluded from the capturing mechanism saying that click can go to the element that doesn't have capture

So I was not sure what you meant in your first paragraph. Which of these you think is the most reasonable?

@scottgonzalez

This comment has been minimized.

Copy link
Member

commented Jun 3, 2016

Why scenario 3 is correct?

Sorry, I missed that after going back up and reading the summary. Scenario 3 should have blue as the target.

So I was not sure what you meant in your first paragraph. Which of these you think is the most reasonable?

Well, given that the relative order of lostpointercapture and click is undefined, I don't think 2 can be specified unless we start defining the order of events that we don't define the behavior of. I'm not sure what 3 means since click is already excluded from the capturing mechanism.

@NavidZ

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2016

I modified the original issue to have the outputs right after each scenario to make it more clear.

So basically in 3, what I'm saying is that if click is defined as an event that will be fired to the first common ancestor of pointerdown and pointerup targets then scenario 4 makes sense. However from my understanding of the current spec no event should be fired to an element that doesn't have capturing except the boundary events (which we are trying to remove that to in other issues anyway). So click is not allowed to be sent to grey if blue has capturing either. Solution 3 of mine suggests to just explicitly exclude click from this in the spec and let click always be sent to the first common ancestor of pointerdown/up targets as per definition.

@scottgonzalez

This comment has been minimized.

Copy link
Member

commented Jun 3, 2016

However from my understanding of the current spec no event should be fired to an element that doesn't have capturing except the boundary events (which we are trying to remove that to in other issues anyway).

Even with the current spec, boundary events are not sent to other elements. What we're discussing removing is the boundary events that are sent to the captured element.

So click is not allowed to be sent to grey if blue has capturing either.

But the spec says "Immediately after firing the pointerup or pointercancel events, a user agent must run the steps as if the releasePointerCapture() method has been called with an argument equal to the pointerId property of the pointerup or pointercancel event just dispatched." so even though the timing of lostpointercapture in relation to click is undefined, I interpret this to mean that the actual capturing has already ended prior to the click event being fired, even if lostpointercapture has not yet fired.

Solution 3 of mine suggests to just explicitly exclude click from this in the spec and let click always be sent to the first common ancestor of pointerdown/up targets as per definition.

I believe this is already the case, but I may have forgotten a place where we mention specific details about click. Is there a part of the spec that you think contradicts this?

@teddink

This comment has been minimized.

Copy link

commented Jun 6, 2016

I am in agreement with Scott on this one. Scenario 2 seems like a bug to me - the click should follow the spec and move to grey as the first common ancestor. Also, Scenario 3 also seems like a bug - since blue experienced both the pointerdown and pointerup, blue should get the click.

Regarding the ordering of click and lostpointercapture - I agree from the cleanliness and logical perspective that click should come after lostpointercapture. However, I am not sure it really matters. Can we come up with a scenario where this ordering hampers what a web developer is trying to accomplish?

@RByers

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2016

I agree that scenario 2 seems like a bug - click should be on grey (common ancestor of the target of pointerdown and pointerup).

But if we agree on that, then what about scenario 3? Following the same principle, the pointerdown and pointerup targets are both blue so click should go to blue right? This, I think, is the most important question here, and the one directly related to the debate in #61. If capturing is a modification of hit-testing, then you'd absolutely expect a click on blue here. But maybe that's not really what developers would expect, and not web compatible with current pointer event usage?

@scottgonzalez

This comment has been minimized.

Copy link
Member

commented Jun 8, 2016

@RByers it seems like @teddink and I were both in agreement that blue should get the click event in scenario 3. I think this is what developers will expect and I doubt it would cause compatibility issues, though I don't have any data for compatibility.

@RByers

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2016

Whoops sorry, I see that now - thanks. Great, that really makes for the most consistent model I think! We should give that a try in Chrome and see how it goes.

@mustaqahmed

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2016

It seems we have all agreed that the nearest common ancestor of pointerdown & pointerup should receive the click event, right?

We shouldn't fire click for non-primary buttons, so we can restrict the events to primary buttons only ("pointerdown with button=0" and "pointerup with button=0" instead). This would cover the chorded buttons case but won't fire a click when the primary button doesn't cause pointerdown and/or pointerup.

I think this is fine. Alternatively, we can somehow include "mousemove with button=0" to fire a click event even on chorded primary buttons. IMO, this would make the spec a bit counter-intuitive w.r.t. the UI event spec, which perhaps doesn't make sense for this rare corner case.

@mustaqahmed

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2016

For the original problem stated here (click target with capturing), I think the current spec wording is fine because we agreed to the UI Event spec definition of click target (= the nearest common ancestor of down & up events). Do we still need a note?

(The other problem I mentioned in the last post is orthogonal to capturing, so I will create a new issue to cover that.)

@RByers

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2016

Yeah I think the current spec text is fine, but we need a test for this case and a bug filed against Edge for that test failing.

@teddink

This comment has been minimized.

@RByers

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2016

Agreed on the call https://www.w3.org/2016/06/22-pointerevents-minutes.html that we can now close this, remainder of the work is tracked elsewhere (Edge bug and WPT change).

@NavidZ

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2017

@patrickkettner we talked about this issue before with Ted. You and Andrew were also on that thread. The last decision was to stick to what UI event spec says and follow the behavior that is explained in this chrome bug. I remember Ted mentioning you guys are implementing that solution as well for this Edge bug.

I wanted to follow up with you guys regarding that and whether you hit any regressions with that or not. Particularly we hit a few regressions like this one.

@patrickkettner

This comment has been minimized.

Copy link

commented Apr 12, 2017

So the reason for our implementation was around the expected behavior for click interactions with buttons. If you click without releasing on an intractable element (e.g. button, anchors,etc), then drag off and release, the button does not trigger a click event. We can obviously special case for the literal <button>, elements, but we get into weird behavior when you are dealing with sites that have created their own links/buttons via divsoup

@scottgonzalez

This comment has been minimized.

Copy link
Member

commented Apr 17, 2017

We spent a long time discussing this specific scenario. We could not come up with a reason to use pointer capture on a button.

@freshp86

This comment has been minimized.

Copy link

commented Dec 6, 2017

Hi everyone. I would like to present a real world use-case from Chromium's codebase which is suffering from the fact that the 'cilck' event is fired on a non-capturing element.

Consider the following element, where a switcher UI control lies within a container row.
switcher_in_container_real

Requirements for this component are:

  • Switch can be toggled by clicking on it directly
  • Switch can be toggled dragging it towards the desired direction
  • Switch can be toggled by clicking on the surrounding box (white in this case).

I created a more minimal repro example here, which I've only tried in Chrome. Please ignore the Polymer code, and just scroll down to the bottom of the JS code (line 163 to the end).

When the user drags the toggle, then releases the pointer on top of the container box , the extraneous click event, is triggering the container's click handler, which unexpectedly toggles the switch. The hack we've resorted is to cache the last pointerup event timeStamp and compare it with the click event's timeStamp. If they are the same the event is ignored. Otherwise, this indicates the user clicked directly on the container and the event is handled.

I would like to remove that hack, so trying to restart this conversation on why the current behavior of letting click events fire on non-capture element is useful.
Thank you

@NavidZ

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2017

Hi @freshp86 .
I myself am a big fan of sending the click event to the captured node. I tried implementing it in chrome back then but we hit a few regressions and we couldn't ship it yet. We are still trying to ship that as that is a more predictable and consistent behavior IMO. But it might take a little longer.

However, I don't quite agree with your definition of hack.
Mainly because the logic you have in the click handler of the toggle button itself (i.e. dom-module) is different from the click handler of the clickable-row. Don't you think that is unfair to expect them do the same thing?
You already have a hack in the click handler of the dom-module to ignore the click that is sent to itself.

// User gesture has already been taken care of inside |pointermove| // handlers, Do nothing here. if (this.handledInPointerMove_) return;
That logic kicks in when I move the pointer around and then release the pointer inside that module. But then you don't have that logic in the outer click handler and when you add it to that you call it a hack? If I were you I would have a function in my dom-module to handle the click (which you do and already has that handledInPointerMove check). Then I would just call that from the click handler of my clickable-row rather than trying to modify the checked attribute directly.

@freshp86

This comment has been minimized.

Copy link

commented Dec 6, 2017

Then I would just call that from the click handler of my clickable-row rather than trying to modify the checked attribute directly.

@NavidZ: Agreed that I could modify the click handler of the clickable row to call a method on the toggle button itself, instead of comparing the timestamps. But it is worth noting that being able to toggle the switch by simply setting/unsetting the checked attribute, is kind of important. It makes the custom <cr-toggle> element more consistent with a native <input type="checkbox">, and easier to drop-in replace. On the other hand, calling a non-standard method from the parent's click handler, makes the parent unnecessarily aware of the type of toggle it holds, which hinders the "drop-in replacement" ability.

Also, my point mainly is that the clickable row should not have received a 'click' event, when the toggle was the capturing element. And therefore just setting/unsetting the checked attribute should be sufficient, because it would only trigger when no capturing was involved. Any additional work seems unnecessary, whether it qualifies as a "hack" or not.

Edit: Also note that extraneous 'click' event does not have the cr-toggle anywhere in its |path| attribute, which eliminates one more way that could be used to filter out that event.

@scottgonzalez

This comment has been minimized.

Copy link
Member

commented Dec 6, 2017

Based on the previous resolution of this ticket, this seems like a browser bug. Both pointerdown and pointerup occur on the cr-toggle element because of pointer capture. As such, click should be fired on cr-toggle and not on div.clickable-row.

If the click event were fired on cr-toggle, then the click handler on div.clickable-row would be able to filter out events based on the target, which would still allow for drop-in replacements of the toggle button.

@scottgonzalez

This comment has been minimized.

Copy link
Member

commented Dec 6, 2017

@freshp86

This comment has been minimized.

Copy link

commented Dec 6, 2017

If the click event were fired on cr-toggle, then the click handler on div.clickable-row would be able to filter out events based on the target, which would still allow for drop-in replacements of the toggle button.

Actually cr-toggle itself calls stopPropagation() already, so if the event was fired on cr-toggle, the div.clickable-row click handler would not be triggered in the first place.

Based on the previous resolution of this ticket, this seems like a browser bug.

I did not realize that this discussion has concluded. Thanks.

@scottgonzalez

This comment has been minimized.

Copy link
Member

commented Dec 6, 2017

I did not realize that this discussion has concluded. Thanks.

I thought we re-closed this back in April but apparently not.

@NavidZ @patrickhlauke Is there still debate about this? I thought the conclusion was that this is all standard behavior and we didn't need any special mention for dealing with clicks and pointer capture.

@scottgonzalez

This comment has been minimized.

Copy link
Member

commented Dec 6, 2017

Actually cr-toggle itself calls stopPropagation() already, so if the event was fired on cr-toggle, the div.clickable-row click handler would not be triggered in the first place.

I don't think that's correct behavior if you're trying to build a drop-in replacement for a checkbox. It is the row's responsibility to handle the composition since it is the composed widget. You cannot expect your building blocks to anticipate every possible composition.

@NavidZ

This comment has been minimized.

Copy link
Member Author

commented Dec 7, 2017

Based on the previous resolution of this ticket, this seems like a browser bug. Both pointerdown and pointerup occur on the cr-toggle element because of pointer capture. As such, click should be fired on cr-toggle and not on div.clickable-row.

@scottgonzalez your conclusion is totally right. I believe there is no need to change in the spec to address this.
The only problem was that when we wanted to have the change in Chrome there were some websites that stopped working correctly. So we had to revert it. I believe both Firefox and Edge do the same thing as current Chrome implementation (i.e. not honoring the capture when sending click). Assuming we can get around the compat issues then it will be the browser bugs.

But if not we may need to make an exception for the click case solely for the compat reasons which is not my favorite. Either way I'm happy with closing this for now and only keep the browser bugs open for this issue.

@smaug----

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2018

FWIW, current FF does not do the same thing. Since pointer/mouseup happen on different element than pointer/mouseup in capturing case, click isn't dispatched.
FF follows the current spec, but apparently the current spec isn't web compatible :/

Note, current Blink doesn't really seem to capture anything if one tries to capture pointer in a different document. Gecko has also an bug there, but different. Edge behaves sanely.
(click the blue box, *up events should go to HTMLHeadingElement)

@NavidZ NavidZ self-assigned this Mar 28, 2018

@NavidZ NavidZ added the v3 label Mar 28, 2018

@NavidZ

This comment has been minimized.

Copy link
Member Author

commented Mar 28, 2018

We talked about this issue in the call and we all agreed with the proposed solution here. Basically stick to sending click to the first common ancestor of pointerdown and pointerup targets and pointercapture might changed the targets of pointerup/down and consequently it might effect the click targets. Right now neither Chrome nor Edge is following this. But they are going to fix it. Adding a v3 to keep an eye on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.