-
Notifications
You must be signed in to change notification settings - Fork 26
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
Conversation
this commit supersedes #446 TODO: finalize `popover` attribute mappings
…state to assistive technologies https://bugs.webkit.org/show_bug.cgi?id=257666 rdar://105425310 Reviewed by Chris Fleizach. Per w3c/html-aam#481, buttons with the `popovertarget` attribute and valid associated popover should expose expanded state to assistive technologies. This commit implements that, and also submits a notification to ATs when a popover is expanded and collapsed. * LayoutTests/accessibility/mac/expanded-notification-expected.txt: * LayoutTests/accessibility/mac/expanded-notification.html: Popover testcase added. * Source/WebCore/accessibility/AXObjectCache.cpp: (WebCore::AXObjectCache::onPopoverTargetToggle): * Source/WebCore/accessibility/AXObjectCache.h: * Source/WebCore/accessibility/AccessibilityNodeObject.cpp: (WebCore::AccessibilityNodeObject::popoverTargetElement const): * Source/WebCore/accessibility/AccessibilityNodeObject.h: * Source/WebCore/accessibility/AccessibilityObject.cpp: (WebCore::AccessibilityObject::supportsExpanded const): (WebCore::AccessibilityObject::isExpanded const): * Source/WebCore/accessibility/AccessibilityObject.h: (WebCore::AccessibilityObject::popoverTargetElement const): * Source/WebCore/html/HTMLFormControlElement.cpp: (WebCore::HTMLFormControlElement::handlePopoverTargetAction const): Canonical link: https://commits.webkit.org/264852@main
…state to assistive technologies https://bugs.webkit.org/show_bug.cgi?id=257666 rdar://105425310 Reviewed by Chris Fleizach. Per w3c/html-aam#481, buttons with the `popovertarget` attribute and valid associated popover should expose expanded state to assistive technologies. This commit implements that, and also submits a notification to ATs when a popover is expanded and collapsed. * LayoutTests/accessibility/mac/expanded-notification-expected.txt: * LayoutTests/accessibility/mac/expanded-notification.html: Popover testcase added. * Source/WebCore/accessibility/AXObjectCache.cpp: (WebCore::AXObjectCache::onPopoverTargetToggle): * Source/WebCore/accessibility/AXObjectCache.h: * Source/WebCore/accessibility/AccessibilityNodeObject.cpp: (WebCore::AccessibilityNodeObject::popoverTargetElement const): * Source/WebCore/accessibility/AccessibilityNodeObject.h: * Source/WebCore/accessibility/AccessibilityObject.cpp: (WebCore::AccessibilityObject::supportsExpanded const): (WebCore::AccessibilityObject::isExpanded const): * Source/WebCore/accessibility/AccessibilityObject.h: (WebCore::AccessibilityObject::popoverTargetElement const): * Source/WebCore/html/HTMLFormControlElement.cpp: (WebCore::HTMLFormControlElement::handlePopoverTargetAction const): Canonical link: https://commits.webkit.org/264852@main
made related follow up #505 |
index.html
Outdated
<tr> | ||
<th><a href="https://msdn.microsoft.com/en-us/library/dd373608%28v=VS.85%29.aspx">MSAA</a> + <a href="http://accessibility.linuxfoundation.org/a11yspecs/ia2/docs/html/">IAccessible2</a></th> | ||
<td> | ||
<span class="type">Relations:</span> `IA2_RELATION_DETAILS_FOR` points to invoking element |
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.
index.html
Outdated
<tr> | ||
<th><a href="https://msdn.microsoft.com/en-us/library/dd373608%28v=VS.85%29.aspx">MSAA</a> + <a href="http://accessibility.linuxfoundation.org/a11yspecs/ia2/docs/html/">IAccessible2</a></th> | ||
<td> | ||
<span class="type">Relations:</span> `IA2_RELATION_DETAILS_FOR` points to invoking element |
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.
</p> | ||
<p> | ||
If the associated element is an accessibility ancestor of the element with the `popovertarget` attribute: | ||
<a class="core-mapping" href="#ariaExpandedUndefined">`aria-expanded=undefined`</a> |
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?
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
closing - will be merged via w3c/aria#2219 |
this commit supersedes #446
TODO: finalize
popover
attribute mappingsPreview | Diff