[webkit-reviews] review denied: [Bug 50505] HTML5 Slider does not work correctly with VoiceOver : [Attachment 75583] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied chris fleizach <cfleizach at apple.com>'s
request for review:
Bug 50505: HTML5 Slider does not work correctly with VoiceOver
https://bugs.webkit.org/show_bug.cgi?id=50505

Attachment 75583: Patch
https://bugs.webkit.org/attachment.cgi?id=75583&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=75583&action=review

review- because the min/max work should be done differently

> WebCore/accessibility/AccessibilityRenderObject.cpp:2767
> +    if (result->isInputSlider()) 
> +	   return result->doAccessibilityHitTest(point);

It seems really unfortunate here that we have to call one virtual function just
to find out whether to call another. Is there a better way to design this?

> WebCore/accessibility/AccessibilitySlider.cpp:135
> +    const AtomicString& maxValue = getAttribute(maxAttr);
> +    // HTML says default value for max is 100.
> +    if (maxValue.isEmpty())
> +	   return 100;
> +    return maxValue.toFloat();

HTMLInputElement has a maximum function, which will give the correct answer.
RangInputType.cpp has the default value of 100. It's not smart to repeat the
logic here, because the two can so easily get out of sync.

Also, there are multiple cases where toFloat will return 0, including the null
attribute (no max at all), the empty string, and a string that can’t be parsed
as a number. This code special cases null and empty attributes, but does it do
the right thing for strings like max="x"? The toFloat function has an out
parameter to indicates whether it parsed a number or not, which is better if
you want to handle non-empty strings that are not parsed as numbers.

> WebCore/accessibility/AccessibilitySlider.cpp:144
> -    return getAttribute(minAttr).toFloat();
> +    const AtomicString& minValue = getAttribute(minAttr);
> +    // HTML says default value for max is 0.
> +    if (minValue.isEmpty())
> +	   return 0;
> +    return minValue.toFloat();

HTMLInputElement's minimum function is the better way to do this.

Default behavior for toFloat is already to return 0, so no code change is
needed. Comment says max, but code is for min. This call site could use
fastGetAttribute.