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.

Sign up for GitHub

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

Jump to bottom

Should popover events bubble? #8888

Closed
nt1m opened this issue Feb 15, 2023 · 15 comments · Fixed by web-platform-tests/wpt#38571
Closed

Should popover events bubble? #8888

nt1m opened this issue Feb 15, 2023 · 15 comments · Fixed by web-platform-tests/wpt#38571
Labels
needs tests Moving the issue forward requires someone to write tests topic: popover The popover attribute and friends

Comments

@nt1m
Copy link
Member

nt1m commented Feb 15, 2023

I don't think this is particularly clear from the spec. Since the spec just doesn't say anything I assume it's the default, which is no bubbling.

However, Chrome's implementation does make the events bubble, which I think can be useful if you're implementing context menu like functionality where want to have a common event listener for the whole document.

cc @josepharhar @mfreed7
@josepharhar
Copy link
Contributor

Yes, the events should bubble like they do in chrome's implementation.

I'm not sure if the default is to bubble or not to bubble, I'm having a hard time figuring it out by reading the algorithm in https://dom.spec.whatwg.org/#concept-event-fire

@annevk @domenic is the default to bubble? I'd be happy to update the spec if needed.

@domenic
Copy link
Member

domenic commented Feb 16, 2023

I'm not sure if the default is to bubble or not to bubble, I'm having a hard time figuring it out by reading the algorithm in https://dom.spec.whatwg.org/#concept-event-fire

The default is to not bubble: https://dom.spec.whatwg.org/#concept-event-create step 2 grabs all the defaults from https://dom.spec.whatwg.org/#dictdef-eventinit .

Which popover events is this issue about, specifically? In particular toggle for <details> doesn't bubble, so we should probably keep it not-bubbling for popups...

@nt1m
Copy link
Member Author

nt1m commented Feb 16, 2023

Chromium bubbles both toggle and beforetoggle events I think: https://source.chromium.org/search?q=ToggleEvent::createBubble&ss=chromium

I think bubbling for popovers can be useful. I could see a webapp with lots of menus/context menus modes that would want to have a single event listener for all of them. I don't have a strong preference though.

@domenic
Copy link
Member

domenic commented Feb 16, 2023

Yeah, I just wonder if the same reasoning applies to <details>... Also, I think you could use capture listeners for that case?

In general I don't feel confident about when we want to make events bubbling vs. non-bubbling, so that's why I'm falling back to consistency as a potential guiding factor. I vaguely recall @smaug---- having better intuitions here?

@nt1m
Copy link
Member Author

nt1m commented Feb 16, 2023

Also, I think you could use capture listeners for that case?

Ah right, I'm fine either way honestly.

@annevk
Copy link
Member

annevk commented Feb 16, 2023

I thought we had discussed that this would not bubble. We even discussed removing the attributes that defaulted correctly from the dispatch call site.

@annevk annevk added the topic: popover The popover attribute and friends label Feb 16, 2023
@smaug----
Copy link

It would be very confusing if some toggle events did bubble but some didn't. And that would likely also break sites which use details elements.
Toggle event was discussed also in #1533

@mfreed7
Copy link
Contributor

mfreed7 commented Feb 17, 2023

I was initially thinking the same way that @nt1m (and the folks in #1533) were, that it would be handy to have it bubble. (Indeed that's why I made it bubble in Chromium.) But I can see the confusion arguments, especially with nested popovers. And there's an easy way out using capture. So +1 to changing this behavior to disable bubbling.

Just to make sure: we're talking about both the toggle and beforetoggle events, correct? That would make the most sense.

@smaug----
Copy link

Yes, they need to be consistent with each others.

@annevk
Copy link
Member

annevk commented Feb 17, 2023

Let's close this issue once the tests are updated.

@annevk annevk added the needs tests Moving the issue forward requires someone to write tests label Feb 17, 2023
@mfreed7
Copy link
Contributor

mfreed7 commented Feb 17, 2023

Let's close this issue once the tests are updated.

I'll take care of that now...

Merged
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 17, 2023
@mfreed7 @chromium-wpt-export-bot
Stop bubbling for popover toggle events
47e389e 
Per the discussion at [1], this was an oversight and popover toggle
events (`beforetoggle` and `toggle`) should not bubble.

[1] whatwg/html#8888

Bug: 1307772
Change-Id: I29e286a6f9a8bcd1d725fa85e200a388bde0323c
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 17, 2023
@mfreed7 @chromium-wpt-export-bot
Stop bubbling for popover toggle events
1e6a5b9 
Per the discussion at [1], this was an oversight and popover toggle
events (`beforetoggle` and `toggle`) should not bubble.

[1] whatwg/html#8888

Closes: whatwg/html#8888

Bug: 1307772
Change-Id: I29e286a6f9a8bcd1d725fa85e200a388bde0323c
aarongable pushed a commit to chromium/chromium that referenced this issue Feb 17, 2023
@mfreed7
Stop bubbling for popover toggle events
83e40ab 
Per the discussion at [1], this was an oversight and popover toggle
events (`beforetoggle` and `toggle`) should not bubble.

[1] whatwg/html#8888

Closes: whatwg/html#8888

Bug: 1307772
Change-Id: I29e286a6f9a8bcd1d725fa85e200a388bde0323c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4263380
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1106907}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 17, 2023
@mfreed7 @chromium-wpt-export-bot
Stop bubbling for popover toggle events
0a39c5a 
Per the discussion at [1], this was an oversight and popover toggle
events (`beforetoggle` and `toggle`) should not bubble.

[1] whatwg/html#8888

Closes: whatwg/html#8888

Bug: 1307772
Change-Id: I29e286a6f9a8bcd1d725fa85e200a388bde0323c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4263380
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1106907}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 17, 2023
@mfreed7 @chromium-wpt-export-bot
Stop bubbling for popover toggle events
fec55c3 
Per the discussion at [1], this was an oversight and popover toggle
events (`beforetoggle` and `toggle`) should not bubble.

[1] whatwg/html#8888

Closes: whatwg/html#8888

Bug: 1307772
Change-Id: I29e286a6f9a8bcd1d725fa85e200a388bde0323c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4263380
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1106907}
@mfreed7
Copy link
Contributor

mfreed7 commented Feb 17, 2023

Closed via web-platform-tests/wpt#38571.

@mfreed7 mfreed7 closed this as completed Feb 17, 2023
@domenic
Copy link
Member

domenic commented Feb 18, 2023

Can we add tests specifically that event.bubbles === false? I didn't see that in the linked PR.

@mfreed7
Copy link
Contributor

mfreed7 commented Feb 21, 2023

Can we add tests specifically that event.bubbles === false? I didn't see that in the linked PR.

Yep, I can definitely add that. I agree it is missing.

@nt1m nt1m mentioned this issue Feb 21, 2023
Closed
@nt1m nt1m mentioned this issue Feb 22, 2023
Merged
webkit-early-warning-system pushed a commit to nt1m/WebKit that referenced this issue Feb 22, 2023
@nt1m
[popover] Stop bubbling beforetoggle events
06cfbc9 
https://bugs.webkit.org/show_bug.cgi?id=252692
rdar://105764253

Reviewed by Aditya Keerthi.

They should not bubble per whatwg/html#8888

* LayoutTests/imported/w3c/web-platform-tests/html/semantics/popovers/popover-beforetoggle-opening-event.html:
Confirmed that the test throws when changing `assert_false` to `assert_true`.

* Source/WebCore/dom/ToggleEvent.cpp:
(WebCore::ToggleEvent::ToggleEvent):

Canonical link: https://commits.webkit.org/260661@main
Merged
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 22, 2023
@mfreed7 @chromium-wpt-export-bot
Update popover events test to check bubbles/cancelable
ec6be79 
Per a request [1], this adds a few more checks to ensure the toggle
events have the right values for bubbles and cancelable.

[1] whatwg/html#8888 (comment)

Bug: 1307772
Change-Id: I8f3116703bbcb9dbc98df9be25c1fa03b7c2b97c
aarongable pushed a commit to chromium/chromium that referenced this issue Feb 22, 2023
@mfreed7
Update popover events test to check bubbles/cancelable
b5cb303 
Per a request [1], this adds a few more checks to ensure the toggle
events have the right values for bubbles and cancelable.

[1] whatwg/html#8888 (comment)

Bug: 1307772
Change-Id: I8f3116703bbcb9dbc98df9be25c1fa03b7c2b97c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4278524
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1108470}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 22, 2023
@mfreed7 @chromium-wpt-export-bot
Update popover events test to check bubbles/cancelable
4f76b5c 
Per a request [1], this adds a few more checks to ensure the toggle
events have the right values for bubbles and cancelable.

[1] whatwg/html#8888 (comment)

Bug: 1307772
Change-Id: I8f3116703bbcb9dbc98df9be25c1fa03b7c2b97c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4278524
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1108470}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 22, 2023
@mfreed7 @chromium-wpt-export-bot
Update popover events test to check bubbles/cancelable
5f99590 
Per a request [1], this adds a few more checks to ensure the toggle
events have the right values for bubbles and cancelable.

[1] whatwg/html#8888 (comment)

Bug: 1307772
Change-Id: I8f3116703bbcb9dbc98df9be25c1fa03b7c2b97c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4278524
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1108470}
@mfreed7
Copy link
Contributor

mfreed7 commented Feb 22, 2023

Can we add tests specifically that event.bubbles === false? I didn't see that in the linked PR.

Landed: https://github.com/web-platform-tests/wpt/pull/38657/files

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 1, 2023
@mfreed7 @moz-wptsync-bot
Bug 1817482 [wpt PR 38571] - Stop bubbling for popover toggle events,…
1d23bc8 
… a=testonly

Automatic update from web-platform-tests
Stop bubbling for popover toggle events

Per the discussion at [1], this was an oversight and popover toggle
events (`beforetoggle` and `toggle`) should not bubble.

[1] whatwg/html#8888

Closes: whatwg/html#8888

Bug: 1307772
Change-Id: I29e286a6f9a8bcd1d725fa85e200a388bde0323c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4263380
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1106907}

--

wpt-commits: fec55c3d5937edb83f2b95ee5137669f7781124b
wpt-pr: 38571
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Mar 1, 2023
@mfreed7 @moz-wptsync-bot
Bug 1817482 [wpt PR 38571] - Stop bubbling for popover toggle events,…
a7f44bd 
… a=testonly

Automatic update from web-platform-tests
Stop bubbling for popover toggle events

Per the discussion at [1], this was an oversight and popover toggle
events (`beforetoggle` and `toggle`) should not bubble.

[1] whatwg/html#8888

Closes: whatwg/html#8888

Bug: 1307772
Change-Id: I29e286a6f9a8bcd1d725fa85e200a388bde0323c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4263380
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1106907}

--

wpt-commits: fec55c3d5937edb83f2b95ee5137669f7781124b
wpt-pr: 38571
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 2, 2023
@mfreed7 @moz-wptsync-bot
Bug 1817482 [wpt PR 38571] - Stop bubbling for popover toggle events,…
9ac20f4 
… a=testonly

Automatic update from web-platform-tests
Stop bubbling for popover toggle events

Per the discussion at [1], this was an oversight and popover toggle
events (`beforetoggle` and `toggle`) should not bubble.

[1] whatwg/html#8888

Closes: whatwg/html#8888

Bug: 1307772
Change-Id: I29e286a6f9a8bcd1d725fa85e200a388bde0323c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4263380
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1106907}

--

wpt-commits: fec55c3d5937edb83f2b95ee5137669f7781124b
wpt-pr: 38571
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 7, 2023
@mfreed7 @moz-wptsync-bot
Bug 1818303 [wpt PR 38657] - Update popover events test to check bubb…
d4ef12c 
…les/cancelable, a=testonly

Automatic update from web-platform-tests
Update popover events test to check bubbles/cancelable

Per a request [1], this adds a few more checks to ensure the toggle
events have the right values for bubbles and cancelable.

[1] whatwg/html#8888 (comment)

Bug: 1307772
Change-Id: I8f3116703bbcb9dbc98df9be25c1fa03b7c2b97c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4278524
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1108470}

--

wpt-commits: 5f995902afeb84dcc1f656f2c23f5dc6547c6565
wpt-pr: 38657
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this issue Mar 28, 2023
@mfreed7 @marcoscaceres
Stop bubbling for popover toggle events
6083867 
Per the discussion at [1], this was an oversight and popover toggle
events (`beforetoggle` and `toggle`) should not bubble.

[1] whatwg/html#8888

Closes: whatwg/html#8888

Bug: 1307772
Change-Id: I29e286a6f9a8bcd1d725fa85e200a388bde0323c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4263380
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1106907}
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this issue Mar 28, 2023
@mfreed7 @marcoscaceres
Update popover events test to check bubbles/cancelable
249e378 
Per a request [1], this adds a few more checks to ensure the toggle
events have the right values for bubbles and cancelable.

[1] whatwg/html#8888 (comment)

Bug: 1307772
Change-Id: I8f3116703bbcb9dbc98df9be25c1fa03b7c2b97c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4278524
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1108470}
aosmond pushed a commit to aosmond/gecko that referenced this issue May 18, 2023
@mfreed7 @moz-wptsync-bot
Bug 1818303 [wpt PR 38657] - Update popover events test to check bubb…
e64c109 
…les/cancelable, a=testonly

Automatic update from web-platform-tests
Update popover events test to check bubbles/cancelable

Per a request [1], this adds a few more checks to ensure the toggle
events have the right values for bubbles and cancelable.

[1] whatwg/html#8888 (comment)

Bug: 1307772
Change-Id: I8f3116703bbcb9dbc98df9be25c1fa03b7c2b97c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4278524
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Auto-Submit: Mason Freed <masonf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1108470}

--

wpt-commits: 5f995902afeb84dcc1f656f2c23f5dc6547c6565
wpt-pr: 38657
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
needs tests Moving the issue forward requires someone to write tests topic: popover The popover attribute and friends
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Stop bubbling for popover toggle events web-platform-tests/wpt
6 participants
@domenic @annevk @nt1m @josepharhar @smaug---- @mfreed7