.
<rdar://problem/123781149>
Created attachment 470088 [details] Patch
Created attachment 470097 [details] Patch
Created attachment 470328 [details] Patch
Comment on attachment 470328 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=470328&action=review > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:423 > + if (element->isInUserAgentShadowTree() && element->userAgentPart() == UserAgentParts::webkitDatetimeEditMeridiemField()) { > + // The meridiem field of a datetime control is currently exposed as a button in order for VO to announce its value correctly. > + // It cannot be a SpinButton like other datetime fields because it is not numeric, i.e., its values are textual. > + // FIXME: This should be changed to a meaningful role that better reflects the nature of this UI component, i.e., a spin button with textual values that can be set with up/down arrows or first character. > + return AccessibilityRole::Button; > + } Would it work to call element->setAttribute("role", "button") in `DateTimeMeridiemFieldElement::create`? That way we wouldn't pay the cost for doing this check for every div on the page. > Source/WebCore/html/shadow/DateTimeFieldElements.cpp:-149 > - element->setAttributeWithoutSynchronization(HTMLNames::aria_labelAttr, AtomString { AXTimeFieldMeridiemText() }); Is AXTimeFieldMeridiemText() unused after this patch?
Created attachment 470341 [details] Patch
(In reply to Tyler Wilcock from comment #5) > Comment on attachment 470328 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=470328&action=review > > > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:423 > > + if (element->isInUserAgentShadowTree() && element->userAgentPart() == UserAgentParts::webkitDatetimeEditMeridiemField()) { > > + // The meridiem field of a datetime control is currently exposed as a button in order for VO to announce its value correctly. > > + // It cannot be a SpinButton like other datetime fields because it is not numeric, i.e., its values are textual. > > + // FIXME: This should be changed to a meaningful role that better reflects the nature of this UI component, i.e., a spin button with textual values that can be set with up/down arrows or first character. > > + return AccessibilityRole::Button; > > + } > > Would it work to call element->setAttribute("role", "button") in > `DateTimeMeridiemFieldElement::create`? That way we wouldn't pay the cost > for doing this check for every div on the page. I went back and forth between these two changes, and the main reason to go this way was not to be dependent on ARIA roles. Nevertheless, since this is a temporary fix in my view, I went back to specifying the role via the HTML role attribute in the last revision. > > > Source/WebCore/html/shadow/DateTimeFieldElements.cpp:-149 > > - element->setAttributeWithoutSynchronization(HTMLNames::aria_labelAttr, AtomString { AXTimeFieldMeridiemText() }); > > Is AXTimeFieldMeridiemText() unused after this patch? Fixed. We should do a deeper cleanup of these cause I think there is plenty of unused ones.
Comment on attachment 470341 [details] Patch Sorry, I didn't r+ this, was hoping to discuss more. I still don't understand why this div needs any role for the text within to be readable by VoiceOver. Also, won't VoiceOver read this as a button now, which might be confusing since I assume it doesn't actually act like a button?
(In reply to Tyler Wilcock from comment #8) > Comment on attachment 470341 [details] > Patch > > Sorry, I didn't r+ this, was hoping to discuss more. I still don't > understand why this div needs any role for the text within to be readable by > VoiceOver. Also, won't VoiceOver read this as a button now, which might be > confusing since I assume it doesn't actually act like a button? If we don't set a role, VO will announce it as a group. Button is the best tradeoff with existing roles. We will need to add a new role or extend spin button to handle non-numeric values. I don't think that is in the scope of this change, we should do that as a separate patch.
Created attachment 470360 [details] Patch
Created attachment 470361 [details] Patch
Committed 276195@main (bc6637d9eddb): <https://commits.webkit.org/276195@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 470361 [details].