Bug 96076

Summary: AX: ARIA spin button should support range value attributes
Product: WebKit Reporter: Dominic Mazzoni <dmazzoni>
Component: AccessibilityAssignee: Dominic Mazzoni <dmazzoni>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aboxhall, cfleizach, dglazkov, donggwan.kim, fishd, jamesr, peter+ews, tkent, tkent+wkapi, webkit.review.bot, yosin
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 96032    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Dominic Mazzoni 2012-09-07 00:22:54 PDT
According to the ARIA spec, an ARIA spin button should use aria-valuenow, etc. for its value.
Comment 1 Dominic Mazzoni 2012-09-07 00:45:11 PDT
Created attachment 162693 [details]
Patch
Comment 2 chris fleizach 2012-09-07 00:48:55 PDT
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
Comment 3 Dominic Mazzoni 2012-09-07 01:11:09 PDT
Created attachment 162698 [details]
Patch
Comment 4 Dominic Mazzoni 2012-09-07 01:11:43 PDT
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.
Comment 5 WebKit Review Bot 2012-09-07 01:14:59 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 6 Kent Tamura 2012-09-07 01:16:39 PDT
Comment on attachment 162698 [details]
Patch

OK for Chromium WebKit API change.
Comment 7 Peter Beverloo (cr-android ews) 2012-09-07 01:49:13 PDT
Comment on attachment 162698 [details]
Patch

Attachment 162698 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13785354
Comment 8 Dominic Mazzoni 2012-09-07 01:53:05 PDT
Created attachment 162712 [details]
Patch
Comment 9 chris fleizach 2012-09-07 09:15:46 PDT
Comment on attachment 162712 [details]
Patch

looks good. thanks
Comment 10 WebKit Review Bot 2012-09-07 10:07:03 PDT
Comment on attachment 162712 [details]
Patch

Clearing flags on attachment: 162712

Committed r127882: <http://trac.webkit.org/changeset/127882>
Comment 11 WebKit Review Bot 2012-09-07 10:07:15 PDT
All reviewed patches have been landed.  Closing bug.