RESOLVED FIXED 28841
WAI-ARIA: add support for ranges, including the progressbar, slider, and spinbutton roles
https://bugs.webkit.org/show_bug.cgi?id=28841
Summary WAI-ARIA: add support for ranges, including the progressbar, slider, and spin...
chris fleizach
Reported 2009-08-30 23:27:05 PDT
Additional support needed requires: sending notifications when value changes updated aria-valuenow
Attachments
patch (23.70 KB, patch)
2009-08-30 23:29 PDT, chris fleizach
no flags
patch (24.32 KB, patch)
2009-08-30 23:36 PDT, chris fleizach
eric: review-
updated patch (9.28 KB, patch)
2009-09-11 23:26 PDT, chris fleizach
bdakin: review+
chris fleizach
Comment 1 2009-08-30 23:28:26 PDT
"updated aria-valuenow" should read "update aria-valuenow when the slider is incremented or decremented"
chris fleizach
Comment 2 2009-08-30 23:29:40 PDT
chris fleizach
Comment 3 2009-08-30 23:30:26 PDT
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
chris fleizach
Comment 4 2009-08-30 23:36:20 PDT
Eric Seidel (no email)
Comment 5 2009-08-31 02:57:39 PDT
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.
chris fleizach
Comment 6 2009-09-11 23:26:14 PDT
Created attachment 39506 [details] updated patch
Eric Seidel (no email)
Comment 7 2009-09-14 10:09:05 PDT
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.
chris fleizach
Comment 8 2009-09-14 10:16:20 PDT
(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
Beth Dakin
Comment 9 2009-09-15 12:18:27 PDT
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.
chris fleizach
Comment 10 2009-09-15 17:41:23 PDT
Eric Seidel (no email)
Comment 11 2009-09-15 17:45:03 PDT
(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.
chris fleizach
Comment 12 2009-09-15 20:39:44 PDT
Note You need to log in before you can comment on or make changes to this bug.