Image: Fix Lightbox display bug in Classic Themes. #54837
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ound_color and $close_button_color.
michalczaplinski
added
[Type] Bug
|
@alexstine I'd appreciate your opinion on the accessibility implications here. It should be fine to override the theme's CSS to avoid the ugly (and presumably unwanted) highlights on hover/focus, but if you know of a better way, let me know! |
|
Size Change: +19.2 kB (+1%) Total Size: 1.64 MB
ℹ️ View Unchanged
|
|
CC: @joedolson |
michalczaplinski
added
the
Backport to WP 6.7 Beta/RC
t-hamano
reviewed
Oct 2, 2023
There was a problem hiding this comment.
I think it's unstable to rely solely on whether it's a block theme or not. Block themes don't necessarily have background and text colors defined in theme.json. For example, if you enable an Emptytheme (with no colors defined in theme.json), the error message persists.
I think the wp_get_global_styles() function returns an array not depending on whether the theme is a block theme or not, but when the key does not exist in the global style.
I think we should check the return value of the wp_get_global_styles() function or use some other function that is more reasonable, but as far as I know of no implementation that references global styles in a core block callback function, so I would like to ping @aaronrobertshaw and @ramonjd for help 🙇
|
After doing some research, I think the following code might be better. $global_styles = wp_get_global_styles( array( 'color' ) );
$has_background_color = ! empty( $global_styles['background'] );
$has_text_color = ! empty( $global_styles['text'] );
$background_color = esc_attr( $has_background_color ? $global_styles['background'] : '#fff' );
$close_button_color = esc_attr( $has_text_color ? $global_styles['text'] : '#000' ); |
There was a problem hiding this comment.
@t-hamano makes a good point. Not only can block themes not have background and text colors, but we could also have a hybrid theme that uses theme.json.
Would using wp_theme_has_theme_json assist here?
Once we're checking global styles for all themes that might use theme.json, we still need to handle the edge case where background/text colors aren't set as Aki mentioned.
P.S. Could we get some test instructions on the PR description? It might help others jump into reviews here, especially for a11y feedback and people that don't have the luxury of discerning info from videos.
|
Thanks a lot for the feedback. It totally makes sense to me. I just pushed a commit with your suggestions. I hope that's why you had in mind. Feel free to correct me I am wrong 🙂
Regarding this, we should always include proper testing instructions. Thanks for flagging it. I'll leave that to @michalczaplinski as he opened the pull request, but the way I was testing it was:
Before this change
After this change
|
swissspidy
reviewed
Oct 3, 2023
Comment on lines
261
to
262
| $background_color = esc_attr( ( wp_theme_has_theme_json() && $has_background_color ) ? $global_styles_color['background'] : '#fff' ); | ||
| $close_button_color = esc_attr( ( wp_theme_has_theme_json() && $has_text_color ) ? $global_styles_color['text'] : '#000' ); |
There was a problem hiding this comment.
Just call wp_theme_has_theme_json() once above to avoid duplicate calls
There was a problem hiding this comment.
If you don't have a theme.json, I think you shouldn't need to call wp_get_global_styles(). Furthermore, how about an approach like below to run the wp_theme_has_theme_json() function only once?
$background_color = '#fff';
$close_button_color = '#000';
if ( wp_theme_has_theme_json() ) {
$global_styles_color = wp_get_global_styles( array( 'color' ) );
if ( ! empty( $global_styles_color['background'] ) ) {
$background_color = esc_attr( $global_styles_color['background'] );
}
if ( ! empty( $global_styles_color['text'] ) ) {
$close_button_color = esc_attr( $global_styles_color['text'] );
}
}
There was a problem hiding this comment.
It makes sense to me! Thank you 🙂 I just updated the code with your suggestion.
There was a problem hiding this comment.
Thanks for the update, @SantosGuillamot!
If the diff I'm checking is the latest, then the five lines here shouldn't be necessary 😄
|
I've added a couple of style fixes that address issues in a couple of other classic themes as documented in #54940 Note: While the focus ring on the close button should always appear when using a keyboard, we want to omit that style when opening the lightbox with a mouse click. This generally works as expected, but the focus ring is erroneously showing up in Twenty Twenty One and Twenty Nineteen when using the mouse. I mention this so we can have it on our radar, but we can likely fix that as part of a separate PR. |
There was a problem hiding this comment.
Thanks for the continued iterations here 👍
As noted earlier there's some unnecessary code we can remove.
With that code removed, I only encountered one issue while testing.
The recent change to the button's inline styles also appears to prevent the $close_button_color from being applied correctly. Reverting the inline style back to fill applies the color on the button's inner SVG. cc/ @artemiomorales
To replicate:
- Activate TT4 theme
- Appearance > Editor > Styles and select Onyx variant
- Save changes
- Visit page with image and lightbox and expand
- Note the incorrect dark color for the
X
Screen.Recording.2023-10-04.at.10.38.49.am.mp4
Tested with the following themes:
- TT1
- TT3
- TT4
- emptytheme
- emptyhybrid
|
Just noting that in #55010 I'm going to add a minimum width and height to the Close button, as the target size is really too small. I guess CSS from themes may impact also the size of the button, which should be at least around 40 by 40 pixels. |
|
I totally missed those lines 😅 Thanks for noticing! I just removed them.
I confirm I can reproduce it as well. I reverted it back to use Let me know if that's correct. |
aaronrobertshaw
approved these changes
Oct 5, 2023
There was a problem hiding this comment.
Thanks for the continued iterations here @SantosGuillamot 👍
LGTM! Tests as advertised as well.
For posterity, it might be good to tweak the PR description so the "How" section matches the latest approach i.e. checking if the theme has a theme.json file rather than only being a block theme.
|
Thanks for the review!
That makes sense 🙂 @michalczaplinski as you opened the PR, could you handle that? |
mikachan
pushed a commit
that referenced
this pull request
Oct 5, 2023
* 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>
|
I just cherry-picked this PR to the 6.4-beta3 branch to get it included in the next release: ae40d77 |
mikachan
removed
the
Backport to WP 6.7 Beta/RC
|
Thanks for the great work, @SantosGuillamot! I don't know much about the Lightbox implementation itself, but I believe it used to be called "Behaviours". And similar code that this PR fixes exists here as well. Is this code also used in Lightbox? If so, you might want to make similar changes in your follow-up PR. |
If I am correct, that file is there for backward-compatibility in the Gutenberg plugin. This functionality was previously included as a "behavior" in Gutenberg, so people could have activated that. I believe the plan is to maintain it for 2 releases. Although @michalczaplinski and @artemiomorales will know better than me. Having said that, it makes sense to me to replicate the logic there as well. Although I believe it is unlikely that there are already users with the functionality enabled and using a classic theme because this wasn't reported before.
EDIT: I looked at this I am not sure if |
karmatosed
pushed a commit
that referenced
this pull request
Oct 9, 2023
* 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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Co-authored with @SantosGuillamot
What?
Fixes #54682.
Why?
In Classic Themes, the
wp_get_global_styles()call returns the whole array of all styles, instead of an individual string value. An array cannot be interpolated into HTML (on this line):gutenberg/packages/block-library/src/image/index.php
Line 286 in d3986f0
so it throws a warning.
The CSS of a theme can include styles for
button:hoverandbutton:focus. Since the Lightbox contains a<button>element, we want to reset those styles to avoid the ugly unwanted flashes of content.How?
wp_is_block_theme()check before getting the value fromwp_get_global_styles().button:hover&button:focus.Screenshots or screencast
Before
Screen.Recording.2023-09-26.at.18.28.54.mov
After
Screen.Recording.2023-09-26.at.18.29.26.mov