-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
♿ [amp-story] Allow screen reader users to pause auto-advancing stories #37582
Conversation
Hey @gmajoulet, @newmuis! These files were changed:
|
Can you add a screenshot for the keyboard state? Also, I think we're better off using |
I see, I think I was assuming the button would show on the top left corner but it just shows and hides it in-place, so some of my comments are no longer relevant. Q1: Why do we need to add the 1px styles if using the keyboard will show it anyways? |
Sorry I think maybe I didn't do a great job describing this initially.
The problem is that screen readers do not activate amp's keyboard mode, so the button is invisible both to the eye, and the screen reader. This change sets the SR only css as default, but then removes it if keyboard mode || desktop.
I am able to repro on desktop. If I switch to mobile I see the keyboard active class set as expected. I wonder if this is supposed to be a mobile only thing? Either way I'm not sure it matters for this case as the button is always visible on desktop anyway. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Updated description to reflect the latest logic. |
Introduces new css that makes the pause button invisible to users, but accessible to screen readers. It then removes these properties if the button is visual shown (desktop, keyboard active mode)Introduces CSS selector that will make pause button intractable when screen readers are used.
:focus
pseudo class as expected.Partial #37393