♿ TetraLogical: P1 components accessibility review by TetraLogicalHelpdesk · Pull Request #31208 · ampproject/amphtml

TetraLogicalHelpdesk · 2020-11-17T14:58:11Z

Suggested changes and additions based on the TetraLogical accessibility review commissioned by @nainar / @caroqliu

As menus in the sidebar are not strictly ARIA menus (but can have a variety of content), they should not have menu-like behaviour. #31175 The behaviour also seems to be currently suppressed (I seem to recall an issue from phase 0 that called for it to be removed, if I'm not mistaken)

The HTML markup list seems to suppress rendering of the backtick code markup - see https://amp.dev/documentation/components/amp-selector/#keyboard-select-mode

CLAassistant · 2020-11-17T14:58:18Z

All committers have signed the CLA.

amp-owners-bot · 2020-11-17T14:58:25Z

Hey @alanorozco! These files were changed:

build-system/server/app-index/amphtml-helpers.js

Hey @ampproject/wg-caching! These files were changed:

extensions/amp-3d-gltf/0.1/test/validator-amp-3d-gltf.html
extensions/amp-3d-gltf/0.1/test/validator-amp-3d-gltf.out
extensions/amp-access-laterpay/0.1/test/validator-amp-access-laterpay.html
extensions/amp-access-laterpay/0.1/test/validator-amp-access-laterpay.out
extensions/amp-access-laterpay/0.2/test/validator-amp-access-laterpay.html
extensions/amp-access-laterpay/0.2/test/validator-amp-access-laterpay.out
extensions/amp-access-poool/0.1/test/validator-amp-access-poool.html
extensions/amp-access-poool/0.1/test/validator-amp-access-poool.out
extensions/amp-access-scroll/0.1/test/validator-amp-access-scroll.html
extensions/amp-access-scroll/0.1/test/validator-amp-access-scroll.out
extensions/amp-access/0.1/test/validator-amp-access-missing-extension.html
extensions/amp-access/0.1/test/validator-amp-access-missing-extension.out
+370 more

Hey @gmajoulet! These files were changed:

extensions/amp-story-360/0.1/test/validator-amp-story-360.html
extensions/amp-story-360/0.1/test/validator-amp-story-360.out
extensions/amp-story-interactive/0.1/test/validator-amp-story-interactive-poll.html
extensions/amp-story-interactive/0.1/test/validator-amp-story-interactive-poll.out
extensions/amp-story-interactive/0.1/test/validator-amp-story-interactive-quiz.html
extensions/amp-story-interactive/0.1/test/validator-amp-story-interactive-quiz.out
extensions/amp-story-interactive/0.1/test/validator-amp-story-interactive-results.html
extensions/amp-story-interactive/0.1/test/validator-amp-story-interactive-results.out
extensions/amp-story-interactive/0.1/test/validator-amp-story-interactive-valid.html
extensions/amp-story-interactive/0.1/test/validator-amp-story-interactive-valid.out
extensions/amp-story-player/0.1/test/validator-amp-story-player.html
extensions/amp-story-player/0.1/test/validator-amp-story-player.out
+59 more

Hey @processprocess, @t0mg! These files were changed:

extensions/amp-story-360/0.1/test/validator-amp-story-360.html
extensions/amp-story-360/0.1/test/validator-amp-story-360.out

Hey @mszylkowski! These files were changed:

extensions/amp-story-interactive/0.1/test/validator-amp-story-interactive-poll.html
extensions/amp-story-interactive/0.1/test/validator-amp-story-interactive-poll.out
extensions/amp-story-interactive/0.1/test/validator-amp-story-interactive-quiz.html
extensions/amp-story-interactive/0.1/test/validator-amp-story-interactive-quiz.out
extensions/amp-story-interactive/0.1/test/validator-amp-story-interactive-results.html
extensions/amp-story-interactive/0.1/test/validator-amp-story-interactive-results.out
extensions/amp-story-interactive/0.1/test/validator-amp-story-interactive-valid.html
extensions/amp-story-interactive/0.1/test/validator-amp-story-interactive-valid.out

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

extensions/amp-story-player/0.1/test/validator-amp-story-player.html
extensions/amp-story-player/0.1/test/validator-amp-story-player.out
extensions/amp-story/1.0/test/validator-amp-story-360.html
extensions/amp-story/1.0/test/validator-amp-story-360.out
extensions/amp-story/1.0/test/validator-amp-story-amp-experiment-error.html
extensions/amp-story/1.0/test/validator-amp-story-amp-experiment-error.out
extensions/amp-story/1.0/test/validator-amp-story-amp-experiment.html
extensions/amp-story/1.0/test/validator-amp-story-amp-experiment.out
extensions/amp-story/1.0/test/validator-amp-story-amp-twitter-error.html
extensions/amp-story/1.0/test/validator-amp-story-amp-twitter-error.out
extensions/amp-story/1.0/test/validator-amp-story-amp-twitter.html
extensions/amp-story/1.0/test/validator-amp-story-amp-twitter.out
+49 more

TetraLogicalHelpdesk · 2020-11-17T14:59:42Z

Note that the find/replace change to add lang="en" to all relevant instances of the demos may have been overzealous in places. Apologies if it turns out to be excessive.

nainar · 2020-11-17T23:13:21Z

@caroqliu to kick of the review?

caroqliu

Thanks for your thorough changes @patrickhlauke! Left a few comments.

build-system/server/app-index/amphtml-helpers.js

caroqliu · 2020-11-18T14:29:47Z

extensions/amp-3d-gltf/0.1/test/validator-amp-3d-gltf.out

 |  -->
 |  <!doctype html>
-|  <html ⚡>
+|  <html ⚡lang="en">


Please update all .out files by running gulp validator --update_tests and do not manually modify.

i'm currently unable to get a working local dev environment up and running for this (some quirk of windows/wsl/ubuntu that's beyond my technical ability to hunt down just now). is it ok to leave these as they are in my PR, and for you to re-update them properly at your end?

extensions/amp-ad-network-fake-impl/0.1/amp-ad-network-fake-impl.js

extensions/amp-auto-ads/amp-auto-ads.md

extensions/amp-base-carousel/amp-base-carousel.md

caroqliu · 2020-11-18T14:34:53Z

extensions/amp-lightbox-gallery/amp-lightbox-gallery.md

-  <amp-img src="image1" width="200" height="100"></amp-img>
-  <amp-img src="image1" width="200" height="100"></amp-img>
-  <amp-img src="image1" width="200" height="100"></amp-img>
+  <amp-img src="image1" width="200" height="100" alt="Image 1"></amp-img>


FMI: Is this type of alt description actually useful?

Only in this instance where it's a placeholder image, to be honest. Deemed it borderline acceptable, mainly due to the sheer number of these sorts of images without alt at all (and the fact that not all of the instances I spotted were easy to find/replace throughout). This sort of thing is certainly more of a stopgap solution - ideally, all examples should have appropriate/more real-world descriptive alternative text.

Makes sense, thank you

caroqliu · 2020-11-18T14:39:22Z

extensions/amp-selector/amp-selector.md

-    `select`: Tab key gives focus to `<amp-selector>`. The selection changes as the user navigates options with arrow keys.
-  </li>
-</ul>
+- `none` (default): The tab key changes focus between items in the `<amp-selector>`. The user must press enter or space to change the selection. Arrow keys are disabled.


@CrystalOnScript Is this HTML removal acceptable for amp.dev?

note that currently, with the html there, it seems amp.dev doesn't then correctly render the backticks and shows them as literal content on the page - see https://amp.dev/documentation/components/amp-selector/#keyboard-select-mode

caroqliu · 2020-11-18T14:40:08Z

extensions/amp-sidebar/0.1/amp-sidebar.md

@@ -216,10 +216,6 @@ Please see [this example file](https://github.com/ampproject/amphtml/blob/master

 `<amp-sidebar>` supports drilldown (nested) menus through a child component named [`<amp-nested-menu>`](../../amp-nested-menu/amp-nested-menu.md). With `<amp-nested-menu>`, `<amp-sidebar>` can support nesting one or more layers of submenus (and transition between them) as demonstrated by the following example:

-[tip type="note"]


FMI: Does this not actually improve accessibility and keyboard support? Is there anything that needs to be done to improve user supplied content for this component?

it's unclear to me what actually happens (as i've not reviewed the logic behind amp-nested-menu). I assumed there was some intended menu-like behaviour being tied to <li> (allowing cursor keys to be used), which is what would not be appropriate. what is the actual outcome of doing what the tip suggests?

bringing up https://playground.amp.dev/?url=https%3A%2F%2Fpreview.amp.dev%2Fdocumentation%2Fcomponents%2Famp-sidebar.example.1.html%3Fformat%3Dwebsites&format=websites and manually removing the <ul> and <li> elements inside that nested menu seems to not have any tangible effect on the outcome (things are still turned into keyboard-focusable role="button" elements regardless.

if there was some intended extra cursor key behaviour, it's not there in the demo/broken?

I'm not too sure, maybe @nainar can weigh in from having participated in design conversations? I assumed it was encouraging use of semantic HTML for a list of items as opposed to non-meaningful tags i.e. <div>, but if this does not improve UX, we don't need to encourage people to do this.

validator/validator.pb.go

validator/validator.proto

patrickhlauke · 2020-11-18T17:40:18Z

thanks @caroqliu ... yeah, the dangers of find/replace (also, on my Windows system at least, it's really tricky to tell visually when the ⚡ has a space after it or not ... should have caught that though). I'll do a second pass of this tonight to address the cases you spotted there

…mentation-phase1

banaag

LGTM as per validator related changes.

caroqliu · 2020-11-24T15:19:03Z

extensions/amp-lightbox-gallery/amp-lightbox-gallery.md

-  <amp-img src="image1" width="200" height="100"></amp-img>
-  <amp-img src="image1" width="200" height="100"></amp-img>
-  <amp-img src="image1" width="200" height="100"></amp-img>
+  <amp-img src="image1" width="200" height="100" alt="Image 1"></amp-img>


Makes sense, thank you

spec/amp-html-format.md

src/auto-lightbox.js

src/standalone.js

extensions/amp-a4a/0.1/test/testdata/valid_css_at_rules_amp.reserialized.js

caroqliu

Thanks so much for your diligence and thoroughness here! Looks like there are a couple merge conflicts - once those are resolved and tests are healthy I'd be happy to sign off.

they were causing merge conflicts, and not sure if it's all that important for these to have `lang="en"`

TetraLogicalHelpdesk · 2020-11-30T21:01:08Z

Thanks so much for your diligence and thoroughness here! Looks like there are a couple merge conflicts - once those are resolved and tests are healthy I'd be happy to sign off.

I've resolved the merge conflicts by resetting the validator/testdata files (as noted above, I'm currently unable to sort out a working build environment for some reason, with obscure WSL errors, so can't actually run the tests, nor generate the .out files correctly). assuming that the presence or not of lang="en" in the test files is not mission-critical for testing the actual functionality/running the test suite. If you feel these should be changed too, let me know and I can try and craft a more specific change, just affecting the .html files (and leave updating the .out files as an exercise that can hopefully be done at your end?)

caroqliu

I've resolved the merge conflicts by resetting the validator/testdata files (as noted above, I'm currently unable to sort out a working build environment for some reason, with obscure WSL errors, so can't actually run the tests, nor generate the .out files correctly). assuming that the presence or not of lang="en" in the test files is not mission-critical for testing the actual functionality/running the test suite. If you feel these should be changed too, let me know and I can try and craft a more specific change, just affecting the .html files (and leave updating the .out files as an exercise that can hopefully be done at your end?)

With regards to the build environment, what kind of errors are you seeing? Do you mind filing a follow up issue, or would you like help debugging more actively via Slack? cc @rsimha as FYI

As for the validator tests, the validator is stringent down to the character, so the addition of lang=en in an input file and not the output file (or vice versa) will cause tests to fail. For now, I'd recommend to not modify .html validator files if their corresponding.out files cannot be updated according to those change in the same PR. cc @banaag for another look in light of these removals/updates.

caroqliu · 2020-11-30T21:06:17Z

extensions/amp-a4a/0.1/test/testdata/valid_css_at_rules_amp.reserialized.js

 </body></html>`,

-  adTemplate: `<!doctype html><html ⚡ lang="en"><head><meta charset="utf-8"><link rel="canonical" href="self.html" /><meta name="viewport" content="width=device-width,minimum-scale=1"><style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript><script async src="https://cdn.ampproject.org/v0.js"></script><script async custom-template="amp-mustache" src="https://cdn.ampproject.org/v0/amp-mustache-0.1.js"></script>
+  adTemplate: `<!doctype html><html ⚡><head><meta charset="utf-8"><link rel="canonical" href="self.html" /><meta name="viewport" content="width=device-width,minimum-scale=1"><style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript><script async src="https://cdn.ampproject.org/v0.js"></script><script async custom-template="amp-mustache" src="https://cdn.ampproject.org/v0/amp-mustache-0.1.js"></script>


Please also reset minifiedTemplateCreative in this same file.

amaltas · 2020-11-30T23:15:45Z

Can you try updating tests (.out files) using the instructions at: https://github.com/ampproject/amphtml/tree/master/validator#usage

TetraLogicalHelpdesk · 2021-02-18T11:04:22Z

Sorry, only just revisiting this (as we're working on another phase of these reviews), and I see that this got stalled at the time and is now conflicting. Is anybody able to help get this updated and merged?

caroqliu · 2021-02-18T17:29:31Z

Sorry, only just revisiting this (as we're working on another phase of these reviews), and I see that this got stalled at the time and is now conflicting. Is anybody able to help get this updated and merged?

@TetraLogicalHelpdesk I spent some time this morning taking a stab at resolving the merge conflicts -- that led to this change which includes all the intermediary commits. I'm not confident that creating a secondary pull request into this working branch is the best way to go about it, but the most important part is the diff in the last two commits - one (8c14d45) which resolves merge conflicts after merging with fresh master, and the other (545bfba) which persists one of the original changes to the file's new location.

Hopefully you can execute the merge with the following recommendations above as reference if needed. Otherwise please please find me (Caroline Liu) on the AMP Slack and we can hopefully resolve this in live time together.

Finally, one recommendation going forward could be to break this PR up so the majority of changes aren't stalled by recent changes in the codebase to a few others. The most obvious is to separate the lang="en" additions from the handful of documentation changes, but another could also be to fragment the set of lang="en" additions as well by a few directories in the repo at a time.

patrickhlauke · 2021-02-20T10:48:03Z

@patrickkettner not sure if this falls into your area, but wondering if you'd be able to help push this over the line? for context, this was done as part of an audit/review in november, and i've since been on other projects (so had no time allocated to nurse this PR further). i'm also far from being a wizard with git, as evidenced by my "let's just make it one giant PR" approach here.

Fix merge conflicts

patrickhlauke · 2021-02-23T09:15:34Z

After wrapping my head around this, I managed to work out what the actual steps required were. thank you @caroqliu for doing the hard work with to resolve the conflicts ... once I worked out that all that was needed was for me to merge that work back into our branch and then push it back up, it all went smoothly. Sorry for sounding the "not a git expert" alarm bell, @patrickkettner. I think this should now be mergeable.

patrickkettner · 2021-02-23T14:36:51Z

sorry I missed the ping, @patrickhlauke 🤦‍♂️ - great job!

kristoferbaxter

That's a whole lot of lang attributes. Thanks for the fixes!

patrickhlauke · 2021-02-23T15:05:41Z

That's a whole lot of lang attributes

yes, sorry...the joy/pain of doing a fairly global find and replace :)

caroqliu · 2021-06-07T18:44:05Z

Looks like this PR has fallen behind a few merge conflicts. Anything I can do to help get this through? @TetraLogicalHelpdesk @patrickhlauke

TetraLogicalHelpdesk · 2021-06-07T19:55:11Z

Looks like this PR has fallen behind a few merge conflicts. Anything I can do to help get this through? @TetraLogicalHelpdesk @patrickhlauke

Hi @caroqliu, it's probably best at this stage (since the project relating to this was closed quite some time ago) if somebody on your side could shepherd this PR through to completion. Alternatively, we can add some extra time to the next AMP-related project to take care of this? Let us know how you'd like to proceed.

Gregable · 2022-01-10T20:48:41Z

@kristoferbaxter for triage, unclear what should be done with this old PR

stale · 2023-09-17T01:36:43Z

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

patrickhlauke added 9 commits November 17, 2020 12:49

Add lang to all <html> elements

5cb4a01

Closes #31172

Add a few low-hanging alt attributes to images

88371fa

x-ref #31154 #31177 #31178

Fix incorrect display

52285c8

The HTML markup list seems to suppress rendering of the backtick code markup - see https://amp.dev/documentation/components/amp-selector/#keyboard-select-mode

Missing backticks

e902dfd

Typo: elligible > eligible

78c7566

Stray <

721060f

Typo in amp-base-carousel example code

b3ff063

Missing code backticks

63c144c

TetraLogicalHelpdesk changed the title ~~♿ TetraLogical: P0 components accessibility review~~ ♿ TetraLogical: P1 components accessibility review Nov 17, 2020

amp-owners-bot bot requested a review from dvoytenko November 17, 2020 14:58

TetraLogicalHelpdesk added the Related to: Accessibility label Nov 17, 2020

TetraLogicalHelpdesk mentioned this pull request Nov 17, 2020

♿ Add missing lang attributes to examples ampproject/amp.dev#4972

Merged

honeybadgerdontcare requested a review from twifkak November 17, 2020 17:09

honeybadgerdontcare added the WG: caching label Nov 17, 2020

caroqliu requested changes Nov 18, 2020

View reviewed changes

patrickhlauke added 3 commits November 18, 2020 19:15

Merge remote-tracking branch 'upstream/master' into tetralogical-docu…

c1db74f

…mentation-phase1

Tweaks after first review

f3d0cb3

Add lang="en" to some missed-out examples

3d8be1f

amp-bundle-size bot requested a review from jridgewell November 18, 2020 20:28

banaag self-requested a review November 23, 2020 23:05

banaag approved these changes Nov 23, 2020

View reviewed changes

caroqliu requested changes Nov 24, 2020

View reviewed changes

Changes following review

103f762

caroqliu reviewed Nov 30, 2020

View reviewed changes

Reset all validator testdata files

1eda3ae

they were causing merge conflicts, and not sure if it's all that important for these to have `lang="en"`

caroqliu requested changes Nov 30, 2020

View reviewed changes

caroqliu added 2 commits February 18, 2021 10:19

Merge branch 'master' into fix-merge-conflicts

8c14d45

Copy original amp-accordion changes to its new file location

545bfba

patrickhlauke and others added 2 commits February 23, 2021 09:05

Merge pull request #1 from caroqliu/fix-merge-conflicts

7b9f9f3

Fix merge conflicts

Merge branch 'master' into tetralogical-documentation-phase1

3d16fea

kristoferbaxter approved these changes Feb 23, 2021

View reviewed changes

caroqliu approved these changes Feb 23, 2021

View reviewed changes

antiphoton added the WG: Caching: Triaged Initial triage from wg-caching complete. Remove label if new input required. label Apr 12, 2021

twifkak removed their request for review April 22, 2021 19:29

jridgewell removed their request for review August 31, 2022 21:02

stale bot added the Stale Inactive for one year or more label Sep 17, 2023

stale bot closed this Oct 15, 2023

		@@ -216,10 +216,6 @@ Please see [this example file](https://github.com/ampproject/amphtml/blob/master

		`<amp-sidebar>` supports drilldown (nested) menus through a child component named [`<amp-nested-menu>`](../../amp-nested-menu/amp-nested-menu.md). With `<amp-nested-menu>`, `<amp-sidebar>` can support nesting one or more layers of submenus (and transition between them) as demonstrated by the following example:

		[tip type="note"]

Conversation

TetraLogicalHelpdesk commented Nov 17, 2020

Uh oh!

CLAassistant commented Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amp-owners-bot bot commented Nov 17, 2020 • edited by ampprojectbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TetraLogicalHelpdesk commented Nov 17, 2020

Uh oh!

nainar commented Nov 17, 2020

Uh oh!

caroqliu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caroqliu Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

patrickhlauke commented Nov 18, 2020

Uh oh!

banaag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

caroqliu left a comment

Choose a reason for hiding this comment

Uh oh!

TetraLogicalHelpdesk commented Nov 30, 2020

Uh oh!

caroqliu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amaltas commented Nov 30, 2020

Uh oh!

TetraLogicalHelpdesk commented Feb 18, 2021

Uh oh!

caroqliu commented Feb 18, 2021

Uh oh!

patrickhlauke commented Feb 20, 2021

Uh oh!

patrickhlauke commented Feb 23, 2021

Uh oh!

patrickkettner commented Feb 23, 2021

Uh oh!

kristoferbaxter left a comment

CLAassistant commented Nov 17, 2020 •

edited

Loading

amp-owners-bot bot commented Nov 17, 2020 •

edited by ampprojectbot

Loading

caroqliu Nov 18, 2020 •

edited

Loading