<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:cfleizach@apple.com" title="chris fleizach <cfleizach@apple.com>"> <span class="fn">chris fleizach</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - role progress bar does not support indeterminate state"
href="https://bugs.webkit.org/show_bug.cgi?id=142887">bug 142887</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #249957 Flags</td>
<td>
</td>
<td>review-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - role progress bar does not support indeterminate state"
href="https://bugs.webkit.org/show_bug.cgi?id=142887#c5">Comment # 5</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - role progress bar does not support indeterminate state"
href="https://bugs.webkit.org/show_bug.cgi?id=142887">bug 142887</a>
from <span class="vcard"><a class="email" href="mailto:cfleizach@apple.com" title="chris fleizach <cfleizach@apple.com>"> <span class="fn">chris fleizach</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=249957&action=diff" name="attach_249957" title="patch">attachment 249957</a> <a href="attachment.cgi?id=249957&action=edit" title="patch">[details]</a></span>
patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=249957&action=review">https://bugs.webkit.org/attachment.cgi?id=249957&action=review</a>
<span class="quote">> Source/WebCore/ChangeLog:3
> + Fixes: <a class="bz_bug_link
bz_status_NEW "
title="NEW - role progress bar does not support indeterminate state"
href="show_bug.cgi?id=142887">Bug 142887</a> - role progress bar does not support indeterminate state </span >
This line should remove Fixes.
It should match the other entries in ChangeLog
<span class="quote">> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:865
> + return 0.0f;</span >
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
<span class="quote">> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:881
> + if (isProgressIndicator())</span >
I don't think we want to return 0 unconditionally for minValue if it's a progressIndicator
<span class="quote">> Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp:92
> + if (progress && progress->position() >= 0)</span >
why was this change necessary?
<span class="quote">> Source/WebCore/accessibility/AccessibilityProgressIndicator.cpp:103
> + // Indeterminate progress bar should return 0.</span >
this comment doesn't seem necessary, since there's it doesn't explain anything that's happening in the code at this locus
<span class="quote">> LayoutTests/ChangeLog:3
> + Test that checks if indeterminate progress indicators return</span >
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
<span class="quote">> 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.");</span >
Since this is a test that will implement a Mac platform specific implementation, we should put this in the platform/mac/accessibility folder
<span class="quote">> LayoutTests/accessibility/progressbar-indeterminate.html:19
> + // div element given progressbar role</span >
when should make this comment a full sentence (capitalize first letter, ends with period)
<span class="quote">> LayoutTests/accessibility/progressbar-indeterminate.html:31
> + shouldBe("obj.minValue", "0");</span >
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)</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>