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

Accessibility support #198

Closed
gorner opened this issue Jul 31, 2018 · 46 comments
Closed

Accessibility support #198

gorner opened this issue Jul 31, 2018 · 46 comments

Comments

@gorner
Copy link

gorner commented Jul 31, 2018

I'm part of a team that has been looking at using Shepherd (and ember-shepherd) within one of our externally-facing Ember sites, however we've noticed that the accessibility support is fairly minimal - there are no ARIA attributes on any Shepherd elements and the semantic markup could be improved (no nav tag around the navigation buttons, for example).

I know there was an issue #26 raised along these lines but it was closed without explanation late last year.

I realize that this is a period of transition in terms of the stewardship of Shepherd but I was wondering if any efforts were being planned in this regard.

@RobbieTheWagner
Copy link
Member

Hi @gorner, thanks for bringing this to our attention. We did not have any specific plans, but a11y is a good goal for all projects these days. If you have a set of things you'd like us to look at and/or wanted to help implement some of those things, we'd be glad to take a look 😃

@gorner
Copy link
Author

gorner commented Aug 3, 2018

Fair enough, I'm planning to take a look in the next few days/weeks to better articulate what we'd be looking for.

@RobbieTheWagner
Copy link
Member

@gorner any updates on the sort of accessibility things you are looking for?

@BrianSipple @chuckcarpenter do you guys have any ideas?

@gorner
Copy link
Author

gorner commented Sep 23, 2018

@rwwagner90 Thanks for the reminder 🙂

We've had a few suggestions from our a11y SMEs recently – to be clear, our evaluation is still ongoing and my time to play around with this myself has been limited. A brief summary of what where we've gotten thus far:

  • We should be able to set an ARIA role on the main parent element of the step content (it's a bit unclear to us if this should be .shepherd or .shepherd-content). Perhaps this attribute should be set to a appropriate default with ability to override. For modal scenarios, "dialog" or "alertdialog" might make sense. In our case we're planning to use the non-modal option which may imply "alert". I note that the similar intro.js library uses "dialog".
    • For the non-alert, non-modal case, we may need to be able to separately set an aria-live value.
  • Parent element should have an aria-describedby attribute (with the id of the .shepherd-text element); and if applicable, an aria-labelledby attribute (with the id of the .shepherd-title element)
  • When used as a modal, tabindex="0" may need to be set on parent element
  • Buttons (that perform actions without linking elsewhere) should be coded as button elements, not as a tags. One of our SMEs also suggested these should be put into the footer directly without an intermediary ul.

Other points that are less clear-cut:

  • The main content div might make sense as a <section> tag since it is a thematic grouping of content. However in this case it should have a heading in which case the title would need to be mandatory (or perhaps it should be setup such that it's only a section if the title is present, and leave as a div otherwise).
  • Because of how Shepherd structures its DOM, we may need some ability to provide screenreader-only content and/or action/link, to direct users with low/no sight to the actual feature. In theory the aria-flowto attribute would help handle this but browser support seems to be very limited.
  • One of our SMEs suggested that h2 may be more appropriate for the heading than h3, but I realize that this may be a matter of interpretation.
  • Re my earlier comments about whether the buttons should be contained within a nav tag, we are still examining whether this is needed. The role="navigation" attribute might also make sense for this.

@gorner
Copy link
Author

gorner commented Oct 1, 2018

One other a11y item - the option to close the tour when clicking outside the tour area or pressing the Esc key (as suggested in #141) would also be helpful for us, though as noted in that issue, those parts at least are feasible to implement on a per-app basis.

@RobbieTheWagner
Copy link
Member

@BrianSipple do we get some of this for free with the conversion to tippy?

@BrianSipple
Copy link
Contributor

@rwwagner90 Tippy has a minor perk where it adds tabindex=0 to the target element, but that's it. I think the list @gorner provided is worth going through to see what we can address. I'm open as to whether or not we want to target that as part of the 2.0 release, or 2.x or 3.0.

@RobbieTheWagner
Copy link
Member

@BrianSipple I think we can do it in 2.x or 3.0. It should be all additional stuff, hopefully nothing breaking.

@BrianSipple BrianSipple added this to To do in 2.x via automation Oct 9, 2018
@stale
Copy link

stale bot commented Jan 13, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jan 13, 2019
@stale stale bot closed this as completed Jan 20, 2019
2.x automation moved this from To do to Done Jan 20, 2019
2.x automation moved this from Done to In progress Jan 21, 2019
@stale stale bot removed the wontfix label Jan 21, 2019
@knoobie
Copy link
Contributor

knoobie commented Feb 19, 2019

Do you have any news about this feature? If you have a working draft, I would love to let our sightless user test it and get you some feedback if that helps.

@RobbieTheWagner
Copy link
Member

@knoobie I'm not really sure of what things to do here, but would love to work with you to determine a list of tasks we should implement!

@knoobie
Copy link
Contributor

knoobie commented Feb 19, 2019

@rwwagner90 Awesome! Gonna ask my management tomorrow.

I'm going to send you a follow up e-mail to your gmail (linked in your GitHub profile) in the next days. Thanks for your quick response!

@stale
Copy link

stale bot commented Mar 21, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Mar 21, 2019
@gorner
Copy link
Author

gorner commented Mar 22, 2019

Would like to keep this open if possible.

@stale stale bot removed the wontfix label Mar 22, 2019
@chuckcarpenter
Copy link
Contributor

chuckcarpenter commented Mar 22, 2019

@gorner we would also love to support this. I think where we need help in the very least of generating the list of items of what that should be. Are the initial items from
#198 (comment) enough? Otherwise, PRs also welcome!

@gorner
Copy link
Author

gorner commented Mar 23, 2019

@chuckcarpenter mostly just trying to keep the issue open! We did end up putting shepherd into our Ember app with a bunch of extra jQuery hooks to get the a11y support we needed. We're still looking into how much we'd be able to contribute back.

@RobbieTheWagner
Copy link
Member

@gorner I would love to hear about these extra jQuery hooks. We should definitely incorporate the functionality into Shepherd. This stale bot keeps trying to close things in 30 days, so thanks for commenting to keep things open!

If you could share specific things you implemented in small digestible chunks, I would be glad to port them into Shepherd 👍

@stale
Copy link

stale bot commented Apr 24, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Apr 24, 2019
@RobbieTheWagner
Copy link
Member

@gorner @knoobie I believe most everything is now implemented. I will be releasing a new version today, and perhaps you guys could help us test it out?

@RobbieTheWagner
Copy link
Member

@gorner @knoobie I released 3.1.0, and I would very much like to have you guys try things out! Would you be able to give it a try?

@knoobie
Copy link
Contributor

knoobie commented Jun 25, 2019

Have updated our project! Need some time to test it properly with our sightless guy. Currently it's summer vacation here

@gorner
Copy link
Author

gorner commented Jun 25, 2019

@rwwagner90 Thanks for the update. I've actually moved on from the company that was using this - sorry for the delay in noting this. Not sure if they'll have bandwidth to test this anytime soon but I'll try looping in @gregmuck just in case.

@RobbieTheWagner
Copy link
Member

@knoobie sure thing, just let us know when you have tested it and what changes we should make!

@gorner no worries!

@gregmuck we would be happy to make any adjustments necessary for this to work better for you!

@segux
Copy link

segux commented Jul 1, 2019

Hi, im testing keyboard support, but i think is not stable.

Should be optionally disabled, on tour options or on step options.

Like:

// Tour
{
  off: {keydownBack: ()=>return false}
}

// Step
{
  when: {keydownBack: ()=>return false}
}

@RobbieTheWagner
Copy link
Member

@segux what do you mean it is not stable? I think we need to leave it enabled, for a11y support.

@segux
Copy link

segux commented Jul 3, 2019

@rwwagner90 Sorry for late response.

Here ill explain what is going wrong on keyboard navigation.

I made simple example on stackblitz:
https://stackblitz.com/edit/shepherdjs-keyboard-test?file=index.html

Steps to reproduce:

  1. Click to start tour button
  2. press right arrow
  3. press left arrow
  4. press right arrow
  5. press left arrow
  6. press right arrow

Expected result:

  1. Tour starts
  2. goes to second step
  3. backs to first step
  4. go to second step
  5. go to first step
  6. go to second step

Obtained result:

  1. Tour starts
  2. Go to second step
  3. Back to first step
  4. Go to third step
  5. Go to second step
  6. Ends magically the tour and cant enter to fourth step

Probably is not saving the current position of tour.
And this is another thing that i missed. Because usually users want to know the context of tour viewing current step and total steps. And now i cant show that.

@RobbieTheWagner
Copy link
Member

@segux I noticed the issues a couple days ago, and fixed them. I was registering the listener in the wrong place, so every time we called show a new listener was added, resulting in skipping multiple steps ahead. The latest beta has the fix, and I hope to get 4.0.0 out soon.

@segux
Copy link

segux commented Jul 4, 2019

And what do you think about have properties for total steps number and current step?

@RobbieTheWagner
Copy link
Member

@segux that is a separate discussion, this issue is about a11y support. If you have other things you would like to suggest as features, please feel free to open new issues 😃

@RobbieTheWagner
Copy link
Member

@knoobie any updates?

@knoobie
Copy link
Contributor

knoobie commented Jul 13, 2019

@rwwagner90 summer vacation is 4-6 weeks in my country :) kinda hard to get my coworker in this time

@RobbieTheWagner
Copy link
Member

@knoobie ah okay, I wish I could take 4-6 weeks of vacation in a row! 😃

@RobbieTheWagner
Copy link
Member

@knoobie when do you think you guys can take a look at this? Just trying to get an idea. I would like to ensure a11y is good to go before we release 4.0

@knoobie
Copy link
Contributor

knoobie commented Jul 26, 2019

@rwwagner90 gonna try to get feedback by the end of next week.

@knoobie
Copy link
Contributor

knoobie commented Jul 26, 2019

@rwwagner90 I have seen that you have updated your demo page. Just an idea / note: For sightless people it would be better when the tour can be started by hand (click on a button) instead of "on load" - that's kinda annoying and not intuitive.

@RobbieTheWagner
Copy link
Member

@knoobie it's just the demo page. When implementing in another page it can be triggered however you would like.

@knoobie
Copy link
Contributor

knoobie commented Jul 26, 2019

@rwwagner90 I know - we do this. Just as enhancement for your demo to show people other ways of opening it and allowing for better usage of your demo as show case. You know, developer are lazy people ;)

@knoobie
Copy link
Contributor

knoobie commented Jul 30, 2019

Got finally some feedback for you:

  • The 'close' a href next to the h3 should be something like <button type="button" class="close" aria-label="Close"><span aria-hidden="true">&times;</span></button> instead
  • CSS Highlighting of the buttons (:focus) doesn't work properly using the keyboard

Nice to have feature:

  • allow user to configure what heading level they want (h1-h6)

Didn't tested:

  • closing the tour should move the focus back to the component that opened it

Overall - pretty good upgrade from 2.x 👍 (using correct buttons, better focus handling, trapping the user in the modal / focus circling and so on)

@RobbieTheWagner
Copy link
Member

@knoobie thanks for the feedback!

CSS Highlighting of the buttons (:focus) doesn't work properly using the keyboard

I'm confused on this. What is not working?

The 'close' a href next to the h3 should be something like × instead

I just made these changes and released 4.1.0, thanks!

@knoobie
Copy link
Contributor

knoobie commented Jul 30, 2019

Steps to reproduce:

Opening: https://shepherdjs.dev/demo/
Navigate only using "TAB" on the keyboard

  • "x" gets focused (you can visually see it)
  • "Exit" gets focused (you can visually see it)
  • "Next" gets focused (you can't visually see it)

You can read more about it here

@RobbieTheWagner
Copy link
Member

@knoobie I get the blue outline on all the elements. The next button takes maybe half a second to show up, maybe due to animations, but I see it.

@knoobie
Copy link
Contributor

knoobie commented Jul 31, 2019

@rwwagner90 what Browser did you use? I'm on Firefox 69 (Ubuntu / MacOS)
IE 11 on a VM looks okay for me as well.

@RobbieTheWagner
Copy link
Member

@knoobie it appears FF for some reason highlights the text inside the button, instead of the button. It still works though. If you tab to next then hover over it, you can see it is focused. It's just hard to see because the FF focused outline blends in with the button color.

@RobbieTheWagner
Copy link
Member

I believe most of this has been addressed. If there are additional issues, please feel free to open new ones!

Also, @knoobie what is your company? We would love to feature your Shepherd usage in our users 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
2.x
  
In progress
Development

No branches or pull requests

6 participants