Bug 270243 – AX: VoiceOver doesn't speak the value of the meridiem component in time controls.

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Andres Gonzalez]

.

[Radar WebKit Bug Importer]

<rdar://problem/123781149>

[Andres Gonzalez]

Created attachment 470088 [details]
Patch

[Andres Gonzalez]

Created attachment 470097 [details]
Patch

[Andres Gonzalez]

Created attachment 470328 [details]
Patch

[Tyler Wilcock]

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?

[Andres Gonzalez]

Created attachment 470341 [details]
Patch

[Andres Gonzalez]

(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.

[Tyler Wilcock]

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?

[Andres Gonzalez]

(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.

[Tyler Wilcock]

Created attachment 470360 [details]
Patch

[Tyler Wilcock]

Created attachment 470361 [details]
Patch

[EWS]

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].