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

Add popover light dismiss integration #460

Open
wants to merge 2 commits into
base: gh-pages
Choose a base branch
from

Conversation

josepharhar
Copy link

@josepharhar josepharhar commented Nov 22, 2022

I am adding a new HTML attribute called "popover" in whatwg/html#8221

Part of this feature involves "light dismiss", where clicking outside of an element with the popover attribute causes it to be closed. This functionality was originally implemented by changing the event dispatching system of the DOM, but it was recommended to move it here instead:
whatwg/dom#1117 (comment)
https://chromium-review.googlesource.com/c/chromium/src/+/4023021


💥 Error: 520 💥

PR Preview failed to build. (Last tried on Jan 27, 2023, 5:07 PM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Spec Generator - Spec Generator is the web service used to build specs that rely on ReSpec.

🔗 Related URL

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

I am adding a new HTML attribute called "popover" in
whatwg/html#8221

Part of this feature involves "light dismiss", where clicking outside of
an element with the popover attribute causes it to be closed. This
functionality was originally implemented by changing the event
dispatching system of the DOM, but it was recommended to move it here
instead:
whatwg/dom#1117 (comment)
https://chromium-review.googlesource.com/c/chromium/src/+/4023021
@patrickhlauke
Copy link
Member

Briefly discussed at today's meeting. https://www.w3.org/2022/11/23-pointerevents-minutes.html#t03

Waiting for this to stabilise before adding to PE spec.

@josepharhar
Copy link
Author

Sounds good, I'll comment again when the popover HTML PR is done

index.html Outdated Show resolved Hide resolved
@josepharhar
Copy link
Author

The HTML spec for this has been merged: https://html.spec.whatwg.org/multipage/popover.html#light-dismiss-open-popovers
Can we merge this PR now?

@patrickhlauke
Copy link
Member

@mustaqahmed @flackr @smaug---- as you're all closer to the "metal" here in terms of your technical experience/understanding, do you mind reviewing this PR to just double-check it's kosher and then merge?

@@ -409,6 +409,8 @@ <h2>Firing events using the <code>PointerEvent</code> interface</h2>
<p>If the event is {{GlobalEventHandlers/pointerdown}}, the associated device is a direct manipulation device, and the target is an {{Element}},
then <a>set pointer capture</a> for this <code>pointerId</code> to the target element as described in <a>implicit pointer capture</a>.

<p>Run <a href="html.spec.whatwg.org/#popover-light-dismiss">light dismiss open popovers</a> given the event.</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the chromium implementation, light dismiss from the escape key seems to only be invoked if the default wasn't prevented[1]. However, calling it here before the event has been fired means that this will happen regardless of whether the default was prevented for pointer events. This indeed matches the implementation but are we intentionally inconsistent? Has there been discussion about this? I put together a demo at https://jsbin.com/bakoxan/edit?html,js,output to show a page which calls preventDefault on all events after a popover or dialog is open to test the differences.

Our implementation only calls the light dismiss steps if the event is pointerdown or pointerup. I see the steps in the html spec only care about pointerdown and pointerup, but might be worth limiting this here?

I have some concerns about the algorithm in the html spec. It seems like https://html.spec.whatwg.org/#popover-light-dismiss is replicating a form of click detection, however not taking into account many of the cases in which clicks are not generated.

  • Do you really want to light dismiss popovers on the pointerup event if the finger was moved sufficiently such that a click wouldn't be generated?
  • The check of whether the down target is the same as the up target seems a bit overly naive, in complex pages it's possible for the pointer to drift slightly and be over a different target element. Should this really result in not dismissing the popover even though it still generates a click?

I wonder whether it would be better to look for click events.

Apologies if these things have already been discussed I just couldn't find them with the searching that I did.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I'm putting this here instead of in the HTML spec is that I got feedback during the review that we shouldn't be using an event listener to do the light dismiss behavior or really interacting with the event system at all, and that instead we should be going where the events are fired and instrumenting everything there. The lack of preventDefault support seems consistent with that idea.

Perhaps the escape key behavior should do the same if that is really what's preferred. I'm not sure if CloseWatcher, which should replace the current escape key implementation, imposes any changes in behavior regarding preventDefault. @domenic

It seems like https://html.spec.whatwg.org/#popover-light-dismiss is replicating a form of click detection, however not taking into account many of the cases in which clicks are not generated.

I agree, reusing the implementation of click events sounds better. @mfreed7 I believe that your initial implementation used mouseup and mousedown instead of click. Am I right? Was there any reason for that?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the escape key behavior should do the same if that is really what's preferred. I'm not sure if CloseWatcher, which should replace the current escape key implementation, imposes any changes in behavior regarding preventDefault. @domenic

CloseWatcher works per https://wicg.github.io/close-watcher/#close-signals ; note steps 2 and 3. (Also note that per discussion in whatwg/html#9132 (comment), @emilio would prefer we be more explicit about it being keydown for Esc, instead of my current proposal's "Fire any relevant event ... If multiple such events are fired, the user agent must pick one for the purposes of the following steps.")

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether it would be better to look for click events.

If we wanted to use the click event instead, should it be added here? https://w3c.github.io/uievents/#event-type-click

I think I have to find some place outside of the HTML spec to implement this, unless we can add some abstraction in the HTML spec for this kind of behavior. Potentially related: whatwg/html#10762

@dontcallmedom
Copy link
Member

This one-line PR is nearly two years old - is there any update on the discussion above that could help move it forward?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants