[Webkit-unassigned] [Bug 142887] role progress bar does not support indeterminate state

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Apr 1 23:30:19 PDT 2015


chris fleizach <cfleizach at apple.com> changed:

           What    |Removed                     |Added
 Attachment #249957|                            |review-
              Flags|                            |

--- Comment #5 from chris fleizach <cfleizach at apple.com> ---
Comment on attachment 249957
  --> https://bugs.webkit.org/attachment.cgi?id=249957

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)

You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150402/ee7b5779/attachment.html>

More information about the webkit-unassigned mailing list