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

[popup] Consider to add popupshowing, popupshown, popuphiding and popuphidden events #342

Closed
smaug---- opened this issue Apr 27, 2021 · 14 comments
Labels
agenda+ Use this label if you'd like the topic to be added to the meeting agenda needs edits This is ready for edits to be made open-ui-resolved-accepted popover The Popover API

Comments

@smaug----
Copy link

The proposed popup elements shares lots of common with XUL popup element (which is used in Firefox UI to create all sorts of popups). Very common way to use a popup is to fill its content just when it is about to be shown. That is why there is popupshowing DOM event. Other commonly used events are popupshown, popuphiding and popuphidden.

(There is also popuppositioned event.)

@melanierichards melanierichards added the popover The Popover API label May 5, 2021
@una una added the agenda+ Use this label if you'd like the topic to be added to the meeting agenda label May 12, 2021
@gregwhitworth
Copy link
Member

This was discussed in this week's meeting with the following resolution:

RESOLUTION: Events should be emitted when show or hide state changes (i.e. beforeShow, show, beforeHide, hide). Event timing and names to be defined.

Minutes are here

@gregwhitworth gregwhitworth added needs edits This is ready for edits to be made open-ui-resolved-accepted and removed agenda+ Use this label if you'd like the topic to be added to the meeting agenda labels May 13, 2021
@github-actions
Copy link

There hasn't been any discussion on this issue for a while, so we're marking it as stale. If you choose to kick off the discussion again, we'll remove the 'stale' label.

@mfreed7
Copy link
Collaborator

mfreed7 commented Jul 14, 2022

The pop-up API has evolved significantly since this question was raised. In particular, the show and hide events have been more fleshed out as synchronous events, which allows a pop-up to have its content updated prior to being shown, and/or retrieve data from the pop-up prior to being hidden. Also, the animation behavior has been resolved, allowing easy animations of pop-up show and hide. I'm curious what use cases are still left out of the existing proposal that might require more events than just show and hide. Thoughts?

@github-actions github-actions bot removed the stale label Jul 15, 2022
@mfreed7
Copy link
Collaborator

mfreed7 commented Aug 4, 2022

I'm inclined to close this issue, given that I believe (subject to being told I'm wrong!) that the current proposal functionality covers all relevant use cases.

@mfreed7 mfreed7 closed this as completed Aug 4, 2022
@mfreed7
Copy link
Collaborator

mfreed7 commented Dec 8, 2022

Re-opening to let @smaug---- comment with some use cases.

@mfreed7 mfreed7 reopened this Dec 8, 2022
@smaug----
Copy link
Author

From a meeting notes:

  • Popupshown (fired after the popup is already visible) is used for example to implement autohide after some time. And possibly more important use case is to focus certain element in the popup.
  • Popuphiding isn’t used too often and popuphidden could have been used in many places.
  • Popuppositioned fires if the position or size changes

I think some variant of popupshown is pretty critical, especially because of the focus handling.

@mfreed7
Copy link
Collaborator

mfreed7 commented Dec 14, 2022

  • Popupshown (fired after the popup is already visible) is used for example to implement autohide after some time.

This use case could just as easily be done in the "beforetoggle" event, right? I.e. this is some variant of setTimeout(doAutohide,500), which works before or after.

And possibly more important use case is to focus certain element in the popup.

For focusing elements, we have the autofocus attribute, which declaratively does this in exactly the right way (for a11y) at exactly the right time. I'd be hesitant to encourage developers to manually call .focus() in event handlers, since it would be easy to do it wrong and hurt user experience. @scottaohara

  • Popuphiding isn’t used too often and popuphidden could have been used in many places.

I guess this is an argument that either can work?

  • Popuppositioned fires if the position or size changes

This last bit is related more to anchor positioning, I think. Doesn't the ResizeObserver or IntersectionObserver take care of this use case though?

I think some variant of popupshown is pretty critical, especially because of the focus handling.

We have discussed this question several times, not just in this issue, but also in #607 (comment), and more when I proposed it as a more general WHATWG issue. There haven't been compelling developer-driven arguments that there's a use case for the "after" events. Or at least none have emerged thus far in all of the testing and demos for Popover.

Let's discuss further - I've added this to the agenda for a future meeting (will be next year).

@mfreed7 mfreed7 added the agenda+ Use this label if you'd like the topic to be added to the meeting agenda label Dec 14, 2022
@css-meeting-bot
Copy link

The Open UI Community Group just discussed [popup] Consider to add popupshowing, popupshown, popuphiding and popuphidden events.

The full IRC log of that discussion <una> masonf: Ollie from Mozilla requested this be reopen
<una> masonf: issue is why don't we have one event before and one after, but I'd like to get Ollie to be able to present this, so would be happy to wait
<dbaron> s/Ollie/Olli/
<una> Action: will check if this is a time Olli (smaug----) can make before further discussion

@dbaron
Copy link
Collaborator

dbaron commented Jan 5, 2023

@smaug---- We were curious if you'd be able to make a future OpenUI meeting to present/discuss this. They're currently Thursdays at 14:00 New York / 11:00 Los Angeles, which (except during the weird summer time weeks) is I think 21:00 Helsinki.

@smaug----
Copy link
Author

Sure, 21:00 should be fine.

@mfreed7
Copy link
Collaborator

mfreed7 commented Jan 9, 2023

Sure, 21:00 should be fine.

Great! @gregwhitworth can we please make sure this issue gets back on the agenda for this week Thursday so @smaug---- can participate?

@css-meeting-bot
Copy link

The Open UI Community Group just discussed [popup] Consider to add popupshowing, popupshown, popuphiding and popuphidden events, and agreed to the following:

  • RESOLVED: Add an asynchronous `aftertoggle` event, which also uses the same ToggleEvent that `beforetoggle` uses, and which runs after the popover is fully shown or hidden.
The full IRC log of that discussion <gregwhitworth> Topic: [popup] Consider to add popupshowing, popupshown, popuphiding and popuphidden events
<gregwhitworth> github: https://github.com//issues/342
<gregwhitworth> masonf: this is an issue we briefly discussed last week since smaug asked to re-open it
<gregwhitworth> masonf: it's misnamed a bit; we have beforetoggle; it's more of a request for aftertoggle of the hide or show
<gregwhitworth> masonf: we've talked on 342 and 607 and WHATWG 8386 and in all of those cases we all agreed that there was no usecase that needs no after
<gregwhitworth> smaug: it's really only about the one event popupafter
<gregwhitworth> smaug: it's pretty important, the current API has auto focus, but what then
<gregwhitworth> smaug: what do you do if you want to focus somewhere else later
<gregwhitworth> smaug: you'll need to switch to another API since autofocus no longer works at this phase of the event
<gregwhitworth> smaug: that's my main point here
<sarah_higley> q+
<gregwhitworth> masonf: the auto focus manages it correctly but other scenarios after it is shown they'll use .focus elsewhere
<gregwhitworth> masonf: the event would be after the popup is shown the focus is moved. Mozilla Firefox actually uses this paradigm
<gregwhitworth> ack sarah_higley
<gregwhitworth> sarah_higley: I was going to add some usecases that are similar to what smaug said
<gregwhitworth> sarah_higley: when the popup is shown you want to conditionally focus something
<gregwhitworth> sarah_higley: also the hidden event would allow you to do this for when the page is no longer inert
<gregwhitworth> sarah_higley: this makes more sense for dialog but for symetry it makes sense
<masonf> Proposed resolution: Add an asynchronous `aftertoggle` event, which also uses the same ToggleEvent that `beforetoggle` uses.
<masonf> ...and which runs after the popover is fully shown or hidden.
<masonf> RESOLVED: Add an asynchronous `aftertoggle` event, which also uses the same ToggleEvent that `beforetoggle` uses, and which runs after the popover is fully shown or hidden.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 12, 2023
This CL adds an async `aftertoggle` event to both the show and hide
popover transitions, and renames the event class from BeforeToggle
to PopoverToggle, to be used by both events.

This was resolved by OpenUI here:
openui/open-ui#342 (comment)

Bug: 1307772
Change-Id: I996be74b90f43eee4bf859cecfed051fa6f633d5
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 12, 2023
This CL adds an async `aftertoggle` event to both the show and hide
popover transitions, and renames the event class from BeforeToggle
to PopoverToggle, to be used by both events.

This was resolved by OpenUI here:
openui/open-ui#342 (comment)

Bug: 1307772
Change-Id: I996be74b90f43eee4bf859cecfed051fa6f633d5
@mfreed7
Copy link
Collaborator

mfreed7 commented Jan 13, 2023

Side note: we just decided in the HTML spec triage meeting to rename aftertoggle to toggle but otherwise retain the behavior described in the resolution above.

@mfreed7 mfreed7 closed this as completed Jan 13, 2023
aarongable pushed a commit to chromium/chromium that referenced this issue Jan 13, 2023
This CL adds an async `aftertoggle` event to both the show and hide
popover transitions, and renames the event class from BeforeToggle
to PopoverToggle, to be used by both events.

This was resolved by OpenUI here:
openui/open-ui#342 (comment)

Bug: 1307772
Change-Id: I996be74b90f43eee4bf859cecfed051fa6f633d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4160443
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1092158}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 13, 2023
This CL adds an async `aftertoggle` event to both the show and hide
popover transitions, and renames the event class from BeforeToggle
to PopoverToggle, to be used by both events.

This was resolved by OpenUI here:
openui/open-ui#342 (comment)

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

annevk commented Jan 13, 2023

Side note: such decisions are non-binding: https://whatwg.org/working-mode#meetings.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 13, 2023
This CL adds an async `aftertoggle` event to both the show and hide
popover transitions, and renames the event class from BeforeToggle
to PopoverToggle, to be used by both events.

This was resolved by OpenUI here:
openui/open-ui#342 (comment)

Bug: 1307772
Change-Id: I996be74b90f43eee4bf859cecfed051fa6f633d5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4160443
Auto-Submit: Mason Freed <masonf@chromium.org>
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Mason Freed <masonf@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1092158}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jan 19, 2023
…estonly

Automatic update from web-platform-tests
Add `aftertoggle` event for popover

This CL adds an async `aftertoggle` event to both the show and hide
popover transitions, and renames the event class from BeforeToggle
to PopoverToggle, to be used by both events.

This was resolved by OpenUI here:
openui/open-ui#342 (comment)

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

--

wpt-commits: e3246afc19a62a2d4f26786e6eae72a601a2c032
wpt-pr: 37915
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Jan 20, 2023
…estonly

Automatic update from web-platform-tests
Add `aftertoggle` event for popover

This CL adds an async `aftertoggle` event to both the show and hide
popover transitions, and renames the event class from BeforeToggle
to PopoverToggle, to be used by both events.

This was resolved by OpenUI here:
openui/open-ui#342 (comment)

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

--

wpt-commits: e3246afc19a62a2d4f26786e6eae72a601a2c032
wpt-pr: 37915
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agenda+ Use this label if you'd like the topic to be added to the meeting agenda needs edits This is ready for edits to be made open-ui-resolved-accepted popover The Popover API
Projects
None yet
Development

No branches or pull requests

8 participants