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

-Add alt attribute for instagram img container #3520

Merged
merged 7 commits into from Jun 10, 2016

Conversation

Projects
None yet
5 participants
@patHyatt
Copy link
Contributor

patHyatt commented Jun 9, 2016

This PR addresses a couple of accessibility issues that exist with the amp-instagram component.

Namely:

  1. rendered tag that houses the Instagram image does not have option for an alt attribute to make image screen reader friendly.
  2. iframe pulled in from Instagram does not have a title attribute which helps screen reader users differentiate frames.

Issue #3503 details the amp-instagram component use in markup and rendered output.

Commit log
-Add title attribute for instagram iframe
-Update instagram tests to include alt and title attribute checks

-Add alt attribute for instagram img container
-Add title attribute for instagram iframe
-Update instagram tests to include alt and title attribute checks
@@ -87,6 +88,10 @@ class AmpInstagram extends AMP.BaseElement {
image.setAttribute('src', 'https://www.instagram.com/p/' +
encodeURIComponent(this.shortcode_) + '/media/?size=l');
image.setAttribute('layout', 'fill');

//Push alt to underlying amp-img component

This comment has been minimized.

@mkhatib

mkhatib Jun 9, 2016

Contributor

No need for comment.

@@ -110,6 +115,8 @@ class AmpInstagram extends AMP.BaseElement {
this.iframe_ = iframe;
iframe.setAttribute('frameborder', '0');
iframe.setAttribute('allowtransparency', 'true');
//title frame for a11y

This comment has been minimized.

@mkhatib

mkhatib Jun 9, 2016

Contributor

Nit: // Add title to the iframe for better accessibility. Or drop comment.

@@ -26,6 +26,7 @@
<h1>amp #0</h1>
<amp-instagram
data-shortcode="fBwFP"
alt="Guy computing on a couch."

This comment has been minimized.

@mkhatib

mkhatib Jun 9, 2016

Contributor

Nit: Malte Ubl computing on a couch. 😄

This comment has been minimized.

@erwinmombay

This comment has been minimized.

@erwinmombay

erwinmombay Jun 9, 2016

Member

better yet, use cramforce 🎆

@mkhatib mkhatib added the NEEDS REVIEW label Jun 9, 2016

@mkhatib mkhatib self-assigned this Jun 9, 2016

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Jun 10, 2016

Great. This will also need a validator change.

@mkhatib

This comment has been minimized.

Copy link
Contributor

mkhatib commented Jun 10, 2016

@cramforce this doesn't need to wait for validator change right and can be merged?

@cramforce

This comment has been minimized.

Copy link
Member

cramforce commented Jun 10, 2016

It can. But somebody needs to make the change!

@patHyatt

This comment has been minimized.

Copy link
Contributor

patHyatt commented Jun 10, 2016

I can add this, to the issue @mkhatib created #3533 later tonight/tomorrow.

@mkhatib

This comment has been minimized.

Copy link
Contributor

mkhatib commented Jun 10, 2016

@patHyatt mind actually adding the validator change to this same PR?

@mkhatib mkhatib added NEEDS REVIEW and removed LGTM labels Jun 10, 2016

@Gregable

This comment has been minimized.

Copy link
Member

Gregable commented Jun 10, 2016

Alternatively, I can also send a PR for this so that @patHyatt doesn't need to figure out our scheme for validation.

@patHyatt

This comment has been minimized.

Copy link
Contributor

patHyatt commented Jun 10, 2016

@Gregable Go for it, I won't be able to get back in to tihs for another 6+ hours.

@mkhatib

This comment has been minimized.

Copy link
Contributor

mkhatib commented Jun 10, 2016

Awesome thanks @Gregable 👍

@mkhatib mkhatib merged commit 7b2c9b9 into ampproject:master Jun 10, 2016

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment