-
Notifications
You must be signed in to change notification settings - Fork 34
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
josepharhar
wants to merge
2
commits into
w3c:gh-pages
Choose a base branch
from
josepharhar:popover
base: gh-pages
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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