Bug 50505 – HTML5 Slider does not work correctly with VoiceOver

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

[chris fleizach]

Here are the HTML5 slider problems

1) value indicator is not hit testable
2) value indicator does not return an AXValue
3) slider - min/max values are not correct
4) when you change the value with the KB, it doesn't send out the right notification


rdar://8724757

[chris fleizach]

5) the slider was returning NSAccessibilityValueAttribute twice

[chris fleizach]

Created attachment 75583 [details]
Patch

[Darin Adler]

Comment on attachment 75583 [details]
Patch

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.

[chris fleizach]

(In reply to comment #3)
> (From update of attachment 75583 [details])
> 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.

Didn't realize those methods were there. will definitely use them. 

I filed a different bug to handle refactoring the hit test code, rather than muddle this patch with that

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

[chris fleizach]

Created attachment 75715 [details]
Patch

[Darin Adler]

Comment on attachment 75715 [details]
Patch

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

> WebCore/accessibility/AccessibilityObject.cpp:850
> +    return element->fastGetAttribute(attribute);

This is OK as long as we never want to get the "style" attribute or an SVG animatable attribute.

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

This special cases seems really lame. Is there any way to handle this with a virtual function that works in a more consistent way rather than specifically doing isListBox and then isInputSlider?

> WebCore/accessibility/AccessibilitySlider.h:47
>      virtual bool isSlider() const { return true; }
> -
> +    virtual bool isInputSlider() const { return true; }

These overrides should be private.

[chris fleizach]

(In reply to comment #6)
> (From update of attachment 75715 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75715&action=review
> 
> > WebCore/accessibility/AccessibilityObject.cpp:850
> > +    return element->fastGetAttribute(attribute);
> 
> This is OK as long as we never want to get the "style" attribute or an SVG animatable attribute.
> 

So far, we have not needed to do that. if that need arises, we can add a case for it.

> > WebCore/accessibility/AccessibilityRenderObject.cpp:2771
> > +    if (result->isInputSlider()) 
> > +        return result->doAccessibilityHitTest(point);
> 
> This special cases seems really lame. Is there any way to handle this with a virtual function that works in a more consistent way rather than specifically doing isListBox and then isInputSlider?
> 

I don't have a good solution for that right now. The first step is performing a hit test, which gives us the render object we want. Since most elements are render based, we're done

However, if an element has "fake" children (list box, slider, etc), we still need another step to hit test into those fake children.

My thoughts now are that we could have just one method that by default returns this. then that can be overriden by whomever is interested.

> > WebCore/accessibility/AccessibilitySlider.h:47
> >      virtual bool isSlider() const { return true; }
> > -
> > +    virtual bool isInputSlider() const { return true; }
> 
> These overrides should be private.

aye aye

[chris fleizach]

http://trac.webkit.org/changeset/73388

[James Robinson]

It looks like this doesn't compile on Leopard:

cc1plus: warnings being treated as errors
/Volumes/Big/WebKit-BuildSlave/leopard-intel-debug/build/WebCore/accessibility/AccessibilitySlider.cpp:131: warning: implicit conversion shortens 64-bit value into a 32-bit value
/Volumes/Big/WebKit-BuildSlave/leopard-intel-debug/build/WebCore/accessibility/AccessibilitySlider.cpp:136: warning: implicit conversion shortens 64-bit value into a 32-bit value

[Ryosuke Niwa]

Build fix landed in http://trac.webkit.org/changeset/73395.  Please verify the change.

[chris fleizach]

(In reply to comment #10)
> Build fix landed in http://trac.webkit.org/changeset/73395.  Please verify the change.

thanks!