Bug 142887 – role progress bar does not support indeterminate state

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

[chris fleizach]

Summary:
When creating a progress bar using role progress bar, I want to be able to create a indeterminate progress bar. For this I do NOT specify the aria-valuenow attribute as documented in aria-valuenow. http://www.w3.org/TR/wai-aria/states_and_properties#aria-valuenow

I did however specify valuemin (0) and valuemax (100), as the progress bar might become determinate at some point. When doing this, VoiceOver tells me that the current value is 0, which is not what I was expecting.

Steps to Reproduce:
<div class="oo-ui-widget oo-ui-widget-enabled oo-ui-progressBarWidget" role="progressbar" aria-valuemin="0" aria-valuemax="100"><div class="oo-ui-progressBarWidget-bar" style="width: 33%;"></div></div>

Expected Results:


Actual Results:
Was able to workaround by also removing valuemin and valuemax, but that does not seem like the appropriate requirement here I think.

Version:
Yosemite 10.10.1

-----------------------

The only author requirement is to leave out the valuenow attr. VO should convey and unknown value regardless of whether the min and max are specified.

https://w3c.github.io/aria/aria/aria.html#progressbar

------------------------

WebKit is reporting a writable AXValue of 0

<rdar://problem/19621710>

Created attachment 249689 [details]
test case

See attached test case. VoiceOver returns 0 percent, despite progress bars not including aria value now attribute. VoiceOver will recognize them as indeterminate if you remove aria min and max attributes, but aria documentation says author should only need to exclude aria value now attribute to make it indeterminate.

[chris fleizach]

I think that discrepancy is fine. Our job is to translate from web standards to Apple accessibility platforms. This case sounds like it falls within those parameters

(In reply to comment #2)
> See attached test case. VoiceOver returns 0 percent, despite progress bars
> not including aria value now attribute. VoiceOver will recognize them as
> indeterminate if you remove aria min and max attributes, but aria
> documentation says author should only need to exclude aria value now
> attribute to make it indeterminate.

(In reply to comment #2)
> See attached test case. VoiceOver returns 0 percent, despite progress bars
> not including aria value now attribute. VoiceOver will recognize them as
> indeterminate if you remove aria min and max attributes, but aria
> documentation says author should only need to exclude aria value now
> attribute to make it indeterminate.

Created attachment 249957 [details]
patch

[chris fleizach]

Comment on attachment 249957 [details]
patch

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

> Source/WebCore/ChangeLog:3
> +        Fixes: Bug 142887 - role progress bar does not support indeterminate state 

This line should remove Fixes. 
It should match the other entries in ChangeLog

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:865
> +        return 0.0f;

I don't think we can make this assumption for each platform. I think this is a platform implementation detail, so we should move this logic in the WebAXObjectWrapperMac.

It's possible that GTK does not follow the same convention

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:881
> +    if (isProgressIndicator())

I don't think we want to return 0 unconditionally for minValue if it's a progressIndicator

> Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp:92
> +        if (progress && progress->position() >= 0)

why was this change necessary?

> Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp:103
> +    // Indeterminate progress bar should return 0.

this comment doesn't seem necessary, since there's it doesn't explain anything that's happening in the code at this locus

> LayoutTests/ChangeLog:3
> +        Test that checks if indeterminate progress indicators return

this change log should follow the other WebCore change log, where there's a Bug name, then URL

Then put comments below the Reviewed by line

> LayoutTests/accessibility/progressbar-indeterminate.html:16
> +    description("Make sure progress indicators that don't have a value return 0 for min and max. This ensures VoiceOver says indeterminate instead of 0.");

Since this is a test that will implement a Mac platform specific implementation, we should put this in the platform/mac/accessibility folder

> LayoutTests/accessibility/progressbar-indeterminate.html:19
> +          // div element given progressbar role

when should make this comment a full sentence (capitalize first letter, ends with period)

> LayoutTests/accessibility/progressbar-indeterminate.html:31
> +          shouldBe("obj.minValue", "0");

you should verify that in fact you are messaging the progressbar2 element here with some shouldBe line (just in case you're still messaging the old one)

Created attachment 250124 [details]
patch

Moved logic to WebAccessibilityObjectWrapperMac.mm. Return 0 if getting max value from an indeterminate progress indicator. LayoutTest ensures 0 is returned for max value of a div element and progress element, both indeterminate with role progressbar.

[chris fleizach]

Comment on attachment 250124 [details]
patch

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2501
>          return [NSNumber numberWithFloat:m_object->minValueForRange()];

don't we need to do the same thing for the min value for indeterminate items?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2506
> +            return [NSNumber numberWithFloat:0.0f];

this can be written as
@0

> LayoutTests/platform/mac/accessibility/progressbar-indeterminate-expected.txt:8
> +PASS prog1.maxValue is 0

i think we need to test the min value too right

Created attachment 250125 [details]
patch

Added the same logic for when accessing min value attribute. If it is indeterminate, return 0. 
Added a shouldBe check for min value. I also set test elements aria min and max attributes to non zero values.

Created attachment 250126 [details]
patch

Fixed a typo in the test (max and max should be min and max)

[chris fleizach]

Comment on attachment 250126 [details]
patch

looks good. thanks!

[WebKit Commit Bot]

Comment on attachment 250126 [details]
patch

Rejecting attachment 250126 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 250126, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
unexpectedly ends in middle of line
patch: **** malformed patch at line 36:  

fatal: pathspec 'LayoutTests/platform/mac/accessibility/progressbar-indeterminate.html' did not match any files
Failed to git add LayoutTests/platform/mac/accessibility/progressbar-indeterminate.html. at /Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply line 448.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Chris Fleizach']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/4803418535231488

[chris fleizach]

I think you need to add a patch where you do an svn add on the new files first

Created attachment 250195 [details]
patch

I made sure there were no untracked files before I ran svn diff.

[WebKit Commit Bot]

Comment on attachment 250195 [details]
patch

Clearing flags on attachment: 250195

Committed r182520: <http://trac.webkit.org/changeset/182520>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.