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

Add title attribute to the iframe for the regular archive.org embed #13494

Conversation

rickcurran
Copy link
Contributor

This commit adds a Title attribute to the iframe for the regular 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 which are to be updated via separate PRs).

This change adds a fixed text string which would be a translatable string within Jetpack's i8ln language strings.

It also updates the test file test-class.archiveorg.php to include the new title attribute.

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 id=Wonderfu1958 width=320 height=240]

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 the regular Archive.org embed to improve the Section 508 and WCAG 2.0 accessibility compliance.

This adds a Title attribute to the iframe output generated for the regular Archive.org shortcode.
This adds an update to the test file for regular Archive.org shortcode to include the new `title` attribute.
@rickcurran rickcurran requested a review from a team September 19, 2019 13:39
@jeherve jeherve added [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 [Feature] Shortcodes / Embeds labels Sep 19, 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.

You seem to have started that branch from an outdated version of master, so this needs a rebase. Could you rebase after pulling all latest changes into your master branch, as explained here:
https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request

@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] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Sep 19, 2019
@rickcurran
Copy link
Contributor Author

Weird, I thought I'd pulled in all the latest changes before doing it. I'll go sort that out.

@rickcurran
Copy link
Contributor Author

rickcurran commented Sep 19, 2019

@jeherve I'm trying to rebase this using the Github desktop app, but it seems to think that it's up to date with master and so it won't let me start a rebase? I'd thought I'd updated from master before making my new branch so this would tie in with that behaviour – I've likely done something wrong though but not sure what! Any suggestions? (Sorry, I haven't rebased anything before so this is all new to me).

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 rebased the file for you. This should now be good to merge.

@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 19, 2019
@jeherve jeherve added this to the 7.8 milestone Sep 19, 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 D32921-code before merging this PR. Thank you!

@rickcurran
Copy link
Contributor Author

Thanks @jeherve, not sure what happened there! I'll double-check when I do the next one that I've pulled all updates from Master before making a branch for it.

@jetpackbot
Copy link

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 40faba9

@jeherve jeherve merged commit aaf47c8 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 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 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