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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=142887

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
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)

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