[Accessibility] Refactor font slider for accessibility #14817

timroes · 2017-11-07T08:28:28Z

This PR changes the font slider in the tag cloud editor a bit, by replacing it's non accessible popovers via two number input fields below it:

When using the slider the values in the input fields will be changed immediately, and you can use the input fields vise versa to change the font, and the slider will automatically update.

This version now is fully accessible and should work well until we build a custom range picker in our UI framework.

Fixes #12905

timroes · 2017-11-07T09:41:36Z

Jenkins, test this

ppisljar · 2017-11-07T09:55:41Z

in the text boxes the second value can be smaller than the first one, which isn't possible to do with slider ... should we block that ?

timroes · 2017-11-07T10:24:59Z

Changed that behavior: the absolute min/max is still enforced by the browser and the min/max attribute on the input fields, but the crossing is now improved, by manually validating that, and always limiting it to the other sliders value.

ppisljar · 2017-11-07T10:45:16Z

ehm ... this gives quite a weird experience ...

type 40 in the first box
try to type 60 in the second box ... you won't be able to (after typing 6 value gets automatically updated to 40 ... )

probably the check should only happen after leaving the box (or on pressing enter)

timroes · 2017-11-07T10:49:16Z

Oh shit, yeah that's true. So we basically have two options here I think about:

a) we change the ng-model-options to only apply the value to scope when bluring the input field. That way the slider wouldn't change immediately when updating the values in the text field (e.g. also by pressing up/down), but therefore we wouldn't have to bother with that above issue.
b) We rely also for the crossing on the browsers min/max behavior (what it was before the last commit), meaning you can enter any value, but it will be highlighted red if it crosses each other, and not set at all to the $scope, so when you submit the form, it will submit (and also show at that point) the last valid values it had before the invalid ones.

Any other options you see, that would be a good solution?

ppisljar · 2017-11-07T10:53:36Z

c) allow submitting crossing ranges, but turn them around on submit ? (so 50 - 5 turns into 5 - 50)

none of the options seem perfect ...

cjcenizal · 2017-11-07T19:05:22Z

I think option A is my expected behavior. If I enter invalid input it will show up in the input fields with an error state and an error message, but the invalid input won't be reflected in the slider and won't be saved if I try to save the visualization.

thomasneirynck · 2017-11-07T22:12:50Z

can we use this PR to address #11406 as well?

timroes · 2017-11-08T08:33:53Z

@thomasneirynck I would rather have a look at #14565 which is most likely the cause of that issue. I thought I checked it against 5.6 and thought it doesn't occur there. But I could check all 5.x versions for the issue again and backport #14565 appropriate.

This reverts commit 575dfac.

timroes · 2017-11-10T15:43:31Z

@cjcenizal The behavior you are describing would actually be more option b not a :-) Option B would also align with other suggestions, so I basically reverted the change to use option B again, and we should consider this again when building our own ranged slider. So would welcome some reviews :-)

thomasneirynck · 2017-11-13T15:14:50Z

#9168 will be a separate effort. For now, this will solve the immediate pain points of this control.

cjcenizal

UX feels great! I tested with VoiceOver and it interprets the inputs clearly. I had some suggestions wrt to using UI Framework classes. Here's how it ends up looking:

We can expand the width of kuiTextInput--small in the UI Framework to accommodate all 3 digits cleanly, if you want.

cjcenizal · 2017-11-13T18:31:59Z

src/core_plugins/tagcloud/public/tag_cloud_vis_params.html

@@ -11,6 +11,32 @@
    <div class="form-group">
      <label>Font Size</label>
      <div class="tag-cloud-fontsize-slider"></div>
+      <div class="tag-cloud-fontsize-inputs">


You can replace this class with kuiBar.

cjcenizal · 2017-11-13T18:32:37Z

src/core_plugins/tagcloud/public/tag_cloud_vis_params.html

@@ -11,6 +11,32 @@
    <div class="form-group">
      <label>Font Size</label>
      <div class="tag-cloud-fontsize-slider"></div>
+      <div class="tag-cloud-fontsize-inputs">
+        <div class="tag-cloud-fontsize-input">


And then you can replace this class and the one on line 27 with kuiBarSection. Then you'll be able to delete these styles from tag_cloud.less.

cjcenizal · 2017-11-13T18:35:50Z

src/core_plugins/tagcloud/public/tag_cloud_vis_params.html

+            type="number"
+            min="1"
+            max="{{vis.params.maxFontSize}}"
+            class="form-control tag-cloud-fontsize-inputfield"


I think we can replace these classes with kuiTextInput kuiTextInput--small.

cjcenizal · 2017-11-13T18:42:37Z

src/core_plugins/tagcloud/public/tag_cloud_vis_params.html

+            aria-label="Maximum tag font size"
+            aria-describedby="tagCloudFontSliderMaxUnit"
+          >
+          <span id="tagCloudFontSliderMaxUnit">px</span>


Screen readers read this as "P, X". Should we add aria-label="pixels" to make it clearer?

thomasneirynck

Looks good.

One minor suggestion, not deal breaker:

Typing in an invalid value shows a red outline. Works good, but then perhaps we should disable the play-button and show some feedback in the little error-icon next to it, similar to other places in visualize?

Another approach, but maybe we can auto-justify the input to 1 and 100, and auto-update any value that exceeds this.

timroes · 2017-11-14T17:05:57Z

@cjcenizal I updated the PR for another review. I just didn't use the kuiTextInput--small class, since we accept 100 as max value, which wouldn't fit into that, so I still wanted to have a view more pixel width on these fields. Hope that's okay with you?

cjcenizal

@timroes LGTM! Re the input, we can change the kuiTextInput--small size in the UI Framework itself, so that it fits "100" as an input. I think you've exposed a flaw with that component which we can/should fix but it's not a big deal.

* Refactor font slider for accessibility, fix elastic#12905 * Limit input fields min/max values * Prevent crossing of min/max values * Revert "Prevent crossing of min/max values" This reverts commit 575dfac. * Use UI framework classes

* Refactor font slider for accessibility, fix #12905 * Limit input fields min/max values * Prevent crossing of min/max values * Revert "Prevent crossing of min/max values" This reverts commit 575dfac. * Use UI framework classes

Tim Roes added 2 commits November 7, 2017 09:20

Refactor font slider for accessibility, fix elastic#12905

2b8b812

Limit input fields min/max values

f5e229a

timroes added Feature:Tagcloud Tag cloud visualization feature Feature:Visualizations Generic visualization features (in case no more specific feature label is available) Project:Accessibility v6.1.0 v7.0.0 labels Nov 7, 2017

timroes requested review from cjcenizal, thomasneirynck, ppisljar and alexfrancoeur November 7, 2017 08:28

Prevent crossing of min/max values

575dfac

Revert "Prevent crossing of min/max values"

b4bc5a1

This reverts commit 575dfac.

cjcenizal suggested changes Nov 13, 2017

View reviewed changes

thomasneirynck approved these changes Nov 13, 2017

View reviewed changes

Use UI framework classes

75c4c61

cjcenizal approved these changes Nov 14, 2017

View reviewed changes

timroes merged commit 815cfe1 into elastic:master Nov 15, 2017

timroes deleted the fontslider-a11y branch November 15, 2017 13:36

timroes added the release_note:enhancement label Nov 15, 2017

timroes mentioned this pull request Nov 15, 2017

[6.x] [Accessibility] Refactor font slider for accessibility (#14817) #14961

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Accessibility] Refactor font slider for accessibility #14817

[Accessibility] Refactor font slider for accessibility #14817

timroes commented Nov 7, 2017

timroes commented Nov 7, 2017

ppisljar commented Nov 7, 2017

timroes commented Nov 7, 2017

ppisljar commented Nov 7, 2017 •

edited

Loading

timroes commented Nov 7, 2017

ppisljar commented Nov 7, 2017 •

edited

Loading

cjcenizal commented Nov 7, 2017

thomasneirynck commented Nov 7, 2017

timroes commented Nov 8, 2017

timroes commented Nov 10, 2017

thomasneirynck commented Nov 13, 2017

cjcenizal left a comment •

edited

Loading

cjcenizal Nov 13, 2017

cjcenizal Nov 13, 2017

cjcenizal Nov 13, 2017

cjcenizal Nov 13, 2017

thomasneirynck left a comment

timroes commented Nov 14, 2017

cjcenizal left a comment

[Accessibility] Refactor font slider for accessibility #14817

[Accessibility] Refactor font slider for accessibility #14817

Conversation

timroes commented Nov 7, 2017

timroes commented Nov 7, 2017

ppisljar commented Nov 7, 2017

timroes commented Nov 7, 2017

ppisljar commented Nov 7, 2017 • edited Loading

timroes commented Nov 7, 2017

ppisljar commented Nov 7, 2017 • edited Loading

cjcenizal commented Nov 7, 2017

thomasneirynck commented Nov 7, 2017

timroes commented Nov 8, 2017

timroes commented Nov 10, 2017

thomasneirynck commented Nov 13, 2017

cjcenizal left a comment • edited Loading

Choose a reason for hiding this comment

cjcenizal Nov 13, 2017

Choose a reason for hiding this comment

cjcenizal Nov 13, 2017

Choose a reason for hiding this comment

cjcenizal Nov 13, 2017

Choose a reason for hiding this comment

cjcenizal Nov 13, 2017

Choose a reason for hiding this comment

thomasneirynck left a comment

Choose a reason for hiding this comment

timroes commented Nov 14, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

ppisljar commented Nov 7, 2017 •

edited

Loading

ppisljar commented Nov 7, 2017 •

edited

Loading

cjcenizal left a comment •

edited

Loading