AX: ARIA spin button should support range value attributes

[Dominic Mazzoni]

According to the ARIA spec, an ARIA spin button should use aria-valuenow, etc. for its value.

[Dominic Mazzoni]

Created attachment 162693 [details]
Patch

[chris fleizach]

Comment on attachment 162693 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162693&action=review

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1410
> +    if (isInputTag)

is this part of the same bug?

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3155
> +    if (m_ariaRole != UnknownRole)

is this part of the same bug?

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3394
> +    case ScrollBarRole:

should you have removed scrollbar role?

> Source/WebCore/accessibility/AccessibilityRenderObject.h:305
> +    bool isAriaRange() const;

should be isARIARange()

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:2002
> +        if (m_object->isProgressIndicator() || m_object->isSlider() || m_object->isScrollbar() || m_object->isSpinButton())

Looks like we should probably have a supportsRangeValue() method

[Dominic Mazzoni]

Created attachment 162698 [details]
Patch

[Dominic Mazzoni]

Comment on attachment 162693 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162693&action=review

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1410
>> +    if (isInputTag)
> 
> is this part of the same bug?

I can move this to a separate bug.

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3155
>> +    if (m_ariaRole != UnknownRole)
> 
> is this part of the same bug?

Yes. Without this, a spin button's inner text becomes its title, but I think it generalizes to any ARIA role other than the ones that already explicitly get their title from inner text (like button).

This is covered by the test, as well.

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3394
>> +    case ScrollBarRole:
> 
> should you have removed scrollbar role?

I wasn't sure. I'll leave it out.

>> Source/WebCore/accessibility/AccessibilityRenderObject.h:305
>> +    bool isAriaRange() const;
> 
> should be isARIARange()

Done.

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:2002
>> +        if (m_object->isProgressIndicator() || m_object->isSlider() || m_object->isScrollbar() || m_object->isSpinButton())
> 
> Looks like we should probably have a supportsRangeValue() method

Good idea, done.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Kent Tamura]

Comment on attachment 162698 [details]
Patch

OK for Chromium WebKit API change.

[Peter Beverloo (cr-android ews)]

Comment on attachment 162698 [details]
Patch

Attachment 162698 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13785354

[Dominic Mazzoni]

Created attachment 162712 [details]
Patch

[chris fleizach]

Comment on attachment 162712 [details]
Patch

looks good. thanks

[WebKit Review Bot]

Comment on attachment 162712 [details]
Patch

Clearing flags on attachment: 162712

Committed r127882: <http://trac.webkit.org/changeset/127882>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.