Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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/revise popover related attribute mappings #481
add/revise popover related attribute mappings #481
Changes from 11 commits
39ae862
e4694e5
9eed4ec
5fde9c0
cbd0cc6
4cad167
e64eaae
0a3851e
5829073
75d44ff
4691b00
ed8270a
82b42e3
2b36d6f
bfa65de
bcdb24e
beccc2c
ce1773d
7ed1441
1fde1de
bf0a2fe
fd42971
5e31f10
44d2432
71c0c0b
fcdd9ac
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It feels weird that we express this as a platform specific relation, but we express popovertarget's details relation as a cross-platform aria-details relation. Is this just because we have no way of expressing reverse relations for ARIA mappings and we want to be explicit here? I wonder whether it'd be better to link to a more general note about implicit reverse relations or something like that.
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'd be fine with either replacing the cross-platform aria-details relation with the specific platform mappings, or keeping the cross-platform relation and removing the platform specific and replacing with a note about the reverse relations.
over in #359 (which incase you missed, i pinged you for a review) we use the platform specific reverse relations for figure/figcaption. So we can do that here too, or similarly change over in that PR.
Whatever makes the most sense for those that need to read the spec is what I want to do, so just let me know which is preferred.
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 assume this shouldn't happen if we don't expose aria-details due to the invoking element being an immediate sibling or having popovertargetaction=hide? Again, hopefully common sense will prevail, but probably best not to rely on that.
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 doesn't appear to be implemented in Chromium:
data:text/html,<button popovertarget="pop">tog</button><div id="pop" popover>pop<button popovertarget="pop" popovertargetaction="hide">hide
When the popover is shown, the "hide" button gets the expanded state. If I'm reading this correctly, it shouldn't.
Just to double check, is this definitely what we want and it just hasn't been done in Chromium yet? Or is this still an open question we need to sort out in this PR?
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 it should not be getting the expanded state for the hide button, this will need to be adjusted in chromium
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.
Have we filed a bug for this?