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

Fix: Gallery Block: Append images alt in aria-label attribute. #17102

Conversation

donmhico
Copy link
Contributor

@donmhico donmhico commented Aug 20, 2019

Description

Fixes #16886.

Appends the images alt attributes (if any) in the aria-label.

How has this been tested?

Followed the steps provided in #16886.

Screenshots

Screen Shot 2019-08-20 at 1 51 05 PM

Types of changes

Fixes #16886

Checklist:

  • My code is tested.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Hi @donmhico thank you for your contribution this change generally looks good 👍 I just found a bug that we need to address before merging.
Cc: @afercia in case there is some feedback for the aria-label text used in this PR.

@@ -302,8 +302,10 @@ class GalleryEdit extends Component {
) }
>
{ images.map( ( img, index ) => {
const imgAlt = ( img.alt.length > 0 ) ? `${ img.alt }. ` : '';
Copy link
Member

Choose a reason for hiding this comment

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

We need to make img.alt is defined e.g: img.alt && img.alt.length > 0. Currently, if I upload an image to a gallery using drag & drop the editor crashes because the image does not contain an alt text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @jorgefilipecosta. I've amended the commit to include this change.

{ images.map( ( img, index ) => {
const imgAlt = ( img.alt && img.alt.length > 0 ) ? `${ img.alt }. ` : '';

Sorry if I wasn't able to test the drag and drop. I'll be more careful the next time.

@donmhico donmhico force-pushed the fix/16886-gallery-block-show-alt-in-aria-label branch from 894b59c to 14b57c7 Compare August 20, 2019 22:35
@afercia
Copy link
Contributor

afercia commented Aug 26, 2019

@donmhico @jorgefilipecosta thanks for working on this!.

in case there is some feedback for the aria-label text used in this PR

I'd say that if we want to keep the n of n information, it's fine to use the aria-label. However, it'd be great to adjust a couple things.

When the alt text is empty:
For consistency, I'd recommend to use the same fallback mechanism used for the image, see

const filename = this.getFilename( url );
let defaultedAlt;
if ( alt ) {
defaultedAlt = alt;
} else if ( filename ) {
defaultedAlt = sprintf( __( 'This image has an empty alt attribute; its file name is %s' ), filename );
} else {
defaultedAlt = __( 'This image has an empty alt attribute' );
}

It works thanks to a small utility function getFilename() (which in turn uses getPath imported from @wordpress/url). This mechanism allows screen reader users to have some context on the image even when the alt attribute is empty. Maybe worth considering to abstract getFilename()? @jorgefilipecosta I'd defer to you about this.

Simplify the string
Screen readers already announce the element type. There's no need to repeat "image". I know this is pre-existing but it's a good opportunity to remove the word "image", otherwise it will get repeated:

Screenshot 2019-08-26 at 19 13 48

I'd also remove "in gallery", as users already know they're editing a gallery. So the string could be something like:

%1$s%2$d of %3$d

This would be announced by screen readers as:

Unicorns! 2 of 3

or (with the fallbacks):

This image has an empty alt attribute; its file name is unicorn-wallpaper-1024x768.jpg 2 of 3

or (last fallback):

This image has an empty alt attribute 2 of 3

Note: the translators comment should be updated, as there are now 3 placeholders.

jorgefilipecosta and others added 22 commits August 27, 2019 10:59
… `withFocusOutside` HOC (WordPress#17051)

* Adds check to determine whether event occured within element trapping focus

Previously there was no check to see whether the blur event occured within the wrapped component. If it occurs within then we do not want to trigger the `handleFocusOutside` callback because (by definition) the focus has not moved outside the wrapped element.

This may have been a regression introduced in WordPress#16878

* Fix handling blur event when document is not actually focused

Previously we fixed the wrong problem. The actual issue is that previously we were handling document blur events as true “focus outside” events when in fact they should be ignored as if the document is not focused then presumably the user did not intent the blur.

* Update to test for document loss of focus scenario
…s#16997)

* Expand the block style variations documentation to document server functions

* Update docs/designers-developers/developers/filters/block-filters.md

Co-Authored-By: Chris Van Patten <[email protected]>

* Update docs/designers-developers/developers/filters/block-filters.md

Co-Authored-By: Chris Van Patten <[email protected]>

* Update docs/designers-developers/developers/filters/block-filters.md

Co-Authored-By: Chris Van Patten <[email protected]>

* Update docs/designers-developers/developers/filters/block-filters.md

Co-Authored-By: Chris Van Patten <[email protected]>

* Update docs/designers-developers/developers/filters/block-filters.md

Co-Authored-By: Chris Van Patten <[email protected]>

* Update docs/designers-developers/developers/filters/block-filters.md

Co-Authored-By: Chris Van Patten <[email protected]>

* Update docs/designers-developers/developers/filters/block-filters.md

Co-Authored-By: Chris Van Patten <[email protected]>

* Update docs/designers-developers/developers/filters/block-filters.md

Co-Authored-By: Chris Van Patten <[email protected]>

* Update docs/designers-developers/developers/filters/block-filters.md

Co-Authored-By: Chris Van Patten <[email protected]>

* Update docs/designers-developers/developers/filters/block-filters.md

Co-Authored-By: Chris Van Patten <[email protected]>

* Update docs/designers-developers/developers/filters/block-filters.md

Co-Authored-By: Chris Van Patten <[email protected]>

* Update docs/designers-developers/developers/filters/block-filters.md

Co-Authored-By: Chris Van Patten <[email protected]>

* Update docs/designers-developers/developers/filters/block-filters.md

Co-Authored-By: Chris Van Patten <[email protected]>

* Update docs/designers-developers/developers/filters/block-filters.md

Co-Authored-By: Chris Van Patten <[email protected]>

* Fix line break and code block closing
* [RNMobile] update mobile to not use ListEdit

This update is in response to changes made on the web side in WordPress#15113
that were causing a crash on mobile.

* Extract list block logic not shared with mobile to separate component

* Improve name of AdditionalSettings component
* Typewriter XP

* Address feedback

* Do not maintain scroll position when it's out of view

* Reset scroll position when aborting

* Set initial trigger %

* Add test for viewport

* Clean up

* Adjust doc

* Return children for IE

* Hide bottom space when there are metaboxes

* Debounce scroll with RAF
- Add a new gridicon play icon, from: https://github.com/Automattic/gridicons/blob/87c9fce08b4a9f184b9fb4963228757fdd4f4e74/svg-min/gridicons-play.svg
- Replace the Dashicon play by this one
- Update icon size and icon color
- Update the overlay color
* Writing Flow/Quote: allow splitting

* Add extra merge e2e test
…aired node/npm version (WordPress#17134)

* Build: remove global install of latest npm since we want to use the paired node/npm version
* Also update travis to remove --latest-npm flag
* Project automation: Rewrite actions using JavaScript

* Project automation: Don't transpile or install all dependencies

* Project automation: pull-request-automation -> project-management-automation

* Project automation: Add explanatory comment for `npm install` hack

* Project automation: Add debug statements

* Project automation: Don't use GitHub's debug() function

* Project automation: Use `payload` in tasks, not `context`

* Project automation: Link to the relevant GitHub documentation
…s#14776)

* RichText/Patterns/Input Interaction: allow undo of patterns with BACKSPACE and ESC

* Only run input rules when inserting text

* Fix typo

* Simplify
* Editor: Update the store to use Core Data entities.

* Editor: Fix selector test suites.

* Editor: Fix some legacy selectors and behaviors.

* Editor: Fix action tests.

* Editor: Fix remaining broken unit tests.

* Editor: Fix more tests.

* Editor: Fix more e2e test behaviors.

* Editor: Fix preview functionality.

* Core Data: Fix autosaves filtering.

* Editor: Don't make entity dirty with initial edits.

* Editor: Don't save if the post is not saveable.

* Core Data: Fix merged edits logic.

* Core Data: Fix undo to fit e2e expected behaviors.

* Core Data: Handle more change detection and saving flows.

* Block Editor: Fix undo level logic.

* Core Data: Clean up undo reducer comment.

* Editor: Make `serializeBlocks` a util.

* Core Data: Clarify raw attribute usage.

* Core Data: Memoize .

* Core Data: Use new raw entity record selector instead of modifying the existing one.

* Core Data: Make save notices the caller's responsibility.

* Editor: Use the store key constant in actions instead of a string literal.

* Editor: Defer serialization of blocks until save.

* Editor: Fix raw content access in set up.

* Editor: Revert broken test change.

* Editor: Make initial edits a dirtying operation.

* Editor: Add comment clarifying why we set content to a new function on edits.

* Demo: Fix tests to consider the initial edits dirtying.

* Core Data: Set auto-drafts to drafts when autosaving them.

* Core Data: Handle receiving autosaves correctly when editing non-autosave-persisting-properties.
…nderers (WordPress#16512)

* Add callbacks to ServerSideRenderer to handle failures

* Update variable naming and set default renderers

* LoadingResponsePlaceholder

* swtich case

* fetchComplete

* Remove fetchcomplete method

* Pass render props

* add changelog entry

* add documentation for new props
Previously, tapping at the end of the post would insert a block
immediately after the currently selected block. In addition, this commit
is cleaning out a few unusued props in the block-list file.
…WordPress#17156)

GitHub returns the search result count as `data.total_count`, not
`total_count`.
…ess#17027)

* Babel Preset Default: Add missing @wordpress/element dependency

* Remove `@wordpress/dependency-group` and `@wordpress/gutenberg-phase` rules from the `custom` and `recommended` configs and leave them as opt-in features.

* ESLint Plugin: Extract 2 test configs and add them conditionally to the recommended one

* Add missing documentation for changes applied

* Scripts: Update CHANGELOG file
Octokit returns all resources wrapped in a `data` key, e.g.
`data.milestone` instead of `milestone`. Also,
`octokit.repos.getContents()` returns a base64 encoded string.
@gziolo gziolo added [Block] Gallery Affects the Gallery Block - used to display groups of images [Type] Bug An existing feature does not function as intended [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). and removed [Type] Bug An existing feature does not function as intended [Type] Enhancement A suggestion for improvement. labels Aug 27, 2019
@donmhico
Copy link
Contributor Author

Thanks for the input @afercia. Can you look in my latest commit 503a804? It address the points you mentioned.

Also I think I messed up this PR when I tried to rebase this PR with the latest master. Let me know if I need to create a new PR with a new branch.

Thank you.

@jorgefilipecosta
Copy link
Member

Hi @donmhico, thank you for the updates 👍 It seems there we a problem during the rebase of this PR and it now has lots of conflicts and includes unrelated changes.

@guarani
Copy link
Contributor

guarani commented Oct 27, 2020

Looks like this needs to be rebased with master and conflicts resolved to be testable on mobile since this PR pre-dates the monorepo changes.

@guarani
Copy link
Contributor

guarani commented Oct 27, 2020

fyi GitHub has a limit of 15 reviewers so when I remove some mobile reviewers others get removed too (I suppose at some point in the past GitHub allowed more than 15 reviewers to be added).

Base automatically changed from master to trunk March 1, 2021 15:42
@glendaviesnz
Copy link
Contributor

With the merging of the gallery block refactor the gallery images are now Image blocks, which don't currently have an aria label, so alt tag should be read by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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). [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve the Gallery images alt attribute within the editor block