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.

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

Added Title attribute to the iframe for Archive.org book embed #13451

Conversation

rickcurran
Copy link
Contributor

@rickcurran rickcurran commented Sep 9, 2019

This commit adds a Title attribute to the iframe for Archive.org book embed to improve the Section 508 and WCAG 2.0 accessibility compliance.

The change here tackles the open issue #7370 "Shortcode Embeds: should include title attribute on iframe output to meet accessibility standards". There are multiple files which have iframes without a title attribute, but this changes tackles the first one I found in order to establish a correct format / methodology before updating the rest.
This change adds a fixed text string which would be a translatable string within Jetpack's i8ln language strings. However, as this embed is generated from a shortcode it would also be an option to enable this string to be specified by the user as an additional attribute to the shortcode.

Fixes #

Changes proposed in this Pull Request:

A title attribute with a fixed i8ln string is added to the iframe that is rendered within this piece of code.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

This adds to an existing feature of Jetpack.

Testing instructions:

To test this please try adding an Archive.org book using the archiveorg-book shortcode. E.g.

[archiveorg-book id=inferno00dant_2 width=560 height=384]

With the code in place the rendered iframe should now have a title attribute.

Proposed changelog entry for your changes:

This commit adds a Title attribute to the iframe for Archive.org book embed to improve the Section 508 and WCAG 2.0 accessibility compliance.

This commit adds a `Title` attribute to the iframe for Archive.org book embed to improve the Section 508 and WCAG 2.0 accessibility compliance.
@jetpackbot
Copy link

jetpackbot commented Sep 9, 2019

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: October 1, 2019.
Scheduled code freeze: September 24, 2019

Generated by 🚫 dangerJS against 8dc77e2

@jeherve jeherve added [Pri] Low [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Focus] Accessibility Improving usability for all users (a11y) [Feature] Shortcodes / Embeds labels Sep 12, 2019
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

LGTM!

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Sep 12, 2019
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello rickcurran! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D32728-code before merging this PR. Thank you!

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Unit Tests are failing right now though:

WP_Test_Jetpack_Shortcodes_ArchiveOrg::test_shortcode_book
Failed asserting that '<div class='embed-archiveorg-book' style='text-align:center;'><iframe title='Archive.org Book' src='https://archive.org/stream/goodytwoshoes00newyiala?ui=embed#mode/1up' width='600' height='300' style='border:0;' webkitallowfullscreen='true' mozallowfullscreen='true' allowfullscreen></iframe></div>' contains "iframe src='https://archive.org/stream/goodytwoshoes00newyiala?ui=embed#mode/1up' width='600' height='300'".
/tmp/wordpress-latest/src/wp-content/plugins/jetpack/tests/php/modules/shortcodes/test-class.archiveorg.php:53

You'll need to update the test to match the new structure.

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Sep 12, 2019
@rickcurran
Copy link
Contributor Author

@jeherve Thanks! Can you give me some guidance about updating Unit Tests for this, it's not something I've done before so not sure what action to take here.

@jeherve
Copy link
Member

jeherve commented Sep 13, 2019

In tests/php/modules/shortcodes/test-class.archiveorg.php you'll find a function named test_shortcode_book. That function checks that when you add a specific shortcode to post content, that post includes a specific iFrame as a result. You'll want to update the expected markup in there since the iFrame that is now displayed now includes a title attribute as well.

To run the tests locally after making the changes, and thus checking that the tests now pass, you can run yarn docker:phpunit --filter WP_Test_Jetpack_Shortcodes_ArchiveOrg.

…tribute

Updated the Unit test for the Archive-book shortcode to include the new `title` attribute on the iframe in the test code.
@rickcurran
Copy link
Contributor Author

@jeherve Thanks, I've updated the unit test file to include the title attribute and committed that to the PR.

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

I needed to rebase that branch, but it should be okay now. This should be good to merge!

@jeherve jeherve added this to the 7.8 milestone Sep 17, 2019
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Sep 17, 2019
@rickcurran
Copy link
Contributor Author

Ok, great. If this is all ok then I can take a look at all the other iframes that need title attributes added to them, should I just do each of them as separate PRs?

@matticbot
Copy link
Contributor

rickcurran, Your synced wpcom patch D32728-code has been updated.

@jeherve jeherve merged commit 2e12984 into Automattic:master Sep 19, 2019
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Sep 19, 2019
@jeherve
Copy link
Member

jeherve commented Sep 19, 2019

If this is all ok then I can take a look at all the other iframes that need title attributes added to them, should I just do each of them as separate PRs?

Let's go at them one file at a time, if that's alright with you. It's easier to test and review that way, it ensures we won't miss anything by mistake.

jeherve added a commit that referenced this pull request Sep 20, 2019
jeherve added a commit that referenced this pull request Sep 24, 2019
jeherve added a commit that referenced this pull request Sep 24, 2019
* Changelog: initial set of changes for 7.8

* Changelog: add #13310

* Changelog: add #13103

* Changelog: add #13426

* Changelog: add #13389

* Changelog: add #13449

* Changelog: add #13461

* Changelog: add #13460

* Changelog: add #13441

* Changelog: add #13454

* Changelog: add #13457

* Changelog: add #13425

* Changelog: add #13473

* Changelog: add #13355

* Changelog: add #13451

* Changelog: add #13358

* Changelog: add #13464

* Changelog: add #13416

* Changelog: add #13494

* Changelog: add #13465

* Changelog: add #13424

* Changelog: add #13432

* Changelog: add #13471

* Changelog: add 7.7.2 entry

* Changelog: add #13446

* Add more testing elements
jeherve added a commit that referenced this pull request Sep 24, 2019
* Changelog: initial set of changes for 7.8

* Changelog: add #13310

* Changelog: add #13103

* Changelog: add #13426

* Changelog: add #13389

* Changelog: add #13449

* Changelog: add #13461

* Changelog: add #13460

* Changelog: add #13441

* Changelog: add #13454

* Changelog: add #13457

* Changelog: add #13425

* Changelog: add #13473

* Changelog: add #13355

* Changelog: add #13451

* Changelog: add #13358

* Changelog: add #13464

* Changelog: add #13416

* Changelog: add #13494

* Changelog: add #13465

* Changelog: add #13424

* Changelog: add #13432

* Changelog: add #13471

* Changelog: add 7.7.2 entry

* Changelog: add #13446

* Add more testing elements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Shortcodes / Embeds [Focus] Accessibility Improving usability for all users (a11y) [Pri] Low Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants