Core Css class for visually invisible but screen-reader accessible elements #5542

aghassemi · 2016-10-12T06:13:38Z

This PR introduced a new core Css class amp-screen-reader that can be used by us or page authors to visually hide elements but keep them accessible to screen-readers.

We use this new class in sidebar to fix #5517.

Also as part of this PR, we are now installing core styles in the testing iframe so that we can use computedStyles to ensure proper Css is being applied.

/cc @camelburrito @dvoytenko

camelburrito · 2016-10-12T06:44:07Z

css/amp.css

+ * changing width from 1 to 0 would prevent TalkBack from activating (clicking)
+ * buttons despite TalkBack reading them just fine.
+ */
+.amp-screen-reader {


.-amp-screen-reader

We dont want to make this overridable

we will be doing most of the a11y for things with interactions

Although I don't want it to be overridable, I do want page authors to use it in their own markup to fix a11y issues in their page. For now renamed to -amp but this is something we should expose as part of the core kick-start Css.

camelburrito · 2016-10-12T06:47:53Z

css/amp.css

+ * buttons despite TalkBack reading them just fine.
+ */
+.amp-screen-reader {
+  position: fixed !important;


not sure if more of these elements would make fixed layer operations slow. May be we should just whitelist from fixed layer somehow.

CC @dvoytenko

fixed elements in our runtime css are not added to fixed layer by default. This should never make it to fixed layer, not because of performance but because fixed layer breaks DOM hierarchy which breaks accessibility.

camelburrito · 2016-10-12T07:00:01Z

css/amp.css

+  border: none !important;
+  margin: 0 !important;
+  padding: 0 !important;
+  display: block !important;


does this matter? once you set positioning-absolute/static i think you dont have to set this

I am setting display:block !important to ensure when this class is used in combination with other classes/styles on an element, the display is never set to none (which would defeat the purpose of this class all together). Will add some comments to the code to clarify.

camelburrito · 2016-10-12T07:03:48Z

css/amp.css

+ * changing width from 1 to 0 would prevent TalkBack from activating (clicking)
+ * buttons despite TalkBack reading them just fine.
+ */
+.amp-screen-reader {


Rather than doing this in the css i would create a method in dom.js that would add this div and this would make it explicit when being called from inside the extension's build callback.

@dvoytenko what do you think?

this is not always a div ( actually most cases it would be a button or a p) and it is not always a new element that is created in a component, sometimes we want to toggle this class on an existing node (for example to fix #5487 we could just add this to the next/previous buttons when hiding them).

We can add dom.js utilities but we should keep this in Css (another reason to keep it in Css is that we want to expose this to page authors as part of the kick-start core Css)

aghassemi · 2016-10-12T16:11:03Z

@jridgewell Please review the test-amp-fx-flying-carpet.js changes. They broke because we now install the runtime Css when creating the test iframe and therefore iframe.doc.body.style.height = '400vh' was not being applied because of !important! rules coming from runtime Css.

jridgewell · 2016-10-12T18:11:12Z

extensions/amp-fx-flying-carpet/0.1/test/test-amp-fx-flying-carpet.js

@@ -44,7 +44,11 @@ describe('amp-fx-flying-carpet', () => {
      iframe = i;
      toggleExperiment(iframe.win, 'amp-fx-flying-carpet', true);

-      iframe.doc.body.style.height = '400vh';
+      const bodyResizer = iframe.doc.createElement('div');


different way of sizing the body's height. We now install the runtime Css when creating the test iframe and therefore iframe.doc.body.style.height = '400vh' was not being applied because of !important! rules coming from runtime Css was overriding it.

jridgewell · 2016-10-12T18:11:55Z

extensions/amp-sidebar/0.1/amp-sidebar.js

@@ -99,7 +99,19 @@ export class AmpSidebar extends AMP.BaseElement {
        this.close_();
      }
    });
-    //TODO (skrish, #2712) Add history support on back button.


Don't delete this todo.

it's done though...

jridgewell · 2016-10-12T18:12:59Z

extensions/amp-sidebar/0.1/amp-sidebar.js

+    screenReaderCloseButton.addEventListener('click', () => {
+      this.close_();
+    });
+    this.element.appendChild(screenReaderCloseButton);


Is this element going to be removed eventually?

nope, will always be there.

jridgewell · 2016-10-12T18:13:33Z

extensions/amp-sidebar/0.1/amp-sidebar.js

+    screenReaderCloseButton.textContent = 'Close the sidebar';
+    screenReaderCloseButton.classList.add('-amp-screen-reader');
+    // This is for screen-readers only, should not get a tab stop.
+    screenReaderCloseButton.tabIndex = -1;


Would it be better to create a screenReaderElement function that does this stuff for you?

generic one doesn't make sense cause these elements could be buttons, paragraphs, links, etc.. depending on the use-case.

Yah, but we can pass the element to make. Essentially, if we always want .-amp-screen-reader and tabIndex = -1, it'd be helpful because we're gonna forget the tabIndex.

function createScreenReaderElement(tag) { const elem = doc.create(tag); // ... psuedocode... return elem; }

Oh, I thought this was being added on open.

How about

/** * Visually hides an element but keeps it accessible to screen readers. * If opt_visibleOnlyForScreenReaders is false or element was already only * visible to screen-readers, it removes the screen-reader specific styling * from the element. * @param {boolean=} opt_visibleOnlyForScreenReaders */ export function toggleScreenReaderVisibility(element, opt_visibleOnlyForScreenReaders) { if (opt_visibleOnlyForScreenReaders === undefined) { opt_visibleOnlyForScreenReaders = !(element.classList.contains('-amp-screen-reader')); } element.classList.toggle('-amp-screen-reader'); //TODO have to check whether default is '' or null or undefined element.tabIndex = opt_visibleOnlyForScreenReaders ? -1 : ''; }

in style.js

We would need to hide hide these when they aren't visible. But I like the idea.

no, I don't want this to be used to hide hide things. There are cases where you want to apply ''-amp-screen-reader" to an existing visible node and then remove ''-amp-screen-reader" later to make the node visible again for everyone. Let's do the refactoring later when we have more uses of this, I think refactoring this right now might be premature.

aghassemi · 2016-10-13T16:06:55Z

@mkhatib @camelburrito @jridgewell PTAL, I like to merge this to unblock #5487 and #5503. Thanks!

jridgewell · 2016-10-13T18:24:47Z

css/amp.css

@@ -121,11 +121,44 @@ i-amp-sizer {
 }

 .-amp-replaced-content {
+


…ible elements (ampproject#5542)

aghassemi added 2 commits October 11, 2016 17:02

Off-screen css class for screen-readers

71bc1c9

Some changes after testing

2176f26

aghassemi added the NEEDS REVIEW label Oct 12, 2016

aghassemi assigned mkhatib Oct 12, 2016

camelburrito reviewed Oct 12, 2016

View reviewed changes

Review comments and fix test

6d6602c

Increase size to 2 pixels or VoiceOver won't activate buttons

a2ee1df

jridgewell reviewed Oct 12, 2016

View reviewed changes

This was referenced Oct 12, 2016

[A11Y] Slides carousel: Slides not navigable by screen readers without controls attribute present #5487

Closed

[A11Y] amp-image-lightbox: Needs an accessible (yet invisible) close button. #5503

Closed

jridgewell reviewed Oct 13, 2016

View reviewed changes

css/amp.css

@@ -121,11 +121,44 @@ i-amp-sizer {

}

.-amp-replaced-content {

Copy link

Contributor

jridgewell Oct 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔪

jridgewell approved these changes Oct 13, 2016

View reviewed changes

remove whitespace

b31a8d6

aghassemi merged commit 91eef43 into ampproject:master Oct 13, 2016

aghassemi mentioned this pull request Oct 14, 2016

Install core Css in the test iframe #5612

Closed

Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016

[A11Y] Core Css class for visually invisible but screen-reader access…

d419a28

…ible elements (ampproject#5542)

Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016

[A11Y] Core Css class for visually invisible but screen-reader access…

c935332

…ible elements (ampproject#5542)

jridgewell mentioned this pull request Apr 20, 2017

Fix a11y of amp-carousel. Add a11y helper class. #8822

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core Css class for visually invisible but screen-reader accessible elements #5542

Core Css class for visually invisible but screen-reader accessible elements #5542

aghassemi commented Oct 12, 2016

camelburrito Oct 12, 2016

camelburrito Oct 12, 2016

aghassemi Oct 12, 2016

camelburrito Oct 12, 2016

aghassemi Oct 12, 2016

camelburrito Oct 12, 2016

aghassemi Oct 12, 2016 •

edited

camelburrito Oct 12, 2016

aghassemi Oct 12, 2016

aghassemi commented Oct 12, 2016

jridgewell Oct 12, 2016

aghassemi Oct 12, 2016

jridgewell Oct 12, 2016

jridgewell Oct 12, 2016

aghassemi Oct 12, 2016

jridgewell Oct 12, 2016

jridgewell Oct 12, 2016

aghassemi Oct 12, 2016

jridgewell Oct 12, 2016

aghassemi Oct 12, 2016

jridgewell Oct 12, 2016 •

edited

jridgewell Oct 12, 2016

aghassemi Oct 12, 2016 •

edited

jridgewell Oct 12, 2016

aghassemi Oct 12, 2016

aghassemi commented Oct 13, 2016

jridgewell Oct 13, 2016

		@@ -121,11 +121,44 @@ i-amp-sizer {
		}

		.-amp-replaced-content {

Core Css class for visually invisible but screen-reader accessible elements #5542

Core Css class for visually invisible but screen-reader accessible elements #5542

Conversation

aghassemi commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aghassemi Oct 12, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aghassemi commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jridgewell Oct 12, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aghassemi Oct 12, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aghassemi commented Oct 13, 2016

Choose a reason for hiding this comment

aghassemi Oct 12, 2016 •

edited

jridgewell Oct 12, 2016 •

edited

aghassemi Oct 12, 2016 •

edited