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 pop-up light dismiss logic to event dispatching #1117

Closed
wants to merge 2 commits into from

Conversation

josepharhar
Copy link
Contributor

@josepharhar josepharhar commented Oct 10, 2022

This was moved from the HTML spec PR for the popup attribute based on this advice: whatwg/html#8221 (comment)
TODO add a better description

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

dom.bs Outdated
Comment on lines 1396 to 1397
<li><p>Run {light dismiss open pop-ups} given <var>struct</var>'s <a
for=Event/path>invocation target</a>.
Copy link
Member

@annevk annevk Oct 18, 2022

Choose a reason for hiding this comment

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

This needs some kind of note explaining the setup. This also seems like a layering violation. Is this really supposed to happen for all events or just certain UI events? Is it supposed to happen for synthetic events or just events dispatched by the user agent?

(If you rebase and force push the CI error will go away.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This also seems like a layering violation.

Is HTML supposed to refer to DOM but not the other way around? I noticed that this spec refers to HTMLSlotElement from the HTML spec...

Is this really supposed to happen for all events or just certain UI events?

Yes, it really happens for all events, at least in the chromium implementation.

(If you rebase and force push the CI error will go away.)

done

Copy link
Member

Choose a reason for hiding this comment

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

It seems to me that the implementation only does something for mouse and keyboard input. Although maybe also for synthetic mouse and keyboard input? It still strikes me as somewhat wrong to have it as part of event dispatch. What's the design motivation for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There has been some discussion about the constraints of when to actually light dismiss based on the event here, hopefully the new changes address this concern: whatwg/html#7785 (comment)

It seems to me that the implementation only does something for mouse and keyboard input.

Do you mean that it should only work if the event is trusted? The implementation does not currently check isTrusted, and mason's new patch doesn't do so either.

Although maybe also for synthetic mouse and keyboard input? It still strikes me as somewhat wrong to have it as part of event dispatch. What's the design motivation for that?

I don't know what the design motivation was, but if you have any alternative ideas I'm all ears

@annevk
Copy link
Member

annevk commented Nov 11, 2022

I think this can be closed now right and instead closing of popovers happens at a higher-level?

@domenic
Copy link
Member

domenic commented Nov 11, 2022

My understanding is that this is still needed for pointerdown/pointerup-based dismissal? https://whatpr.org/html/8221/popover.html#popover-light-dismiss

@annevk
Copy link
Member

annevk commented Nov 11, 2022

I see, so why don't have this logic as part of the code that dispatches those events? Why does it need to be embedded in the dispatch logic of all events?

@josepharhar
Copy link
Contributor Author

why don't have this logic as part of the code that dispatches those events?

Good idea, I'll try moving this to the code that creates pointer events and see what happens

josepharhar added a commit to josepharhar/pointerevents that referenced this pull request 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
@josepharhar
Copy link
Contributor Author

I have opened a PR on pointerevents here: w3c/pointerevents#460
The chromium patch seems to be working ok: https://chromium-review.googlesource.com/c/chromium/src/+/4023021
I will tentatively close this PR since the pointerevents PR replaces this.

aarongable pushed a commit to chromium/chromium that referenced this pull request Nov 29, 2022
This patch also moves the keyboard event handling directly to the
keyboard event manager, where the same logic for dialog elements and
CloseWatcher lives.

This was recommended in my spec reviews:
whatwg/dom#1117 (comment)

Bug: 1307772
Change-Id: Idcfdefed4c783d270ed4ca494c54fee70c444914
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4023021
Commit-Queue: Joey Arhar <[email protected]>
Reviewed-by: Mason Freed <[email protected]>
Auto-Submit: Joey Arhar <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1077035}
webkit-commit-queue pushed a commit to rwlbuis/WebKit that referenced this pull request Mar 29, 2023
https://bugs.webkit.org/show_bug.cgi?id=254384

Reviewed by Tim Nguyen.

Move light dismiss logic to PointerCaptureController::dispatchEvent so the event can't
be cancelled, also see:
whatwg/dom#1117 (comment)

* LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/light-dismiss-event-ordering-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-light-dismiss-expected.txt:
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/html/semantics/popovers/popover-light-dismiss-expected.txt:
* Source/WebCore/dom/Document.cpp:
(WebCore::Document::handlePopoverLightDismiss):
* Source/WebCore/dom/Document.h:
* Source/WebCore/dom/Node.cpp:
(WebCore::Node::defaultEventHandler):
* Source/WebCore/page/PointerCaptureController.cpp:
(WebCore::PointerCaptureController::pointerEventWillBeDispatched):

Canonical link: https://commits.webkit.org/262283@main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants