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

Added option to enable focus on slide change #3032

Merged
merged 2 commits into from
Aug 4, 2017
Merged

Added option to enable focus on slide change #3032

merged 2 commits into from
Aug 4, 2017

Conversation

leggomuhgreggo
Copy link
Collaborator

@leggomuhgreggo leggomuhgreggo commented Aug 4, 2017

There've been issues about the jump resulting from focus being applied after slide change.

Cases that've been identified as problematic:

  • Slick methods that change the slide
  • Slides that are partially obscured by the top of the browser

I've (reluctantly) added a new option called focusOnChange which defaults to false, and updated the docs.

In order to achieve the a11y focus after change, this setting needs to be enabled.

Demos:
Before & After

Closes #3020

@leggomuhgreggo
Copy link
Collaborator Author

@cielt Hey wanted to loop you in on this. I know it's not the most appetizing prospect from the perspective of an a11y stalwart, but it's become pretty clear that it's an issue. Let me know if you have any thoughts, and, as always, thanks!

@cielt
Copy link
Contributor

cielt commented Aug 4, 2017

@leggomuhgreggo: solid fix from my perspective. totally appreciate that setting focus is an opinionated and potentially disruptive default people might not want the visual effects of depending on use case. thanks for resolving this and updating the docs! 👍

@cielt
Copy link
Contributor

cielt commented Aug 4, 2017

(apologies -- should have included this in the prev note but was having connectivity issues)

@leggomuhgreggo: just a couple of small follow-up thoughts / questions:

  1. any plans to do something with focusOnSelect? i think with focusOnChange the a11y concern is covered; selecting (if this means clicking or otherwise interacting with an element that is able to receive focus) will by default transfer browser focus to that element (e.g., a slick dot or arrow button). Implementing this change of focus programmatically is only necessary where focus is not transferred by the browser automatically. My suggestion would be to remove unless you have other designs for this;
  2. we can remove the comment, around line 1700, // for non-autoplay: once active slide (group) has updated, set focus on first newly showing slide;
  3. agree with the README update for the accessibility option referencing focusOnChange; if it may help, i would suggest adding a note to the focusOnChange description, explaining that this is so that screen readers are prompted to call attention to the slide newly showing;
  4. depending on what you decide in 1 above, description of focusOnSelect may not be needed.

Let me know if any questions. And thanks!

@leggomuhgreggo
Copy link
Collaborator Author

Cool, I'll look into deprecating focusOnSelect, and in the meantime remove it from the docs (it was absent before this PR, as a matter of fact). Also removed that comment. Thanks @cielt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants