-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
aria-valuetext as formatter value #646
Changes from all commits
868358e
26982e1
3831d4a
a589672
0495b7c
3741bd4
8c92c31
9260f09
c84c69d
023cec9
cd099c4
d8bfadd
352e495
454c745
1a1867c
a7b1f3c
6609ccc
2c13f59
f6e356d
01d3504
ae6b81a
00f14c5
72a9fd6
a5d1323
071d929
1cbcdf7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1148,9 +1148,15 @@ 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])); | ||
} | ||
|
||
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])); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. great! if you add a test for this in your unit test suite, i think this is good to go! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe it's not failing, lol There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i wonder why this test is still failing (yet passes when the |
||
} | ||
|
||
/* Position highlight range elements */ | ||
if (this.rangeHighlightElements.length > 0 && Array.isArray(this.options.rangeHighlights) && this.options.rangeHighlights.length > 0) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
describe("Aria-valuetext Tests", function() { | ||
it("Sets the aria-valuetext to 'formatter' value", function() { | ||
var textValArrayA = new Array('Monday','Wednesday','Friday'); | ||
var tooltipFormatterA = function(value) { | ||
var arrActiveValueA = value; | ||
return textValArrayA[arrActiveValueA-1]; | ||
}; | ||
|
||
//Formatter is used | ||
var testSliderA = $("#accessibilitySliderA").slider({ | ||
formatter : tooltipFormatterA | ||
}); | ||
testSliderA.slider('setValue', 2); | ||
|
||
var tooltipMessageA = $("#accessibilitySliderA").prev(".slider").children(".min-slider-handle").attr("aria-valuetext"); | ||
var expectedMessageA = tooltipFormatterA(2); | ||
expect(tooltipMessageA).toBe(expectedMessageA); | ||
|
||
}); | ||
|
||
it("Does not use aria-valuetext if 'formatter' is not used", function() { | ||
|
||
//Formatter is not used | ||
var testSliderB = $("#accessibilitySliderB").slider({}); | ||
testSliderB.slider('setValue', 1); | ||
|
||
var ariaValueTextB = $("#accessibilitySliderB").prev(".slider").children(".min-slider-handle").attr("aria-valuetext"); | ||
expect(ariaValueTextB).not.toBeDefined(); | ||
}); | ||
|
||
it("aria-valuetext if 'formatter' is used and has min & max value", function() { | ||
var textValArrayC = new Array('Monday','Tuesday','Wednesday','Thursday','Friday','Saturday','Sunday'); | ||
var tooltipFormatterC = function(value) { | ||
if(value[1]){ | ||
var arrActiveValueC0 = value[0]; | ||
var arrActiveValueC1 = value[1]; | ||
return [ textValArrayC[arrActiveValueC0-1], textValArrayC[arrActiveValueC1-1] ]; | ||
} else { | ||
var arrActiveValueC = value; | ||
return textValArrayC[arrActiveValueC-1]; | ||
} | ||
}; | ||
|
||
//Formatter is used for ranges | ||
var testSliderC = $("#accessibilitySliderC").slider({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh i think i know why this is failing @mediaformat! I believe you forget to add an input element with id There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that was it! |
||
range: true, | ||
formatter : tooltipFormatterC | ||
}); | ||
var valuesToSet = [2,4]; | ||
testSliderC.slider('setValue', valuesToSet); | ||
var expectedMessageC = tooltipFormatterC([2,4]); | ||
var ttminMessage = $("#accessibilitySliderC").prev(".slider").children(".min-slider-handle").attr("aria-valuetext"); | ||
var ttmaxMessage = $("#accessibilitySliderC").prev(".slider").children(".max-slider-handle").attr("aria-valuetext"); | ||
expect(ttminMessage).toBe(expectedMessageC[0]); | ||
expect(ttmaxMessage).toBe(expectedMessageC[1]); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we set this for
handle2
as well?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which would be confusing to screen readers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Purpose is the use case where we have a 2 handle slider - see Example 2 in our examples page: http://seiyria.com/bootstrap-slider/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the README update is so that users of this library know which ARIA attributes are set on the slider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mediaformat we are also setting
aria-valuenow
onthis.handle2
below (line 1155), so I think it would be valuable to set it on handle2 as well