Bug 28841 – WAI-ARIA: add support for ranges, including the progressbar, slider, and spinbutton roles

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

Additional support needed requires:

sending notifications when value changes
updated aria-valuenow

[chris fleizach]

"updated aria-valuenow" should read "update aria-valuenow when the slider is incremented or decremented"

[chris fleizach]

Created attachment 38802 [details]
patch

[chris fleizach]

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]

Created attachment 38803 [details]
patch

[Eric Seidel (no email)]

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  <[e-mail]>

+++ 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]

Created attachment 39506 [details]
updated patch

[Eric Seidel (no email)]

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]

(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 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]

Committed rr48405: <http://trac.webkit.org/changeset/r48405>

[Eric Seidel (no email)]

(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]

Committed rr48407: <http://trac.webkit.org/changeset/r48407>