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.

Sign up for GitHub

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

Jump to bottom

[amp-story] ♿ Make pause/play buttons keyboard focusable #33214

Merged
merged 4 commits into from
Mar 19, 2021
Merged

[amp-story] ♿ Make pause/play buttons keyboard focusable #33214

merged 4 commits into from
Mar 19, 2021

Conversation

Enriqe
Copy link
Contributor

@Enriqe Enriqe commented Mar 11, 2021 •
edited

Closes #32508

Makes pause/play buttons in the system layer focusable via keyboard.

It uses the amp-mode-keyboard-active AMP CSS class to display the button when the keyboard is active.

Outdated comments before 87fed97:

  • Buttons cannot be focused if they are display: none or visibility: hidden, so we have to render them and hide them another way.
  • The other two options to hide the buttons is using opacity or width. I tried using opacity: 0 initially, but it would create an empty space between the other buttons in the system layer.
  • Modifying the width is usually frowned upon since it causes CLS and negatively impacts performance. Alternative would be to use a transform: scaleX(0), but it would still create the space between the other buttons.

@amp-owners-bot
Copy link

amp-owners-bot bot commented Mar 11, 2021 •
edited

Hey @gmajoulet, @newmuis! These files were changed:

extensions/amp-story/1.0/amp-story-store-service.js
extensions/amp-story/1.0/amp-story-system-layer.css
extensions/amp-story/1.0/amp-story-system-layer.js
extensions/amp-story/1.0/amp-story.js

@gmajoulet
Copy link
Contributor

Modifying the width is usually frowned upon since it causes CLS and negatively impacts performance.

There is no shift if the content is rendered with the correct width, are you saying there's a layout shift with your implementation? We need to get the initial render right.
From reading the code I don't think there's any shift. If there is please look into other ways to get the element out of the CSS flow, such as position: absolute.

Deferring to @processprocess for the review

@processprocess
Copy link
Contributor

Let's also check amp-mode-keyboard-active in addition to focus.
That way it will stay visible after keyboard interaction.

I think using width is okay since we're not animating it.

@Enriqe
Copy link
Contributor Author

Enriqe commented Mar 15, 2021 •
edited

@processprocess thanks for the suggestion on using amp-mode-keyboard-active, it actually simplifies the CSS this way.

I did have to add some code in order for the system layer to be able to read that CSS class in the shadow DOM:

  1. Add a mutation observer in amp-story to listen for mutation in classes inside the <body> element.
  2. Update the store for the relevant state update.
  3. If the desired CSS is modified, update the shadow CSS.

PTAL

@processprocess
Copy link
Contributor

I did have to add some code in order for the system layer to be able to read that CSS class in the shadow DOM

Looks good to me. As we discussed, with @newmuis, if we find that we need to pass other classes to the shadow dom element, having a helper that appends classes from an allow list could work. For now I think this is implementation is good though!

@Enriqe Enriqe merged commit 8b4a5f8 into ampproject:master Mar 19, 2021
@gmajoulet
Copy link
Contributor

Can the play/pause button be focused by a screen reader? If not, can you re-open the ticket please?

@processprocess processprocess mentioned this pull request Mar 22, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@processprocess processprocess processprocess approved these changes

Assignees
No one assigned
Labels
PR use: In Beta / Experimental PR use: In LTS PR use: In Stable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

amp-story: Make play/pause button focusable on mobile UI
4 participants
@Enriqe @gmajoulet @processprocess @ampprojectbot