-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
dispatch toggleevents for dialog open/close #10091
base: main
Are you sure you want to change the base?
dispatch toggleevents for dialog open/close #10091
Conversation
I think this needs the "toggle task tracker" concept used in details elements in order to coalesce async toggle events: https://html.spec.whatwg.org/multipage/interactive-elements.html#queue-a-details-toggle-event-task If you do this and open+close the dialog element synchronously , there should be one "toggle" event with both oldState and newState set to "closed": dialog.showModal();
dialog.close(); |
Consider chrome as tentatively interested. I will file an intent to ship to get an official position when this gets a bit more attention from the other browsers, but I don't anticipate objections |
Also Gecko is interested assuming the pr follows what was discussed at TPAC. |
Tests can be added to the OP: web-platform-tests/wpt#44208 |
Hi, this implementation could help us improve a popup, such as a CMP, for better integration by utilizing native browser APIs, specifically the Popover API, to make everything more robust. This will enable us to further improve the INP impact, as described in my case study here: https://web.dev/case-studies/pubconsent-inp?hl=en. Since the CMP is a predominant component on the web, I believe this kind of implementation should be supported as much as possible. I hope it helps. Thank you |
@annevk would you have time to review this or otherwise delegate it out to another editor for review? |
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.
Maybe @nt1m or @josepharhar is interested in reviewing this?
<li><p>Set <var>element</var>'s <span>dialog toggle task tracker</span> to a struct with <span | ||
data-x="toggle-task-task">task</span> set to the just-queued <span | ||
data-x="concept-task">task</span> and <span data-x="toggle-task-old-state">old state</span> set | ||
to <var>oldState</var>.</p></li> |
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.
Just-queued is a lil vague. But I guess we have no alternative?
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.
I cribbed this from the popover part of the spec which does the same thing. Happy to look at alternatives.
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.
Yeah if all the task queueing algorithms returned the task they create then this would be better. Maybe that could be done as a follow up and we could fix up this part and the identical popover one?
whatwg/html#10091 This also slightly refactors FireToggleEvent to allow to to accomodate both popovers and now also dialogs Differential Revision: https://phabricator.services.mozilla.com/D225449 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1876762 gecko-commit: a8b51403fa4f1dd6d936826f2a5b14c670426f3d gecko-reviewers: smaug
…e r=smaug whatwg/html#10091 This also slightly refactors FireToggleEvent to allow to to accomodate both popovers and now also dialogs Differential Revision: https://phabricator.services.mozilla.com/D225449
whatwg/html#10091 This also slightly refactors FireToggleEvent to allow to to accomodate both popovers and now also dialogs Differential Revision: https://phabricator.services.mozilla.com/D225449 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1876762 gecko-commit: a8b51403fa4f1dd6d936826f2a5b14c670426f3d gecko-reviewers: smaug
…e r=smaug whatwg/html#10091 This also slightly refactors FireToggleEvent to allow to to accomodate both popovers and now also dialogs Differential Revision: https://phabricator.services.mozilla.com/D225449 UltraBlame original commit: a8b51403fa4f1dd6d936826f2a5b14c670426f3d
…e r=smaug whatwg/html#10091 This also slightly refactors FireToggleEvent to allow to to accomodate both popovers and now also dialogs Differential Revision: https://phabricator.services.mozilla.com/D225449 UltraBlame original commit: a8b51403fa4f1dd6d936826f2a5b14c670426f3d
…e r=smaug whatwg/html#10091 This also slightly refactors FireToggleEvent to allow to to accomodate both popovers and now also dialogs Differential Revision: https://phabricator.services.mozilla.com/D225449 UltraBlame original commit: a8b51403fa4f1dd6d936826f2a5b14c670426f3d
source
Outdated
@@ -61030,6 +61101,7 @@ interface <dfn interface>HTMLDialogElement</dfn> : <span>HTMLElement</span> { | |||
document</span>.</p></li> | |||
|
|||
<li><p>Run the <span>dialog focusing steps</span> given <span>this</span>.</p></li> | |||
|
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.
Remove this newline?
lgtm, I don't see any differences between this spec and the chromium implementation |
…e r=smaug whatwg/html#10091 This also slightly refactors FireToggleEvent to allow to to accomodate both popovers and now also dialogs Differential Revision: https://phabricator.services.mozilla.com/D225449
This attempts to fix #9733 by specifying that:
show()
steps should dispatch abeforetoggle
cancellable eventtoggle
event.showModal()
steps should dispatch abeforetoggle
cancellable eventtoggle
event.close the dialog
steps should disaptch a non-cancellablebeforetoggle
eventtoggle
event.(See WHATWG Working Mode: Changes for more details.)
/interactive-elements.html ( diff )