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
5) the slider was returning NSAccessibilityValueAttribute twice
Created attachment 75583 [details] Patch
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.
(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
Created attachment 75715 [details] Patch
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.
(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
http://trac.webkit.org/changeset/73388
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
Build fix landed in http://trac.webkit.org/changeset/73395. Please verify the change.
(In reply to comment #10) > Build fix landed in http://trac.webkit.org/changeset/73395. Please verify the change. thanks!