♿ [Story a11y] Improve tabbing on social-share and bookend #32859

mszylkowski · 2021-02-24T15:21:19Z

Part of #31747
Mentioned in #32511

When tabbing through the story, landing on the share icons or the bookend links will scroll the viewport outside the story area.

Changes:

When showing or hiding the share menu, toggle the share icons to tabbable.
Toggle the links on the bookend to tabbable when bookend is open
Bring back focus state:

@ampproject/wg-stories

amp-owners-bot · 2021-02-24T19:34:33Z

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

extensions/amp-story/1.0/amp-story-bookend.css
extensions/amp-story/1.0/amp-story-draggable-drawer.css
extensions/amp-story/1.0/amp-story-draggable-drawer.js
extensions/amp-story/1.0/amp-story-page-attachment.css
extensions/amp-story/1.0/amp-story-share-menu.css
extensions/amp-story/1.0/amp-story-share.css
extensions/amp-story/1.0/amp-story-share.js
extensions/amp-story/1.0/amp-story-vertical.css

processprocess · 2021-02-24T19:59:25Z

An alternate approach:
putting visibility: hidden on the parent when it's not open might prevent the elements from being tabbable.

It may have side effects depending on which node is being transformed.
I'm also not sure how it works with shadow dom. It might need to be on a parent that's in the same shadow dom.

Do you think there's value in trying that? It might be less code.

mszylkowski · 2021-02-24T20:02:27Z

@processprocess Absolutely, let me try that. I suspect that the animations would break since we're hiding the element when not visible

mszylkowski · 2021-02-24T21:17:23Z

@processprocess seems like the shadow dom does interfere but it was worth the effort to get it working through the shadow dom since the code is much clearer this way, PTAL. Animations don't break since the visibility attribute changes gradually

processprocess · 2021-02-25T15:34:34Z

This looks great to me. Thanks Matias :)

I noticed "Get link" gets skipped when tabbing. It looks like screen readers don't recognize it. I think that needs to be fixed.
It's somewhat related to this PR. Do you think it makes sense to do that work in this PR or split it into a new one?

extensions/amp-story/1.0/amp-story-bookend.css

gmajoulet

We already set aria-hidden=bool from the draggable-drawer class, that's supposed to take care of this

amphtml/extensions/amp-story/1.0/amp-story-draggable-drawer.js

Line 505 in 476d99c

this.element.setAttribute('aria-hidden', false);

Why is this not working? Let's fix it instead of introducing yet another way to do the same thing

mszylkowski · 2021-02-25T16:09:01Z

I noticed "Get link" gets skipped when tabbing. It looks like screen readers don't recognize it. I think that needs to be fixed.
It's somewhat related to this PR. Do you think it makes sense to do that work in this PR or split it into a new one?

@processprocess I'll check if the "copy to clipboard" can be triggered from the keyboard, and if it's a couple of lines of code we can do it here.

We already set aria-hidden=bool from the draggable-drawer class, that's supposed to take care of this

@gmajoulet https://web.dev/aria-hidden-focus/ "Using the aria-hidden="true" attribute on an element hides the element and all its children from screen readers and other assistive technologies. If the hidden element contains a focusable element, assistive technologies won't read the focusable element, but keyboard users will still be able to navigate to it, which can cause confusion."

We may be able to remove the aria-hidden from this if we have visibility: hidden, but for tabbing through the story, the aria-hidden doesn't disable the focusable elements (it does remove them from the a11y tree, but that's different)

gmajoulet · 2021-02-25T16:16:03Z

That's good to know thank you. So let's write the fix at the same level so the logic is not split in between two files.

The draggable drawer sets aria-hidden to true/false on open/close, and it also sets a i-amphtml-story-draggable-drawer-open class that you can rely on to set the visibility.
I suspect that setting the visibility on the Shadow host might not work and you could still tab through the content within the Shadow root. I won't get into specifics of shadow dom style scoping, but I think adding :host {visibility: inherit !important;} in both amp-story-bookend.css and amp-story-page-attachment.css will make it work.

TLDR: add the visibility rule on i-amphtml-story-draggable-drawer-open, and make sure the :host inherits it in each CSS file.

mszylkowski · 2021-02-25T17:44:36Z

@gmajoulet had to deal with transitioning the visibility with a delay so it doesn't break the animations, but worked. Thanks for the suggestion! PTAL at the fix.

extensions/amp-story/1.0/amp-story-share.css

extensions/amp-story/1.0/amp-story-share.js

extensions/amp-story/1.0/amp-story-bookend.css

…tabinactive_bookend

…/amphtml into ampstory_tabinactive_bookend

processprocess

Approving with a couple nits/suggestions.

processprocess · 2021-02-26T17:44:42Z

extensions/amp-story/1.0/amp-story-share.css

-
-.i-amphtml-story-share-icon:active::before {
-  background-color: #787878 !important;
+  outline: 2px solid #0389ff !important;


nit:
It's best practice to not override default browser state for a11y. I think we can remove this override.
The outline below it is a nice touch though to help it be visible.

It might be worth putting a comment above this block stating that it's for a11y.

The default for these elements (being custom elements / divs and not necessarily buttons) is to not have anything for the focus state, so this is super useful to know what the user is focusing on. I can add a comment, but I wonder if it's not clear enough if it's inside a rule with the :focus selector

processprocess · 2021-02-26T17:47:45Z

extensions/amp-story/1.0/amp-story-share.js

@@ -277,6 +289,13 @@ export class ShareWidget {
      e.preventDefault();
      this.copyUrlToClipboard_();
    });
+    listen(linkShareButton, 'keyup', (e) => {
+      const code = e.charCode || e.keyCode;
+      if (code === 32 || code === 13) {


nit:
I wonder if this needs a comment for what keys these are for quick readability.
I think we commonly do top level CONSTs but maybe that's overkill for this.

Sounds good to add a comment for this, it's not immediately obvious what these are

Started updating tabindex

e7d33c8

mszylkowski mentioned this pull request Feb 24, 2021

[Story a11y] Inactive page elements are tabbable #31747

Closed

5 tasks

mszylkowski self-assigned this Feb 24, 2021

mszylkowski added this to In progress in wg-stories Sprint via automation Feb 24, 2021

mszylkowski added 2 commits February 24, 2021 12:49

More work

3e9cb43

Fixed share icons on start

9c882ff

mszylkowski requested a review from processprocess February 24, 2021 19:32

Removed console logs

d72431a

mszylkowski marked this pull request as ready for review February 24, 2021 19:34

mszylkowski added 3 commits February 24, 2021 16:08

Use visibility CSS

c65f3e5

Reverted original code

d861261

Visible -> initial

471064e

processprocess reviewed Feb 25, 2021

View reviewed changes

extensions/amp-story/1.0/amp-story-bookend.css Outdated Show resolved Hide resolved

gmajoulet reviewed Feb 25, 2021

View reviewed changes

mszylkowski added 2 commits February 25, 2021 11:26

Added comment and tabbable get link

7a5f31f

Using root container for visibility

2d950b4

Add visibility on drawer when dragging

cf6ca11

mszylkowski requested review from gmajoulet and processprocess February 25, 2021 17:54

Bring back supports landscape

074de2f

gmajoulet reviewed Feb 25, 2021

View reviewed changes

extensions/amp-story/1.0/amp-story-share.css Show resolved Hide resolved

extensions/amp-story/1.0/amp-story-share.js Outdated Show resolved Hide resolved

i18n of get link

ab49251

mszylkowski requested a review from gmajoulet February 25, 2021 19:00

mszylkowski mentioned this pull request Feb 25, 2021

amp-story: Pages visually break when navigating with keyboard #32506

Closed

gmajoulet approved these changes Feb 25, 2021

View reviewed changes

extensions/amp-story/1.0/amp-story-bookend.css Outdated Show resolved Hide resolved

mszylkowski added 5 commits February 25, 2021 16:21

Added comment for a11y on visibility inherit

27155ef

Added comment again

a8b3ccc

Merge branch 'master' of github.com:ampproject/amphtml into ampstory_…

7b65c8f

…tabinactive_bookend

Merge branch 'ampstory_tabinactive_bookend' of github.com:mszylkowski…

fe0351f

…/amphtml into ampstory_tabinactive_bookend

Fixed rendering of bot

a6ceaa6

processprocess approved these changes Feb 26, 2021

View reviewed changes

Added comment to trigger on enter or space

582b84a

mszylkowski merged commit d2c1006 into ampproject:master Feb 26, 2021

wg-stories Sprint automation moved this from In progress to Done Feb 26, 2021

mszylkowski mentioned this pull request Mar 1, 2021

wg-stories Status Update 2020-02-19 ampproject/wg-stories#60

Closed

mszylkowski removed this from Done in wg-stories Sprint Mar 1, 2021

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels Mar 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

♿ [Story a11y] Improve tabbing on social-share and bookend #32859

♿ [Story a11y] Improve tabbing on social-share and bookend #32859

mszylkowski commented Feb 24, 2021 •

edited

amp-owners-bot bot commented Feb 24, 2021 •

edited

processprocess commented Feb 24, 2021 •

edited

mszylkowski commented Feb 24, 2021

mszylkowski commented Feb 24, 2021 •

edited

processprocess commented Feb 25, 2021 •

edited

gmajoulet left a comment

mszylkowski commented Feb 25, 2021

gmajoulet commented Feb 25, 2021 •

edited

mszylkowski commented Feb 25, 2021 •

edited

processprocess left a comment

processprocess Feb 26, 2021

mszylkowski Feb 26, 2021

processprocess Feb 26, 2021

mszylkowski Feb 26, 2021

♿ [Story a11y] Improve tabbing on social-share and bookend #32859

♿ [Story a11y] Improve tabbing on social-share and bookend #32859

Conversation

mszylkowski commented Feb 24, 2021 • edited

amp-owners-bot bot commented Feb 24, 2021 • edited

processprocess commented Feb 24, 2021 • edited

mszylkowski commented Feb 24, 2021

mszylkowski commented Feb 24, 2021 • edited

processprocess commented Feb 25, 2021 • edited

gmajoulet left a comment

Choose a reason for hiding this comment

mszylkowski commented Feb 25, 2021

gmajoulet commented Feb 25, 2021 • edited

mszylkowski commented Feb 25, 2021 • edited

processprocess left a comment

Choose a reason for hiding this comment

processprocess Feb 26, 2021

Choose a reason for hiding this comment

mszylkowski Feb 26, 2021

Choose a reason for hiding this comment

processprocess Feb 26, 2021

Choose a reason for hiding this comment

mszylkowski Feb 26, 2021

Choose a reason for hiding this comment

mszylkowski commented Feb 24, 2021 •

edited

amp-owners-bot bot commented Feb 24, 2021 •

edited

processprocess commented Feb 24, 2021 •

edited

mszylkowski commented Feb 24, 2021 •

edited

processprocess commented Feb 25, 2021 •

edited

gmajoulet commented Feb 25, 2021 •

edited

mszylkowski commented Feb 25, 2021 •

edited