Change light dismiss to use click events#11536
Change light dismiss to use click events#11536josepharhar wants to merge 9 commits intowhatwg:mainfrom
Conversation
This PR replaces the existing pointerdown and pointerup listening and tracking with algorithms which use a click event. The click event is going to be used by the pointerevents spec here: w3c/pointerevents#460 Using click events instead of doing click detection with pointerdown and pointerup fixes a number of issues which are detailed here: w3c/pointerevents#542 Fixes whatwg#10905
foolip
left a comment
There was a problem hiding this comment.
Editorially looks good and makes a lot of things simpler. I just have opinions about types :)
|
@annevk @smaug---- do yall have any feedback on this? |
|
I'm likely missing something here, but since mousedown triggers select's popup to be shown, aren't there cases when just relying on click here then immediately dismisses it. |
Good catch! I'm working on a fix for this, I'll try putting it in this spec PR after I get something that works well. |
|
Here is the aforementioned fix for the immediate light dismiss: #12008 |
This comment was marked as outdated.
This comment was marked as outdated.
|
Thanks for the images, but I'm having a hard time understanding the behavior difference. The change is released in chrome canary with experimental web platform features enabled: chrome://flags/#enable-experimental-web-platform-features Do you think you could test it out and share a video? Or give a list of steps I can read to repro the change in behavior? |
|
Thank you, I tested with the Experimental Web Platform features enabled, and everything works fine for me: But I’m having a hard time understanding from the spec how, with just the click target, it is able to differentiate the various cases. For example, if I click-drag from a popover to Thank you. |
|
Ah good point I messed this up in the process of rewriting the chromium patch several times, I'll update this and the pointerevents spec PR soon |
|
Ok it should have substantially more detail now, please take a look. I also updated the pointerevents spec PR accordingly |
|
Looks good to me! |
| <span>dialog pointerdown target</span> to <var>ancestor</var>.</p></li> | ||
| <li><p>Let <var>pointerDownDialog</var> be the result of running <span>nearest clicked | ||
| dialog</span> given <var>pointerDownTarget</var>, <var>pointerDownX</var>, and | ||
| <var>pointerDownY</var>.</p></li> |
There was a problem hiding this comment.
It is quite unusual to do this kind of hit-testing like operation "long" after the actual event dispatch. The position of pointerDownTarget may very well have moved after pointerdown but before pointerup. Is there some particular reason for this or could this have worked if the algorithm was called for pointerdown and we'd still store the information somewhere, and then when we're about to dispatch click right after pointerup, we'd use that information.
I'm not super concerned about this though, mostly just pondering, since this feels a tiny bit like a short cut because of algorithmic simplicity. But I could be wrong. (and maybe that simplicity is ok)
There was a problem hiding this comment.
I completely agree, and this could be fixed by having the pointerevents spec pass us the actual ::backdrop pseudo-element that was clicked on in the pointerdown event instead of the parent dialog element. I actually implemented it this way at first in chromium.
I got feedback that the pointerevents spec never mentions pseudo-elements though, so it would be a large spec change to make this possible. If we can get to that point in the future though, then we could remove all of this coordinate and hit testing stuff for ::backdrop from this spec, which was already here before this PR. @flackr @mustaqahmed
Maybe we could file a pointerevents issue to follow up on this, and add a note in the spec here pointing to that issue?
There was a problem hiding this comment.
Would w3c/uievents#413 help here for the ::backdrop case?
There was a problem hiding this comment.
Yes, it certainly would! I didn't realize that pseudos were being added to uievents like that, it would be great to use them here!
|
I'm still trying to understand why we need most of these changes. Couldn't we rely on w3c/uievents#413 for the backdrop case and handling down should check that event's .buttons is 1. (that .buttons==1 check is basically what one gets from using click in pointer events spec) |
Yes, that would be great! However, I think this is a bit orthogonal because the backdrop hack we are using which this could replace is already in the HTML spec before this PR. I don't think this improvement should be blocked on waiting for that UIEvents PR to fix the backdrop hack here.
Yes, I started a change for that here but decided that this fix of using click events would be better.
Replicating click detection by looking at pointerdown/pointerup events has caused a lot of issues with touch input, such as scrolling via touch triggering light dismiss when it shouldnt. This is listed in the pointerevents issue. |
This PR replaces the existing pointerdown and pointerup listening and tracking with algorithms which use a click event. The click event is going to be used by the pointerevents spec here:
w3c/pointerevents#460
Using click events instead of doing click detection with pointerdown and pointerup fixes a number of issues which are detailed here: w3c/pointerevents#542
Fixes #10905
(See WHATWG Working Mode: Changes for more details.)
/infrastructure.html ( diff )
/interactive-elements.html ( diff )
/popover.html ( diff )