-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Cover: Add aria-label to fixed and repeated image backgrounds #50990
Conversation
Size Change: +64 B (0%) Total Size: 1.66 MB
ℹ️ View Unchanged
|
Flaky tests detected in 3d8dcf9. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6556761046
|
Deprecation:
Fixtures: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not do a full review, but from a quick look the code seems reasonale. I'm not 100% sure about the usability of this, as alt
and aria-label
arew different.
cc @afercia for some feedback here 🙏
I feel this is definitely an improvement over what we currently have, but it would be good to have some additional feedback here
@@ -19,9 +19,6 @@ | |||
}, | |||
"alt": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the name of the attribute is reflective of what it does now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the name could be "accessibleText". I'm not sure that is clearer.
Since it corrects invalid markup produced by the block (having an alt attr for a div with role img) I labeled this as a bug fix. The code looks good 👏🏻 the only question is if this is the way to do it, same as Ari mentioned, is the solution in the PR a recommented way to handle this situation or an ad hoc workaround that works? |
I don't disagree but we are only allowed one Type label. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will approve this because honestly this looks like the only way to achieve a better assistive tech experience for this situation. The code looks good.
What?
When a cover block has a fixed or repeated background, it uses a div with the attribute
role="img"
.The goal of this PR is to solve two issues:
role="img"
should not be used if the image is presentational (decorative) and has noalt
text.role="img"
and an alternative text. Becausealt=""
is not a valid option for adiv
withrole="img"
,aria-label
is used instead.Closes #49902
Why?
To improve accessibility when the fixed background or repeated backgrounds are used.
How?
In block.json, the alt block attribute used the
img
as the selector and thealt
as the attribute, so I had to remove thesefrom the block.json. I was not able to make this work with the
<image
, the<div>
, thealt
, and thearia-label
without removing it.Previous:
In the PR:
Testing Instructions
role="img"
and does not have anaria-label
. Do the same on the front.img
tag and thealt
attribute. Do the same on the front.Testing Instructions for Screen readers
Screenshots or screencast