Fix Image block lightbox missing alt attribute and improve accessibility #55010
Conversation
afercia
added
[Type] Bug
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/block-supports/behaviors.php ❔ lib/load.php |
gziolo
added
the
[Feature] Interactivity API
|
Size Change: +343 B (0%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
| $alt_attribute = $processor->get_attribute( 'alt' ); | ||
|
|
||
| // An empty alt attribute `alt=""` is valid for decorative images. |
There was a problem hiding this comment.
excellent comment, and excellent fix above. one small note: null indicates that the alt attribute is missing, but an empty value would be '' or if it's only present as a name like src="…" alt loading=eager then the value would be true
if ( is_string( $alt_attribute ) ) {this might be more appropriate, as it ensures that that we're dealing with an alt=something situation
There was a problem hiding this comment.
In a way, the check is a bit redundant in the first place. As far as I know, WordPress enforces at least an empty alt="" attribute everywhere. I'd agree to check for string anyways, just in case.
lib/block-supports/behaviors.php
Outdated
| if ( $alt_attribute ) { | ||
| /* translators: %s: Image alt text. */ | ||
| $aria_label = sprintf( __( 'Enlarge image: %s', 'gutenberg' ), $alt_attribute ); | ||
| } | ||
| $content = $processor->get_updated_html(); | ||
|
|
||
| $processor = new WP_HTML_Tag_Processor( $block_content ); |
There was a problem hiding this comment.
is this line a mistake here? I think we still have the processor above in line 83
There was a problem hiding this comment.
I'm not familiar with the processor inner workings. To my understanding, after the next_tag( 'img' ) the processor points to the image and not to the entire chunk of HTML. Would get_updated_html(); work anyways. Or am I supposed to create a new instance of the processor? I see several instances od the processor being created, I assumed for this reason. Also, the instances are assigner to variables that I'd argue could be better named. Variable names like $w, $m, $q are very confusing and aren't sekf-explanatory. While those short variable names used to be broadely adopted in the early ages of WordPress, I thought that is nwo considered a non-ideal practice. Certainly doesn't help beginners understand how the processor is supposed ot be used.
There was a problem hiding this comment.
Would get_updated_html(); work anyways. Or am I supposed to create a new instance of the processor?
Good catch — we no longer need $content here, and we we're not modifying any HTML yet, so we don't need to do the $processor->get_updated_html() call either.
I see several instances od the processor being created, I assumed for this reason.
Also, the instances are assigner to variables that I'd argue could be better named. Variable names like $w, $m, $q are very confusing and aren't sekf-explanatory.
The other instances of the processor are created because we need to do different processing on the figure and img elements for each instance.
Note: We create new instances of the processor for these three scenarios because the processor is unable to back up or reverse.
There is an exception to that though — we could create a bookmark for the figure element and use that to refactor the code for clarity, removing the need for the different instances and cutting down on the variable names if you think that would be clearer. I believe that should work.
Another option: We could rename the $w, $m, and $q variables to $lightbox_container_processor, $initial_image_processor, and $enlarged_image_processor, or something along those lines.
In either case, we can just use $block_content for the new instance(s) rather than $content — which is no longer necessary — and that should help make the code clearer as well.
There was a problem hiding this comment.
I see several instances od the processor being created, I assumed for this reason. Also, the instances are assigner to variables that I'd argue could be better named. Variable names like $w, $m, $q are very confusing and aren't sekf-explanatory. While those short variable names used to be broadely adopted in the early ages of WordPress, I thought that is nwo considered a non-ideal practice. Certainly doesn't help beginners understand how the processor is supposed ot be used.
Yeah this code is ripe for refactoring. Maybe next week @artemiomorales and I can try and do that. The names, the duplicate processors, they can all be improved.
Would get_updated_html(); work anyways.
@afercia get_updated_html() will always return the full HTML document. if nothing has been modified it will be identical to what was passed in when creating the Tag Processor. if modifications have been made, it will return the result of applying all requested changes to that document.
There was a problem hiding this comment.
@dmsnell Sure, we can do that 👍
lib/block-supports/behaviors.php
Outdated
| $content = $processor->get_updated_html(); | ||
|
|
||
| $processor = new WP_HTML_Tag_Processor( $block_content ); | ||
| $content = $processor->get_updated_html(); |
There was a problem hiding this comment.
if we don't have any calls to set_attribute() or other functions that modify the content, then this line isn't necessary. it should be equivalent to setting $content = $block_content;
am I missing some lines where we set the alt attribute or aria label before this line?
There was a problem hiding this comment.
I'm not sure I understand how the processor is used in this file. If that's true, then why creating one more instance at line 111 and assigning it to $z? Is the content modified in any way before that point?
There was a problem hiding this comment.
if we don't have any calls to set_attribute() or other functions that modify the content, then this line isn't necessary. it should be equivalent to setting $content = $block_content;
am I missing some lines where we set the alt attribute or aria label before this line?
It looks like this line is no longer necessary and was from an earlier implementation — we can remove this.
I'm not sure I understand how the processor is used in this file. If that's true, then why creating one more instance at line 111 and assigning it to $z? Is the content modified in any way before that point?
This new instance also looks like it is not necessary — it can be removed and we can reuse the first instance.
| $alt_attribute = $processor->get_attribute( 'alt' ); | ||
|
|
||
| // An empty alt attribute `alt=""` is valid for decorative images. |
There was a problem hiding this comment.
see above comment about is_string()
| $content = $processor->get_updated_html(); | ||
|
|
||
| $processor = new WP_HTML_Tag_Processor( $block_content ); | ||
| $content = $processor->get_updated_html(); |
There was a problem hiding this comment.
see above comments about recreating the processor and getting unmodified contents.
|
Flaky tests detected in ae939eb. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6425080851
|
afercia
changed the title
afercia
requested review from
spacedmonkey,
luisherranz,
DAreRodz,
artemiomorales and
michalczaplinski
as code owners
afercia
force-pushed
the
fix/lightbox-missing-alt-and-accessibility
branch
from
87538f8
to
82ade8e
Compare
|
A few notes:
|
afercia
changed the title
afercia
force-pushed
the
fix/lightbox-missing-alt-and-accessibility
branch
from
82ade8e
to
b887286
Compare
|
@afercia Tbanks for opening this PR! Here are some notes:
All the changes would need to be applied to the
I left comments here and here on the usage of the processor. In short, 1.) some of the code is outdated and can be removed, and 2.) we could make use of a bookmark to cut down on instantiating new Tag Processor instances if that would help with readability. |
There was a problem hiding this comment.
@afercia I followed the test instructions and everything seems to be working.
I pushed up a change removing some obsolete code that should hopefully make the usage of the Tag Processor clearer.
I also added some changes to block-support/behaviors.php, and also added its inclusion in load.php, which was apparently mistakenly removed at some point.
Note: I'm finding the block supports hard to test since we no longer use it for any part of the image or lightbox. Since we fire a _deprecated_function() call that breaks the layout on the front end anyway, it makes me think that keeping the behaviors.php up-to-date is actually not strictly necessary, but I'm happy to hear other thoughts on this.
I can approve this PR pending a decision on how to handle code cleanup on the multiple instances of the Tag Processor. Thanks! 🙏
artemiomorales
added
the
Backport to WP 6.7 Beta/RC
|
It seems fitting to me to let this slide through without addressing all the Tag Processor issues. We can hit those up in a single pass after updating the |
This actually sounds good to me, as there's already a fair amount happening in this PR. I'll go ahead and approve it. |
|
Thanks both of you! |
|
Thanks everyone for your feedback and clarifications. |
…ity (#55010) * Move lightbox open button after the image. * Fix getting the lightbox image alt attribute. * Improve docblocks. * Do not render empty role attribute. * Remove unnecessary aria-hidden attribute. * Set aria-modal attribute dynamically. * More meaningful and simpler modal dialog aria-label. * Increase Close button target size. * Add enlarged image base64 encoded placeholder. * Better check for alt attribute as a string. * Update changelog. * Move changelog entry to the block library changelog. * Set lightbox dialog aria-label dynamically. * Hide background scrim container from assistive technology. * Remove obsolete code --------- Co-authored-by: Ricardo Artemio Morales <ric.morales22@gmail.com> # Conflicts: # packages/block-library/CHANGELOG.md
mikachan
removed
the
Backport to WP 6.7 Beta/RC
|
I just cherry-picked this PR to the 6.4-beta3 branch to get it included in the next release: 89f5e33 |
* Add clearer labels and context to BlockPatternsSyncFilter (#54838) * Add help text & clearer labeling * Theme & Plugins * Font Library: use snake_case instead of camelCase on fontFamilies endpoint param (#54977) * use snake_case instead of camelCase on endpoint param * updating test * Fix output of Navigation block classnames in the editor. (#54992) * Block Editor: Avoid double-wrapping selectors when transforming the styles (#54981) * Block Editor: Avoid double-wrapping selectors when transforming the styles * Include space in the check * Don't display the navigation section in template parts details when a menu is missing (#54993) * Scripts: Properly use CommonJS for default Playwright config (#54988) * Fix path to `globalSetup` in default Playwright config Oversight from #54856 * `module.exports` * Fix default export usage * Add template replace flow to template inspector (#54609) * Add a modal to allow template switching * fetch template info * Allow switching to different patterns * Allow switching to different patterns * Add columns * move availble templates to the actions * filter for the correct templates * create the right data structure in the use select * move to a hook * inject theme attribute into pattern again * put the overlay over the top of the dropdown * fix the pattern to templates hook * set the template on click * Also set the blocks * remove calls to set template with the current template, since setting blocks correctly updates the content in the editor * serialize blocks so that we have correctly processed template parts * remove duplicated code * Remove unnecessary mapping * refactor * memoize the patterns * combine the useSelect * Update packages/edit-site/src/components/sidebar-edit-mode/page-panels/hooks.js Co-authored-by: Andrei Draganescu <me@andreidraganescu.info> * Fix ESLint error * Only show the button is there is more than 1 pattern * Copy update * Move the hook to a subdir * check that there are patterns * move the check * remove useCallback * change condition to show the button * change condition * move to use editEntityRecord * combine filters * add comments * Update packages/edit-site/src/components/sidebar-edit-mode/template-panel/replace-template-button.js Co-authored-by: Andrei Draganescu <me@andreidraganescu.info> --------- Co-authored-by: Andrei Draganescu <andrei.draganescu@automattic.com> Co-authored-by: Andrei Draganescu <me@andreidraganescu.info> Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com> * List View: Fix performance issue when selecting all blocks (#54900) * List View: Fix performance issue when selecting all blocks within the editor canvas in long posts * Add a comment, rename const * Move block focus to be performed only once at the root of the list view, instead of within each block * Fix left and right aligmnent in children of Post Template (#54997) * Fix left and right aligmnent in children of Post Template * Add align center styles * Fix image placeholder disappearing * Site Editor: Avoid stale navigation block values when parsing entity record (#54996) * Removed unwanted space from the string (#54654) * Update styles.js Removed unwanted space with translation * Update deleted-navigation-warning.js Unwanted space at the end of the string shows translation warning. * Update inspector-controls.js Unwanted space at the end of the string shows translation warning * Fix Deleted Navigation Menu warning string (#55033) * [Inserter]: Fix reset of registered media categories (#55012) * [Inserter]: Fix reset of registered media categories * convert `useInserterMediaCategories` to selector and make private * Try fixing the flaky 'Toolbar roving tabindex' e2e test (#54785) * Try fixing the flaky 'Toolbar roving tabindex' e2e test * Add a link in the comment * Fallback to Twitter provider when embedding X URLs (#54876) * Fallback to Twitter provider when embedding X URLs * Avoid processing empty urls when trying a different provider * Update `react-native-editor` changelog # Conflicts: # packages/react-native-editor/CHANGELOG.md * Based on the efforts in #51761, remove caps case from Template Part and prefer sentence case. As all instances of this string are stand alone, it's okay to have Template capitalized as it's the start of a sentence. (#54709) * Update pattern import menu item (#54782) * Update pattern import menuitem * Revert label * Image Block: Fix browser console error when clicking "Expand on Click" (#54938) * Patterns: Remove category description in inserter panel? (#54894) * Media & Text: Fix React warning (#55038) * Block Style: Display default style labels correctly in the block sidebar (#55008) * Site Editor: Do not display 'trashed' navigation menus in Sidebar (#55072) * Site Editor: Do not display 'trashed' navigation menus in Sidebar * Extract selector into a hook Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> --------- Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> * Image: Fix Lightbox display bug in Classic Themes. (#54837) * If current theme is not a block theme add a default value for $background_color and $close_button_color. * Set lightbox buttons' background & border to none on hover & focus * Change logic to support lightbox in classic themes * Update logic to avoid unnecessary calls * Add style fixes * Remove unnecessary code * Fix close button color --------- Co-authored-by: Mario Santos <santosguillamot@gmail.com> Co-authored-by: Ricardo Artemio Morales <ric.morales22@gmail.com> * Latest Posts: add screen reader title text to Read more links and use alternative to excerpt_more filter (#55029) * In the editor: adds a screen reader text span with the post title in the i18n interpolator In the frontend: removes excerpt_more filter so we don't override themes and also replaces the default ellipsis with an accessible read more link * Removing "of" preposition in favour of a semi-colon. "Read more" is already translated so using a specifier to add it to the string * Fix Image block lightbox missing alt attribute and improve accessibility (#55010) * Move lightbox open button after the image. * Fix getting the lightbox image alt attribute. * Improve docblocks. * Do not render empty role attribute. * Remove unnecessary aria-hidden attribute. * Set aria-modal attribute dynamically. * More meaningful and simpler modal dialog aria-label. * Increase Close button target size. * Add enlarged image base64 encoded placeholder. * Better check for alt attribute as a string. * Update changelog. * Move changelog entry to the block library changelog. * Set lightbox dialog aria-label dynamically. * Hide background scrim container from assistive technology. * Remove obsolete code --------- Co-authored-by: Ricardo Artemio Morales <ric.morales22@gmail.com> # Conflicts: # packages/block-library/CHANGELOG.md * Patterns: Add category selector to pattern creation modal (#55024) --------- Co-authored-by: Kai Hao <kai@kaihao.dev> * Iframe: Fix positioning when dragging over an iframe (#55150) * Site Editor: Fix template part area listing when a template has no edits (#55115) * Alternative: Fix template part area listing when a template has no edits * Fix typos --------- Co-authored-by: Rich Tabor <hi@richtabor.com> Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> Co-authored-by: tellthemachines <tellthemachines@users.noreply.github.com> Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com> Co-authored-by: Pascal Birchler <pascalb@google.com> Co-authored-by: Ben Dwyer <ben@scruffian.com> Co-authored-by: Andrei Draganescu <andrei.draganescu@automattic.com> Co-authored-by: Andrei Draganescu <me@andreidraganescu.info> Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com> Co-authored-by: mujuonly <muju.only@yahoo.in> Co-authored-by: Dave Smith <getdavemail@gmail.com> Co-authored-by: Nik Tsekouras <ntsekouras@outlook.com> Co-authored-by: Carlos Garcia <fluiddot@gmail.com> Co-authored-by: Ramon <ramonjd@users.noreply.github.com> Co-authored-by: James Koster <james@jameskoster.co.uk> Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> Co-authored-by: Michal <mmczaplinski@gmail.com> Co-authored-by: Mario Santos <santosguillamot@gmail.com> Co-authored-by: Ricardo Artemio Morales <ric.morales22@gmail.com> Co-authored-by: Andrea Fercia <a.fercia@gmail.com> Co-authored-by: Glen Davies <glendaviesnz@users.noreply.github.com> Co-authored-by: Kai Hao <kai@kaihao.dev>
|
✅ I updated the PHP Sync Tracking Issue for WP 6.5 to note this PR does not require a backport for WP 6.5 as it seems to have been cherry picked into WP 6.4 by @mikachan. |
Addresses part of the issues from #54971
What?
aria-hiddenfrom the closed modal dialog. This appears to be redundant as the closed modal dialog is already hidden from assistiv technology by the means of thevisibility: hiddenCSS property.aria-modalattribute dynamically instead of hardcoding on hte PHP side. This is also consistent with similar code in the navigation block.aria-labelattribute dynamically.role=""attribute.src=""attribute. A small base64 encoded gif is now used as 'placeholder' for the enlarged image.Why?
aria=*attributes androlewhren the modal dialog is closed is either invalid HTML or not ideal.How?
See description above.
Testing Instructions
Enlarge imagefor the image without alt attribute.Enlarge image: {image alt text here}for the image with alt attribute.Enlarged image.Additionally:
roleattribute.aria-modalattribute.aria-hiddenattribute.aria-labelattribute.srcattribute of the enlarged image is a base64 encoded image.role="dialog"attribute.aria-modal="true"attribute.aria-hiddenattribute.aria-label="Enlarged image"attribute.srcattribute of the enlarged image is now the one of the actual enlarged image.Testing Instructions for Keyboard
Screenshots or screencast