Skip to content
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

Merged
merged 26 commits into from
Nov 22, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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. |
| natural_arrow_keys | bool | false | The natural order is used for the arrow keys. Arrow up select the upper slider value for vertical sliders, arrow right the righter slider value for a horizontal slider - no matter if the slider was reversed or not. By default the arrow keys are oriented by arrow up/right to the higher slider value, arrow down/left to the lower slider value. |
| ticks | array | [ ] | Used to define the values of ticks. Tick marks are indicators to denote special values in the range. This option overwrites min and max options. |
| ticks_positions | array | [ ] | Defines the positions of the tick values in percentages. The first value should always be 0, the last value should always be 100 percent. |
Expand Down
6 changes: 6 additions & 0 deletions src/js/bootstrap-slider.js
Original file line number Diff line number Diff line change
Expand Up @@ -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]));
Copy link
Collaborator

@rovolution rovolution Nov 10, 2016

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?

Copy link
Collaborator

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?

Copy link
Contributor Author

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/

Copy link
Contributor Author

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

Copy link
Collaborator

@rovolution rovolution Nov 10, 2016

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?

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

Copy link
Collaborator

@rovolution rovolution Nov 10, 2016

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

Copy link
Collaborator

@rovolution rovolution Nov 12, 2016

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 on this.handle2 below (line 1155), so I think it would be valuable to set it on handle2 as well

}

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]));
Copy link
Collaborator

Choose a reason for hiding this comment

The 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!

Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it's not failing, lol

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/* Position highlight range elements */
if (this.rangeHighlightElements.length > 0 && Array.isArray(this.options.rangeHighlights) && this.options.rangeHighlights.length > 0) {
Expand Down
57 changes: 57 additions & 0 deletions test/specs/AriaValueTextFormatterSpec.js
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({
Copy link
Collaborator

@rovolution rovolution Nov 21, 2016

Choose a reason for hiding this comment

The 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 #accessibilitySliderC" to the /tpl/SpecRunner.tpl file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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]);
});
});
2 changes: 2 additions & 0 deletions tpl/SpecRunner.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@
<span id="accessibilitySliderLabelB">Label B</span>
<input id="accessibilitySliderA" type="text" data-slider-labelledby="accessibilitySliderLabelA" />
<input id="accessibilitySliderB" type="text" data-slider-labelledby='["accessibilitySliderLabelA", "accessibilitySliderLabelB"]' />

<input id="accessibilitySliderC" type="text" />
</div>
<% with (scripts) { %>
<% [].concat(jasmine, vendor, src, specs, reporters, start).forEach(function(script){ %>
Expand Down