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 Inconsistent Labels for Lightbox Feature #68261

Merged

Conversation

karthick-murugan
Copy link
Contributor

@karthick-murugan karthick-murugan commented Dec 24, 2024

What?

Fixes #55617

This PR ensures consistent terminology between the editor (backend) and frontend for the lightbox feature. The label "Expand on click" in the editor has been updated to "Enlarge image" for alignment with the frontend aria-labels.

Why?

Consistency in terminology is vital for accessibility and usability. Users of assistive technology will now encounter the same terms ("Enlarge image") in both the editor and frontend, reducing potential confusion.

How?

  • Updated the editor UI setting label from "Expand on click" to "Enlarge image."
  • Verified that the aria-label for the trigger button and modal dialog in the frontend is "Enlarge image," ensuring consistency across the interface.

Testing Instructions

  1. Open a post or page.
  2. Insert an image block with the lightbox feature enabled.
  3. Observe that the lightbox setting in the editor now reads "Enlarge image."

Screenshots or screencast

After making changes

enlarge_1

enalrge_2

enlarge_3

Copy link

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: karthick-murugan <[email protected]>
Co-authored-by: afercia <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@karthick-murugan
Copy link
Contributor Author

@afercia - Please have a look at this PR and let me know if you have any suggestions.

@@ -234,6 +229,7 @@ class="lightbox-trigger"
type="button"
aria-haspopup="dialog"
aria-label="' . esc_attr( $aria_label ) . '"
aria-describedby="' . esc_attr( $alt ) . '"
Copy link
Contributor

Choose a reason for hiding this comment

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

The aria-describedby attribute should point to the ID of a hidden element that contains the description. If it turns to be too complicated, we could consider to not use a description at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@afercia - Considering the complexity of this implementation, I have removed the aria-describedby attribute as per your suggestion. We can address it in a separate PR to ensure it is handled comprehensively.

Copy link
Contributor

@afercia afercia left a comment

Choose a reason for hiding this comment

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

I left a comment about aria-describedby that should be addressed.
Also, some tests are failing because they still expect a button with name 'Expand on click'', they should be updated/
A quick note; I think the preferred way to sync with trunk is to rebase insteaf of merging trunk.

@afercia afercia added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback. [Block] Image Affects the Image Block labels Jan 8, 2025
@afercia
Copy link
Contributor

afercia commented Jan 8, 2025

@karthick-murugan thanks for working on this.
The aria-label of the modal dialog should be adjusted to be more meaningful. Currently, it uses the same text used for hte button: Enlarge. However, the modal dialog should be labeled to communicate this is the enlarged image. Something simple like Enlarged image would work, I think.

I think that the button label should stay __( 'Enlarge' ) and a new $dialog_aria_label variable should be added with value __( 'Enlarged image' ) and then be used here.

Copy link
Contributor

@afercia afercia left a comment

Choose a reason for hiding this comment

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

Looks good to me but a new variable should be adder to be used for the modal dialog label.

@karthick-murugan
Copy link
Contributor Author

Looks good to me but a new variable should be adder to be used for the modal dialog label.

@afercia - Thanks for your time on reviewing this PR. Updated the changes that you have requested.

Copy link
Contributor

@afercia afercia left a comment

Choose a reason for hiding this comment

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

Looks good to me.
This improves naming consistency, simplifies labeling and makes the UI more predictable.

@afercia afercia merged commit d74dfb9 into WordPress:trunk Jan 8, 2025
64 checks passed
@github-actions github-actions bot added this to the Gutenberg 20.1 milestone Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Image Affects the Image Block [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image block: Lightbox setting name and labels are inconsistent
2 participants