Additional support needed requires: sending notifications when value changes updated aria-valuenow
"updated aria-valuenow" should read "update aria-valuenow when the slider is incremented or decremented"
Created attachment 38802 [details] patch
I also took the chance to change the notifications sent to an enum type, instead of relying on the strings that are duplicated in Apple's NSAccessibility protocol
Created attachment 38803 [details] patch
Comment on attachment 38803 [details] patch I'm confused. Why does: 604 void AccessibilityRenderObject::increment() not send a notification, but: 139130 void AccessibilitySlider::increment() does? Sad that this method doens't have a clearer name: 369 virtual void changeValue(float percentChange); changeValueByPercent for example. Seems the enum change really should have been done separately. capitalization? 1 2009-08-30 chris fleizach <cfleizach@apple.com> +++ LayoutTests/accessibility/aria-slider-value-change.html (revision 0) would be much better as a js-only test instead of manually modifying the template as you've done. If it's a JS-only test, you shoudl also consider removing teh slider at th end of th test so it doesn't appear in the output.
Created attachment 39506 [details] updated patch
Comment on attachment 39506 [details] updated patch 30 shouldBe("slider.intValue > 50", "true"); shouldBeTrue would do that for you. This isn't quite a js-only test. But I admit the js-only test infrastructure is poorly documented at best. :( To be a real js-only test this would be .js file in script-tests/ or resources/ and make-script-tests-wrappers would generate the .html for you. Why was this check not needed before? + if (!m_renderer) + return; was setValue never called w/o checking the renderer before? Should we remove other renderer checks now that that's added? I'm not sure why this should move out of AccessibilitySlider: +void AccessibilityRenderObject::increment() +{ + if (roleValue() != SliderRole) + return; + + changeValueByPercent(5); +} Can other things have a SliderRole which are not AccessiblitySliders? Is that exactly what this change is doing? supporting the role="slider" on things which are not sliders? element seems like a funny name for a node here: + Node* element = m_renderer->node(); + if (element && element->isElementNode()) I guess I just don't really understand this change enough to review it.
(In reply to comment #7) > (From update of attachment 39506 [details]) > 30 shouldBe("slider.intValue > 50", "true"); > shouldBeTrue would do that for you. > agreed. > This isn't quite a js-only test. But I admit the js-only test infrastructure > is poorly documented at best. :( To be a real js-only test this would be .js > file in script-tests/ or resources/ and make-script-tests-wrappers would > generate the .html for you. > It seems like overkill to create two new files for every layout test, when one will do just fine. This is also the format I've been following for accessibility tests for a number of months now without any other feedback. > Why was this check not needed before? > + if (!m_renderer) > + return; > It was erroneously not checked before. I have been slowly adding these necessary checks back in as I see them over time. > was setValue never called w/o checking the renderer before? Should we remove > other renderer checks now that that's added? > setValue was not called with checking the renderer, which in most cases will not crash, but there's no guarantee that m_renderer will not be nil in a lot of cases, so it needs to be checked > I'm not sure why this should move out of AccessibilitySlider: > I moved it out of AccessibilitySlider, because the change that I makes allows AccessibilityRenderObject to act as a slider, if the ARIA role is a slider type. AccessibilitySlider is being used currently when the type is <input type="range">, and comes with a whole host of assumed capabilities. AccessibilityRenderObject was *already* acting as an ARIA slider when that role was specified, but it did not correctly increment or decrement values (which is what this bug is fixing) > +void AccessibilityRenderObject::increment() > +{ > + if (roleValue() != SliderRole) > + return; > + > + changeValueByPercent(5); > +} > > Can other things have a SliderRole which are not AccessiblitySliders? Is that > exactly what this change is doing? supporting the role="slider" on things > which are not sliders? > correct. > element seems like a funny name for a node here: > + Node* element = m_renderer->node(); > + if (element && element->isElementNode()) > I want that Node to be an elementNode, so element seemed as good a choice as any other, but it can obviously be changed
Comment on attachment 39506 [details] updated patch Looks good! It's true that this is technically not a js test. I only recently learned about js tests myself, and I have made a few of them recently. I agree with Chris that I dislike having two files. That combined with the fact that we have some like this checked into the accessibility tests anyway, I am going to r+ this. Personally, I like this style better anyway.
Committed rr48405: <http://trac.webkit.org/changeset/r48405>
(In reply to comment #9) > (From update of attachment 39506 [details]) > Looks good! > > It's true that this is technically not a js test. I only recently learned about > js tests myself, and I have made a few of them recently. I agree with Chris > that I dislike having two files. That combined with the fact that we have some > like this checked into the accessibility tests anyway, I am going to r+ this. > Personally, I like this style better anyway. Yeah, I agree it's less than ideal that we have 2 files checked in for js tests. We should fix that. :) We just need to find a nice way to view the .js files in a browser.
Committed rr48407: <http://trac.webkit.org/changeset/r48407>