Bug 50505 - HTML5 Slider does not work correctly with VoiceOver
Summary: HTML5 Slider does not work correctly with VoiceOver
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: chris fleizach
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-03 17:27 PST by chris fleizach
Modified: 2010-12-06 13:44 PST (History)
4 users (show)

See Also:


Attachments
Patch (12.12 KB, patch)
2010-12-03 17:38 PST, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (12.62 KB, patch)
2010-12-06 11:22 PST, chris fleizach
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2010-12-03 17:27:54 PST
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
Comment 1 chris fleizach 2010-12-03 17:33:57 PST
5) the slider was returning NSAccessibilityValueAttribute twice
Comment 2 chris fleizach 2010-12-03 17:38:42 PST
Created attachment 75583 [details]
Patch
Comment 3 Darin Adler 2010-12-06 09:43:49 PST
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.
Comment 4 chris fleizach 2010-12-06 10:29:51 PST
(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
Comment 5 chris fleizach 2010-12-06 11:22:28 PST
Created attachment 75715 [details]
Patch
Comment 6 Darin Adler 2010-12-06 12:04:52 PST
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.
Comment 7 chris fleizach 2010-12-06 12:08:53 PST
(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
Comment 8 chris fleizach 2010-12-06 12:38:14 PST
http://trac.webkit.org/changeset/73388
Comment 9 James Robinson 2010-12-06 13:19:38 PST
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
Comment 10 Ryosuke Niwa 2010-12-06 13:28:03 PST
Build fix landed in http://trac.webkit.org/changeset/73395.  Please verify the change.
Comment 11 chris fleizach 2010-12-06 13:44:58 PST
(In reply to comment #10)
> Build fix landed in http://trac.webkit.org/changeset/73395.  Please verify the change.

thanks!