Improve background image control #54439
Conversation
|
Size Change: +92.9 kB (+6%) 🔍 Total Size: 1.62 MB
ℹ️ View Unchanged
|
richtabor
added
[Type] Enhancement
| @@ -96,19 +97,38 @@ export function resetBackgroundImage( { attributes = {}, setAttributes } ) { | |||
| } ); | |||
| } | |||
|
|
|||
| function InspectorImagePreview( { label, url: imgUrl } ) { | |||
| function InspectorImagePreview( { label, filename, url: imgUrl } ) { | |||
| const imgLabel = label || getFilename( imgUrl ); | |||
There was a problem hiding this comment.
imgLabel appears to now always be Background image 🤔
There was a problem hiding this comment.
Yea, we can consolidate that.
|
I like the change to an ItemGroup appearance. This is perhaps general to the entire implementation, and the name of the panel itself (and thus ultimately separate from this PR), but it feels redundant that the Panel is called "Background image" and the item is called "Background image". I know "Media" and "Background" were discussed as simpler panel titles, and ultimately dismissed, but was this redundancy considered in that conversation, and if not, can we revisit? |
|
Expanding on a specific question asked on Slack: How to communicate the Selected color (or no color selected) for the Text and Background buttons: 
ideally:
About name and description:
Please remove it. Honestly, I'm a bit surprised this title attribute slipped through and no one noticed it. It's a few years that WordPress has been progressively removing title attributes from the codebase. Title attributes are only accessible to a tiny fraction o fusers and as such they serve no useful purpose. No new title attributes should be introduced ever in WordPress. |
It was not. As @andrewserong pointed out the control originally displayed the file title/name, but it seems that needs to change for a11y. I agree that having a "Background image" control within a "Background image" panel now feels redundant. That said, having a "Background" control in a "Color" panel and a "Background image" control in a "Background" panel feels a bit unorganised. I'd expect to find the background options grouped together. This may be a can of worms, but did we ever consider contextual color control placement? IE put the text color options in the Typography panel, and the background color option in a Background panel (along with the new background image control). Border color is already organised this way and the inconsistency is a bit strange to me. |
@richtabor and I discussed this a bit yesterday too. I believe part of the rationale for separate groupings was that there are plans for there to be other controls within the background image area, such as focal point, duotone, etc. |
Yes. Following suite of the color selection above, where there is not the hex value of the color presented visually. And reduces the changing of the button text. |
Yea I agree. What about if the panel was called "Fill"? That way we can still house other controls like focal point, repeat, etc. That, or "Media" but I'm not sure on media. |
Yes, if we switch the label of the button to "Background image" in this PR, I agree, renaming the panel label sounds good. I'd probably vote for "Background" — since the panel is next to the Color panel, maybe it isn't too confusing if there are two things called Background?
Ah, gotcha! Yes, I like the consistency of the button in this PR, it does feel more solid that it retains a single state visually between when there's an image and when there isn't one. One of the challenges with images in comparison to colors is that it's a bit harder to distinguish which particular image you've selected via the thumbnail, whereas with color or gradient colors, the preview is clearer at a glance. Would it be worth looking at how we might still expose the image title somewhere in the UI, for example as a Tooltip, or potentially in the dropdown menu when you click on the Background image button? In this way, maybe we could borrow the approach from the Replace button in the Image block, where it says "Current media URL:" 
That could totally be a separate PR, though. Which reminds me, I'll add "support adding images via URL" to the background image tracking issue 🙂 |
The question was more about whether the background color option should move to the Background/Fill panel. Similar to how the border color option lives in the Border panel. But it's a topic for another issue 😅 |
May be interesting to explore that. Cover does not do this though. My hesitation is that if your background image is a randomly named image, the title isn't a helpful affordance visually. One note to add is that while the image indicator is small, you do have the group block in focus on the left as a confirmation. You're not selecting and editing the block without it in view. |
@afercia Do you mind opening a separate issue for that, to continue the discussion for the color and background components. Thanks! |
Good point! Another option would be to only display the tooltip or |
Agreed! |
|
I think this works fine: 
|
|
Is there anything else needed for this? |
|
Flaky tests detected in 80c8b1b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6229516354
|
Looks pretty complete to me! Do we need an accessibility review to ensure everyone's happy with the change? The code looks good to me 👍 I can give it the ✅ tomorrow if we don't hear back from anyone 🙂 |
|
Tested it out. I'm getting a focus loss when selecting an image. Focus should be returned to the Background image button after any selection that was triggered by the button. Here are the states I'm getting a focus loss:
When there is no image selected, I think we should communicate this in the aria-describedBy as well. Something like, "No image selected."? The selected image description is working great, btw! |
I'm cool with following up on this, as the issue persists in trunk. |
There was a problem hiding this comment.
I'm cool with following up on this, as the issue persists in trunk.
That sounds good to me, too, the changes here are an improvement over what we have on trunk, so this looks like a good iterative improvement to me 👍
I suspect the focus loss issues are to do with using a different button when in the empty state versus when an item is selected. In a follow-up it might be worth trying to see if we can consolidate around only using the MediaReplaceFlow component, even in the empty state, so that the button focus can be properly restored. We'd also need to make sure the drop zone still works as expected.
If that doesn't work, or if we still need the separate button for the empty state, then we could look into explicitly directing focus when onSelectMedia is called. But let's look into that separately.
This is testing nicely for me and looks good in Safari, FF, Chrome, and Edge:
| Empty state | With an image |
|---|---|
 |
 |
LGTM! ✨
|
I'll merge this in now so that it can make it for 6.4 Beta 1. Thanks for working on this @richtabor! |
andrewserong
added
the
Backport to Gutenberg RC
I've opened up a follow-up PR that deals with the focus loss problem over in #54711. The solution seems to be to use the |
* Update background.js * Update background.scss * Update background.scss * Use variables * Update background.js * Add describedby for empty state * Add filename description (wip) * Update a11y based on feedback * Use "Background" for panel name
|
I just cherry-picked this PR to the release/16.7 branch to get it included in the next release: e530e3e |
mikachan
removed
the
Backport to Gutenberg RC
What?
Follow-up #53934 to bring this more in line with the color controls. There are also less changes between states (with or without a background image added). I don't love the duplicate panel and button labels though.
A11y based on feedback from the accessibility channel on the Making WordPress Slack.
Testing Instructions
Screenshots or screencast
After
CleanShot.2023-09-13.at.16.44.44.mp4