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.

Sign up for GitHub

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

Jump to bottom

Gallery block: use image caption as fallback for alt text. #26082

Merged
merged 1 commit into from
Oct 18, 2020
Merged

Gallery block: use image caption as fallback for alt text. #26082

merged 1 commit into from
Oct 18, 2020

Conversation

ZebulanStanphill
Copy link
Member

@ZebulanStanphill ZebulanStanphill commented Oct 14, 2020 •
edited
Loading

Description

Addresses feedback on #25560. The image alt text will now fall back to the caption text, and aria-label is no longer used.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@ZebulanStanphill ZebulanStanphill added [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Block library /packages/block-library [Block] Gallery Affects the Gallery Block - used to display groups of images [a11y] Labelling labels Oct 14, 2020
@ZebulanStanphill ZebulanStanphill added this to Needs Review in WordPress 5.6.x Must Haves via automation Oct 14, 2020
@ZebulanStanphill ZebulanStanphill mentioned this pull request Oct 14, 2020
Merged
6 tasks
@github-actions
Copy link

github-actions bot commented Oct 14, 2020 •
edited
Loading

Size Change: -20 B (0%)

Total Size: 1.19 MB

Filename Size Change
build/block-library/index.js 144 kB -20 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.54 kB 0 B
build/api-fetch/index.js 3.35 kB 0 B
build/autop/index.js 2.72 kB 0 B
build/blob/index.js 668 B 0 B
build/block-directory/index.js 8.6 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 130 kB 0 B
build/block-editor/style-rtl.css 11 kB 0 B
build/block-editor/style.css 10.9 kB 0 B
build/block-library/editor-rtl.css 8.93 kB 0 B
build/block-library/editor.css 8.93 kB 0 B
build/block-library/style-rtl.css 7.71 kB 0 B
build/block-library/style.css 7.71 kB 0 B
build/block-library/theme-rtl.css 741 B 0 B
build/block-library/theme.css 741 B 0 B
build/block-serialization-default-parser/index.js 1.77 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 47.6 kB 0 B
build/components/index.js 170 kB 0 B
build/components/style-rtl.css 15.4 kB 0 B
build/components/style.css 15.4 kB 0 B
build/compose/index.js 9.63 kB 0 B
build/core-data/index.js 12.1 kB 0 B
build/data-controls/index.js 683 B 0 B
build/data/index.js 8.63 kB 0 B
build/date/index.js 31.9 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 4.43 kB 0 B
build/edit-navigation/index.js 10.6 kB 0 B
build/edit-navigation/style-rtl.css 868 B 0 B
build/edit-navigation/style.css 871 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.37 kB 0 B
build/edit-post/style.css 6.35 kB 0 B
build/edit-site/index.js 21.6 kB 0 B
build/edit-site/style-rtl.css 3.8 kB 0 B
build/edit-site/style.css 3.81 kB 0 B
build/edit-widgets/index.js 21.6 kB 0 B
build/edit-widgets/style-rtl.css 3.09 kB 0 B
build/edit-widgets/style.css 3.09 kB 0 B
build/editor/editor-styles-rtl.css 480 B 0 B
build/editor/editor-styles.css 482 B 0 B
build/editor/index.js 42.6 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.84 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.49 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 1.74 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.54 kB 0 B
build/is-shallow-equal/index.js 709 B 0 B
build/keyboard-shortcuts/index.js 2.38 kB 0 B
build/keycodes/index.js 1.85 kB 0 B
build/list-reusable-blocks/index.js 3.02 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.12 kB 0 B
build/notices/index.js 1.69 kB 0 B
build/nux/index.js 3.27 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.44 kB 0 B
build/primitives/index.js 1.35 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/reusable-blocks/index.js 3.04 kB 0 B
build/rich-text/index.js 13 kB 0 B
build/server-side-render/index.js 2.61 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.24 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.75 kB 0 B
build/warning/index.js 1.13 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@ZebulanStanphill ZebulanStanphill force-pushed the update/gallery-alt-text-fallback branch 2 times, most recently from 7839b97 to 2648dab Compare October 14, 2020 19:12
@ZebulanStanphill
Copy link
Member Author

Finally got around to actually testing this PR, and I discovered it didn't work. Turns out image.alt is an empty string by default. After a quick change to the logic, I got it working. It's ready for review now.

enriquesanchez
enriquesanchez approved these changes Oct 14, 2020
View reviewed changes
Copy link
Contributor

@enriquesanchez enriquesanchez left a comment

Choose a reason for hiding this comment

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

Works as expected. The image caption is set as the alt text where there is none supplied. Thank you for working on this, Zeb! 👍

@tellthemachines
Copy link
Contributor

Hmm, I wonder if we still need the deprecation added in #25560, given we're no longer changing the output markup.

@ZebulanStanphill
Copy link
Member Author

@tellthemachines We're still changing the markup, though, aren't we? Prior to both this PR and #25560, the alt attribute would be empty even if there was a caption. There's still a net change compared to master.

@ZebulanStanphill ZebulanStanphill merged commit 9ed837b into master Oct 18, 2020
WordPress 5.6.x Must Haves automation moved this from Needs Review to Done Oct 18, 2020
@ZebulanStanphill ZebulanStanphill deleted the update/gallery-alt-text-fallback branch October 18, 2020 19:09
@github-actions github-actions bot added this to the Gutenberg 9.2 milestone Oct 18, 2020
@ockham ockham mentioned this pull request Oct 28, 2020
Closed
@ockham
Copy link
Contributor

ockham commented Oct 28, 2020

We're seeing some funky errors on the frontend with the gallery blocks because of captions that include links (<a /> tags) (which are then mangled by another, custom, layer of sanitization on our site that apparently gets the opening and closing HTML tags wrong): Automattic/wp-calypso#46813

Should we strip the caption of any HTML prior to using it as alt text? Seems like an alt attribute shouldn't contain any HTML anyway, no?

@ockham ockham mentioned this pull request Oct 28, 2020
Closed
@tellthemachines
Copy link
Contributor

Thanks for reporting this @ockham! Indeed the alt attribute shouldn't contain HTML.

This was referenced Nov 3, 2020
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enriquesanchez enriquesanchez enriquesanchez approved these changes

@mkevins mkevins Awaiting requested review from mkevins

@afercia afercia Awaiting requested review from afercia

Assignees
No one assigned
Labels
[Block] Gallery Affects the Gallery Block - used to display groups of images [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Block library /packages/block-library [Type] Enhancement A suggestion for improvement.
Projects
No open projects
WordPress 5.6.x Must Haves
Done
Milestone
Gutenberg 9.2
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ZebulanStanphill @tellthemachines @ockham @enriquesanchez @priethor