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

Review Multi-Thumb Slider Example #520

Closed
3 tasks done
mcking65 opened this issue Nov 7, 2017 · 10 comments
Closed
3 tasks done

Review Multi-Thumb Slider Example #520

mcking65 opened this issue Nov 7, 2017 · 10 comments
Labels
Example Page Related to a page containing an example implementation of a pattern

Comments

@mcking65
Copy link
Contributor

mcking65 commented Nov 7, 2017

The multi-thumb slider example
is ready for review.

Task Force Reviews requested as of Nov 7, 2017

@mcking65 mcking65 added Example Page Related to a page containing an example implementation of a pattern Needs Review labels Nov 7, 2017
@mcking65 mcking65 added this to the 1.1 APG Release 1 milestone Nov 7, 2017
@devarshipant
Copy link

  1. The range on the first slider is from 0 - 400, but the title attribute says 'Select...0 and 500'.

<div title="Select a hotel between $0 and $500" class="rail" style="width: 300px;"> <img ...</div>

  1. Title attribute on div is not read by JAWS (IE 11 / JAWS 18 [with default settings]), and probably not supposed to be, but could be helpful information to convey to screen reader users. Note -- If the title is added to the IMG tag, JAWS picks it up (it speaks aria-label -> aria-value -> title attribute).

Works nicely with JAWS 18 / IE 11.

@annabbott
Copy link

annabbott commented Nov 13, 2017

All text in this example appears to be 'bold', which is not the case in other example pages. No other issues noted.

mcking65 added a commit that referenced this issue Nov 16, 2017
Perfeedback in issue #520, modified examples/slider/multithumb-slider.html:
1. changed the text of the title attribute on the hotel slider. to say  max is $400
2. Fixed incorrectly closed strong tag.
@mcking65
Copy link
Contributor Author

mcking65 commented Nov 16, 2017

Thank you @devarshipant and @annabbott for the review.

I pushed commit 1d23c41 for these issues.

@devarshipant commented:

  1. The range on the first slider is from 0 - 400, but the title attribute says 'Select...0 and 500'.

Corrected the title attribute.

  1. Title attribute on div is not read by JAWS (IE 11 / JAWS 18 [with default settings]), and probably not supposed to be,
    but could be helpful information to convey to screen reader users.
    Note -- If the title is added to the IMG tag, JAWS picks it up (it speaks aria-label -> aria-value -> title attribute).

JAWS 18 is reading the title for me on the div. NVDA does not.

I suppose we could add a hidden element for aria-describedby.

@annabbott commented:

All text in this example appears to be 'bold', which is not the case in other example pages. No other issues noted.

It was an incorrectly closed <strong> tag.

@devarshipant
Copy link

@mcking65: JAWS 18 is reading the title for me on the div. NVDA does not. I suppose we could add a hidden element for aria-describedby.

Not sure why I get opposite results (NVDA works, while JAWS does not). However, a quick check by adding tabindex 0 on div resolves the issue, unless we take the aria-describedby route. Something like:
<div tabindex="0" title="Select a hotel price range between $0 and $400" class="rail" style="width: 300px;"> ... </div>
BTW -- I think it should be okay even if we don't change it.
thanks!

@jnurthen
Copy link
Member

@devarshipant This is not meant to be focusable so I don't want to add tabindex=0 - that would put it in the focus order which is not desirable. Things without a role should not be in the focus order.

@sh0ji
Copy link
Contributor

sh0ji commented Nov 20, 2017

The title attribute doesn't have any kind of accessibility mapping according to the current HTML AAM for title, and should not be read by screen readers that strictly conform. The inconsistency across screen readers is because they're all employing different heuristics to know when to read it. Those heuristics have always been rather opaque to me.

Echoing @devarshipant, I think it'd be good to expose the title text in a more conformant way (aria-describedby, perhaps), or remove it entirely if it's non-essential to this example.

Everything else looks good to me on VoiceOver and NVDA.

@mcking65
Copy link
Contributor Author

I think the title is not necessary and the example would be more clear without it. The purpose of this example is to demonstrate making a slider; we can do that more effectively if the example is simpler.

PR welcome.

@devarshipant
Copy link

@sh0ji -- thanks for the insight. The value of the title attribute "Select a hotel price range between $0 and $400" complements the slider, which is meaningful text when exposed to screen reader users.

mcking65 pushed a commit that referenced this issue Nov 28, 2017
Per discussion in issue #520, remove the title attribute from the slider thumbs.
@mcking65
Copy link
Contributor Author

Per discussion in the Monday, Nov 27, 2017 meeting, we are removing the title attribute.

mcking65 added a commit that referenced this issue Nov 28, 2017
Modified examples/slider/multithumb-slider.html to remove link to review issue #520.
The task force review process is complete.
@mcking65
Copy link
Contributor Author

All work is done on this issue. Thank you everyone, especially @jongund for all the coding work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Example Page Related to a page containing an example implementation of a pattern
Development

No branches or pull requests

5 participants