Bug 50505 - HTML5 Slider does not work correctly with VoiceOver
: HTML5 Slider does not work correctly with VoiceOver
Status: RESOLVED FIXED
: WebKit
Accessibility
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-12-03 17:27 PST by
Modified: 2010-12-06 13:44 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2010-12-03 17:33:57 PST -------
5) the slider was returning NSAccessibilityValueAttribute twice
------- Comment #2 From 2010-12-03 17:38:42 PST -------
Created an attachment (id=75583) [details]
Patch
------- Comment #3 From 2010-12-06 09:43:49 PST -------
(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.
------- Comment #4 From 2010-12-06 10:29:51 PST -------
(In reply to comment #3)
> (From update of attachment 75583 [details] [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 From 2010-12-06 11:22:28 PST -------
Created an attachment (id=75715) [details]
Patch
------- Comment #6 From 2010-12-06 12:04:52 PST -------
(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.

> 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 From 2010-12-06 12:08:53 PST -------
(In reply to comment #6)
> (From update of attachment 75715 [details] [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 From 2010-12-06 12:38:14 PST -------
http://trac.webkit.org/changeset/73388
------- Comment #9 From 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 From 2010-12-06 13:28:03 PST -------
Build fix landed in http://trac.webkit.org/changeset/73395.  Please verify the change.
------- Comment #11 From 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!