aria-valuetext as formatter value #646

mediaformat · 2016-11-01T04:09:55Z

Pull Requests

Original issue : Feature Request : aria-valuetext as formatter value
#641

Example of feature in action:
https://fiddle.jshell.net/bbw6z63n/4/

Issue #641

rovolution · 2016-11-01T16:06:44Z

src/js/bootstrap-slider.js

@@ -1148,6 +1148,7 @@ const windowIsDefined = (typeof window === "object");

 				this.handle1.style[this.stylePos] = positionPercentages[0]+'%';
 				this.handle1.setAttribute('aria-valuenow', this._state.value[0]);
+				this.handle1.setAttribute('aria-valuetext', this.options.formatter(this._state.value[0]));


Can you add a unit test that checks if the aria-valuetext is updated appropriately when the slider value is changed? Thanks!

Added a check that formatter is used

-Sets the aria-valuetext to 'formatter' value -Does not use aria-valuetext if 'formatter' is not used

mediaformat · 2016-11-07T06:21:55Z

I've added a test, and added a check that the value isNaN to use aria-valuetext, as per the spec

rovolution · 2016-11-07T13:53:07Z

@mediaformat tests look good! however, looks like it is failing the lint check on CI:

mediaformat · 2016-11-09T16:07:48Z

Ok, it looks like I need to do this DOM less...

seiyria · 2016-11-09T16:12:43Z

No. Just declare the variables with var instead of making them global (if
I'm reading this right).

On Wed, Nov 9, 2016, 10:07 Django Doucet notifications@github.com wrote:

Ok, it looks like I need to do this DOM less...

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#646 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAum2SfHqZf5OoVSKlmKRnANJuieE3kVks5q8e_UgaJpZM4KlwK0
.

rovolution · 2016-11-10T03:42:25Z

src/js/bootstrap-slider.js

@@ -1148,6 +1148,9 @@ const windowIsDefined = (typeof window === "object");

 				this.handle1.style[this.stylePos] = positionPercentages[0]+'%';
 				this.handle1.setAttribute('aria-valuenow', this._state.value[0]);
+				if (isNaN(this.options.formatter(this._state.value[0])) ) {
+					this.handle1.setAttribute('aria-valuetext', this.options.formatter(this._state.value[0]));


can we set this for handle2 as well?

could you also add a section to our README.md that documents this new ARIA property and add an example to our /tpl/index.tpl page?

about handle2...
What is it's purpose?
It always seems to be hidden and its aria-valuenow is unchanged...and defaults to 1 even when set to something else : https://fiddle.jshell.net/bbw6z63n/7/

which would be confusing to screen readers

about handle2...
What is it's purpose?

Purpose is the use case where we have a 2 handle slider - see Example 2 in our examples page: http://seiyria.com/bootstrap-slider/

And the README update is so that users of this library know which ARIA attributes are set on the slider

@mediaformat we are also setting aria-valuenow on this.handle2 below (line 1155), so I think it would be valuable to set it on handle2 as well

rovolution · 2016-11-10T03:43:09Z

test/specs/AriaValueTextFormatterSpec.js

@@ -0,0 +1,27 @@
+it("Sets the aria-valuetext to 'formatter' value", function() {


can you wrap this entire spec in a describe block?

rovolution · 2016-11-10T03:45:07Z

src/js/bootstrap-slider.js

@@ -1148,6 +1148,9 @@ const windowIsDefined = (typeof window === "object");

 				this.handle1.style[this.stylePos] = positionPercentages[0]+'%';
 				this.handle1.setAttribute('aria-valuenow', this._state.value[0]);
+				if (isNaN(this.options.formatter(this._state.value[0])) ) {
+					this.handle1.setAttribute('aria-valuetext', this.options.formatter(this._state.value[0]));


could you also add a section to our README.md that documents this new ARIA property and add an example to our /tpl/index.tpl page?

mediaformat · 2016-11-11T03:18:24Z

Weird, it fails with Describe, and Passes without it!?

rovolution · 2016-11-11T05:30:43Z

README.md

@@ -187,7 +187,7 @@ Options can be passed either as a data (data-slider-foo) attribute, or as part o
 | handle |	string |	'round' |	handle shape. Accepts: 'round', 'square', 'triangle' or 'custom' |
 | reversed | bool | false | whether or not the slider should be reversed |
 | enabled | bool | true | whether or not the slider is initially enabled |
-| formatter |	function |	returns the plain value |	formatter callback. Return the value wanted to be displayed in the tooltip |
+| formatter |	function |	returns the plain value |	formatter callback. Return the value wanted to be displayed in the tooltip, useful for string values. If a string is returned it will be indicated in an aria-valuetext attribute.  |


could you wrap the "aria-valuetext" string in backticks (`) so that it stands out visually in the README?

aria-valuetext

rovolution · 2016-11-13T16:01:44Z

src/js/bootstrap-slider.js


 				this.handle2.style[this.stylePos] = positionPercentages[1]+'%';
 				this.handle2.setAttribute('aria-valuenow', this._state.value[1]);
+				if (isNaN(this.options.formatter(this._state.value[1])) ) {
+					this.handle2.setAttribute('aria-valuetext', this.options.formatter(this._state.value[1]));


great! if you add a test for this in your unit test suite, i think this is good to go!

@mediaformat have you had a chance to check this out? I would love to merge this in once the test for a multi-handle slider is complete

The multi-handle works in practice : https://fiddle.jshell.net/bbw6z63n/11/

But I can't seem to get the test to work, I've committed it, maybe you'll be able to see why it doesn't work...

maybe it's not failing, lol

i wonder why this test is still failing (yet passes when the describe is not there)

Test is failing, not sure why

Does not use aria-valuetext if 'formatter' is not used. Code was targeting testSliderA testSliderC still failing mysteriously

mediaformat · 2016-11-19T02:31:51Z

Yeah it's a bit hard to see where things went wrong when the error message (for testSliderC) says:

Expected [ 'Tuesday', 'Thursday' ] to be [ 'Tuesday', 'Thursday' ]

rovolution · 2016-11-19T03:23:15Z

@mediaformat instead of that, could you re-write this to be the following?

expect(bothMessages[0]).toBe(expectedMessageC[0]);
expect(bothMessages[1]).toBe(expectedMessageC[1]);

works locally on jasmine standalone 2.5.2

works locally

rovolution · 2016-11-21T05:18:06Z

test/specs/AriaValueTextFormatterSpec.js

+      };
+
+      //Formatter is used for ranges
+      var testSliderC = $("#accessibilitySliderC").slider({


oh i think i know why this is failing @mediaformat!

I believe you forget to add an input element with id #accessibilitySliderC" to the /tpl/SpecRunner.tpl file.

that was it!

rovolution · 2016-11-22T03:29:12Z

just pulled your branch down locally to verify everything...it looks great! merging now

seiyria · 2016-11-22T13:39:01Z

very nice PR!

mediaformat · 2016-11-22T15:39:14Z

Thank you sir!

aria-valuetext as formatter value

868358e

Issue #641

rovolution added the needs-test label Nov 1, 2016

rovolution requested changes Nov 1, 2016

View reviewed changes

mediaformat added 3 commits November 1, 2016 22:09

aria-valuetext as formatter value

26982e1

Added a check that formatter is used

Added proper check that value isNaN

3831d4a

Create AriaValueTextFormatterSpec.js

a589672

-Sets the aria-valuetext to 'formatter' value -Does not use aria-valuetext if 'formatter' is not used

localize scope of variables

0495b7c

rovolution requested changes Nov 10, 2016

View reviewed changes

mediaformat added 3 commits November 9, 2016 23:29

Describe Aria-valuetext Tests

3741bd4

fix wrapper and code

8c92c31

test no-describe

9260f09

added a note about aria-valuetext

c84c69d

rovolution requested changes Nov 11, 2016

View reviewed changes

mediaformat added 2 commits November 11, 2016 13:28

backticks

023cec9

handle2

cd099c4

rovolution requested changes Nov 13, 2016

View reviewed changes

mediaformat added 5 commits November 16, 2016 01:53

multi-handle test failing

d8bfadd

Test is failing, not sure why

Describe tests wrap-up

352e495

Update range test C

454c745

use var testSlider setValue

1a1867c

Fixed testSliderB

a7b1f3c

Does not use aria-valuetext if 'formatter' is not used. Code was targeting testSliderA testSliderC still failing mysteriously

mediaformat added 6 commits November 18, 2016 22:45

Fixed testSlider C (range)

6609ccc

try testSlider3

2c13f59

works locally on jasmine standalone 2.5.2

try testSlider 3

f6e356d

works locally

shot in the dark

01d3504

update

ae6b81a

update

00f14c5

rovolution reviewed Nov 21, 2016

View reviewed changes

mediaformat added 4 commits November 21, 2016 12:28

testSlider3

72a9fd6

update

a5d1323

accessibilitySliderC for range tests

071d929

update

1cbcdf7

rovolution approved these changes Nov 22, 2016

View reviewed changes

rovolution merged commit 07210e0 into seiyria:master Nov 22, 2016

mediaformat mentioned this pull request Nov 22, 2016

tpl ex24 (range formatter) #657

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aria-valuetext as formatter value #646

aria-valuetext as formatter value #646

mediaformat commented Nov 1, 2016

rovolution Nov 1, 2016

mediaformat Nov 1, 2016

mediaformat commented Nov 7, 2016

rovolution commented Nov 7, 2016

mediaformat commented Nov 9, 2016

seiyria commented Nov 9, 2016

rovolution Nov 10, 2016 •

edited

Loading

rovolution Nov 10, 2016

mediaformat Nov 10, 2016

mediaformat Nov 10, 2016

rovolution Nov 10, 2016 •

edited

Loading

rovolution Nov 10, 2016 •

edited

Loading

rovolution Nov 12, 2016 •

edited

Loading

rovolution Nov 10, 2016

rovolution Nov 10, 2016

mediaformat commented Nov 11, 2016

rovolution Nov 11, 2016

rovolution Nov 13, 2016

rovolution Nov 15, 2016

mediaformat Nov 16, 2016

mediaformat Nov 16, 2016

rovolution Nov 19, 2016

mediaformat commented Nov 19, 2016

rovolution commented Nov 19, 2016 •

edited

Loading

rovolution Nov 21, 2016 •

edited

Loading

mediaformat Nov 22, 2016

rovolution commented Nov 22, 2016

seiyria commented Nov 22, 2016

mediaformat commented Nov 22, 2016

		@@ -0,0 +1,27 @@
		it("Sets the aria-valuetext to 'formatter' value", function() {

aria-valuetext as formatter value #646

aria-valuetext as formatter value #646

Conversation

mediaformat commented Nov 1, 2016

Pull Requests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mediaformat commented Nov 7, 2016

rovolution commented Nov 7, 2016

mediaformat commented Nov 9, 2016

seiyria commented Nov 9, 2016

rovolution Nov 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rovolution Nov 10, 2016 • edited Loading

Choose a reason for hiding this comment

rovolution Nov 10, 2016 • edited Loading

Choose a reason for hiding this comment

rovolution Nov 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mediaformat commented Nov 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mediaformat commented Nov 19, 2016

rovolution commented Nov 19, 2016 • edited Loading

rovolution Nov 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rovolution commented Nov 22, 2016

seiyria commented Nov 22, 2016

mediaformat commented Nov 22, 2016

rovolution Nov 10, 2016 •

edited

Loading

rovolution Nov 10, 2016 •

edited

Loading

rovolution Nov 10, 2016 •

edited

Loading

rovolution Nov 12, 2016 •

edited

Loading

rovolution commented Nov 19, 2016 •

edited

Loading

rovolution Nov 21, 2016 •

edited

Loading