-
Notifications
You must be signed in to change notification settings - Fork 2k
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
HTMLElement toggle and before toggle event subfeatures #24927
HTMLElement toggle and before toggle event subfeatures #24927
Conversation
@@ -116,45 +116,6 @@ | |||
"deprecated": false | |||
} | |||
} | |||
}, | |||
"toggle_event": { |
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.
FYI moved the versioning data to top level HTMLElement.toggle_event. That data was added when support was added for popovers, clearly not realizing that the event is actually this one.
@@ -430,9 +430,6 @@ | |||
"description": "<code>beforetoggle</code> event", | |||
"mdn_url": "https://developer.mozilla.org/docs/Web/API/HTMLElement/beforetoggle_event", | |||
"spec_url": "https://html.spec.whatwg.org/multipage/indices.html#event-beforetoggle", | |||
"tags": [ |
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.
FYI Parent no longer restricted to popovers.
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.
Wouldn't it be an option to add the feature to both web feature groups, i.e. popover
and dialog
?
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.
Yes. Did not think of that - not very familiar with tags yet. Fixed #25095
}, | ||
"dialog_elements": { | ||
"__compat": { | ||
"description": "<code>beforetoggle</code> event fires at dialog elements", |
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.
Note, all descriptions like this because it makes them readable if I pull the subfeature into HTMLDialogElement.
I.e. This is a feature on HTMLElement that I want to show compatibility for in HTMLDialogElement, because that is where the event is fired, and it isn't fired on all of them. So when I render the compat data in HTMLDialogElement I want it to be clear that this is the event.
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.
Usually I would say that we can assume the parent feature as context, and then this description should only be "Fires at dialog elements" and both the mdn_url
and spec_url
would be omitted.
For now we can probably make an exception here, but we should eventually solve this in the visualization layer (i.e. the BCD table).
/cc @Elchi3
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.
OK. As long as it is good for now. Thank you.
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.
FYI, this is for docs PR mdn/content#36833
"chrome_android": "mirror", | ||
"edge": "mirror", | ||
"firefox": { | ||
"version_added": "133" |
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.
This is the FF133 addition of support for firing this event at dialogs. Support on Chrome imminent.
Co-authored-by: Keith Cirkel <[email protected]>
Can I please get this merged? It is correct, but it might not be "BCD optimal" |
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.
LGTM, just left two comments that we can discuss in a follow-up.
@@ -430,9 +430,6 @@ | |||
"description": "<code>beforetoggle</code> event", | |||
"mdn_url": "https://developer.mozilla.org/docs/Web/API/HTMLElement/beforetoggle_event", | |||
"spec_url": "https://html.spec.whatwg.org/multipage/indices.html#event-beforetoggle", | |||
"tags": [ |
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.
Wouldn't it be an option to add the feature to both web feature groups, i.e. popover
and dialog
?
}, | ||
"dialog_elements": { | ||
"__compat": { | ||
"description": "<code>beforetoggle</code> event fires at dialog elements", |
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.
Usually I would say that we can assume the parent feature as context, and then this description should only be "Fires at dialog elements" and both the mdn_url
and spec_url
would be omitted.
For now we can probably make an exception here, but we should eventually solve this in the visualization layer (i.e. the BCD table).
/cc @Elchi3
FF133 adds support for the HTMLElement beforetoggle and toggle events that were formerly assumed to be restricted to only being fired at popovers in https://bugzilla.mozilla.org/show_bug.cgi?id=1876762
Note, this data is confirmed by spec author/implementer in mdn/content#36536 (comment)
I have added subfeatures to these events for the elements that they can be fired on. My theory being that for HTMLDialogElement I can add the particular subfeature of HTMLElement as another compat point to show the version at which the event became relevant.
I THINK this is better than trying to somehow show the events directly supported on (say) HTMLDialogElement
It turns out though that
toggle
has been around forever on HTMLDetailsElement. However this has the same derivation and is in the spec as being defined on HTMLElement. So at some point that changed, and I don't think it is worth finding out when.In any case, to make that work, I have update the toggle in HTMLElement to match the original details element version and deleted that subfeature.
related docs work can be tracked in mdn/content#36536