Image block: Set image display to grid when no alignment sent to properly align caption on resize #35787
Conversation
|
Size Change: +308 B (0%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Interesting issue @glendaviesnz! This fix appears to work well for themes where the default styling is for the image block to be centred in the front end view, so it works nicely in TwentyTwentyOne. I noticed in TwentyTwenty, though, the front end styling defaults to the block being rendered left aligned. So with this change applied, in the editor view, resizing the image causes it to appear as though it's centre-aligned, however the front-end view is left aligned.
| TwentyTwentyOne editor view | TwentyTwentyOne front end view |
|---|---|
 |
 |
| TwentyTwenty editor view | TwentyTwenty front end view |
|---|---|
 |
 |
I'm not quite sure what the solution is, but it seems that display: table overrides the theme's max-width for the block, so it effectively makes it centre aligned? I had a quick go at seeing if we could pass width to the style of the RichText instead of using the CSS change, but it then meant we lose the real-time updating of the caption position, so probably doesn't really work as an alternative 🤔
| @@ -59,7 +59,8 @@ figure.wp-block-image:not(.wp-block) { | |||
|
|
|||
| .wp-block[data-align="left"], | |||
| .wp-block[data-align="center"], | |||
| .wp-block[data-align="right"] { | |||
| .wp-block[data-align="right"], | |||
| *:not([data-align]) { | |||
There was a problem hiding this comment.
Does this need to be the broad * selector, or would .wp-block:not([data-align]) work?
There was a problem hiding this comment.
Unfortunately the .wp-block wrapper is only added around the image block if an explicit alignment is set
|
Thank you for creating this PR, Glen @glendaviesnz ! Before:
After:
What I like about this approach is that the boundry box stays close to the image. |
@andrewserong - this is proving to be a difficult one to cover all the basis, which I guess is why the issue has been open so long 😄 I pushed a change to use grid instead of table, and that fixes it for TwentyTwenty, but breaks it for TT1 😢 |
|
Thanks for taking a look @glendaviesnz! Unfortunately it looks like that change also affects the placeholder state and the width when a custom width/height value isn't set:
This is proving surprisingly tricky! I had a go at using the follow, but it wound up causing a variety of other issues too 😞 |
|
I assume one needs to look at what works for most themes and create a trac ticket for either Twenty Twenty or TT1 and get it fixed there. |
|
@glendaviesnz thanks for this fix, looking forward to it making it to a release. only needed for back-end, as I am working with a headless front-end. Curious if anyone had a better idea, since, even if temporary:
thanks! |
|
The work around I have done on a client site is the following... Backend
Frontend
I added this CSS. |
|
@paaljoachim yes I saw that, thanks for the idea! this is for the back-end, in the context of a custom block. and that worked for my needs, in case it helps anyone. (using emotion, but styled-components, etc. would be similar) cheers! 🍻 |
glendaviesnz
force-pushed
the
fix/image-caption-align
branch
from
e694873
to
27344a7
Compare
|
@andrewserong I finally got back to this! I have just pushed a change which seems to fix the issue you identified with the placeholder and non-resized image sizing. |
|
Nice one, thanks @glendaviesnz! That's a clever idea using separate linenames to handle the different behaviour. It's testing well for me at a quick glance, but I'll do some more in depth testing later on today 👍 |
There was a problem hiding this comment.
Nice work @glendaviesnz, this is testing well for me in Chrome, Firefox and Edge on macOS. I noticed some odd behaviour in Safari that I've been able to replicate on trunk (so it doesn't appear to be affected by this PR, but would be good for us look into separately). In Safari, resizing the image block works okay for me at first, but if I switch alignment on the image block (between no alignment and an alignment or vice-versa), I'm then unable to access the resizer controls:
Are you able to replicate that?
Other than that, the only blocker I've run into is that the current e2e test failure for the Image block appears to be caused by this PR, but I'm not sure why. In the test 'should replace, reset size, and keep selection', in the final step, the image block is selected via the .wp-block-image class, and then the Backspace key is pressed to delete the image. This works on trunk, but in testing locally with the following command, it looks like the caret at that stage of the test is within the image caption, so backspace doesn't delete the whole block:
PUPPETEER_HEADLESS="false" npm run test-e2e specs/editor/blocks/image.test.js
I've been unable to replicate getting the caret stuck in the caption while walking through the testing steps manually, so I'm not too sure why the CSS change here would affect it? 🤔
Let me know if you can't replicate the failed e2e test locally, and I can investigate further.
@andrewserong this is weird, I can't replicate that issue in a browser manually as clicking on the block wrapper div never give the figcaption focus for me - but does every time in that test. I can't see how that css change could have caused this and the only way I could fix the test was by clicking on the image wrapper div instead. Not 100% happy with fixing it this way, but a bit stumped on alternatives. |
I could replicate this on trunk on Safari - do you want to add an issue for that @andrewserong ? |
There was a problem hiding this comment.
I could replicate this on trunk on Safari - do you want to add an issue for that @andrewserong ?
Sure thing! Thanks for confirming, I'll open up an issue.
I can't see how that css change could have caused this and the only way I could fix the test was by clicking on the image wrapper div instead.
Me either, it doesn't seem like a bad way to handle it necessarily, but it is confusing why it'd change the behaviour. On its own, I think that change to the e2e test is probably fine — after all, users select the image visually, not by classname 😀
Unfortunately, I ran into another issue re-testing this (sorry I didn't catch it yesterday!) If I upload an image that has a natural size that is less than the width of the current container, then with this PR applied, the resizable box container fills the full width of the container, rather than being constrained to the width of the image (when the image has not yet been resized). It results in the image being stretched during the initial resize:
Could this be due to the resizable box container having grid-column of auto which fills the container rather than being limited to the image width? I couldn't quite work out how to fix it without re-introducing other styling issues 🤔
Turns out there was already and issue for it so I've added my GIF to #24587 |
Nice catch @andrewserong - I have pushed another change which appears to make it behave correctly in both instances. |
There was a problem hiding this comment.
Thanks @glendaviesnz, that improves things, and we're getting closer! There's another couple of issues that I can see now:
The image placeholder doesn't extend to the full width of the container:
In Safari (but seemingly only in Safari), if I switch an Image block from having an alignment to having no alignment it effectively disappears:
I think this is because of the issue where in Safari in that context the div that's a child of the figure element doesn't get the classname .components-resizable-box__container assigned to it:
I'm dealing with a fix for the presence of the classname there (showing the resizer) in #37210, but in the shorter term for this PR, I wonder if we can target it via .wp-block-image > div — though, we'd likely need to account for it being not the .components-placeholder class 🤔
paaljoachim
added
the
[Type] Bug
paaljoachim
added
the
Backport to WP 6.7 Beta/RC
|
I went ahead and added the bug and backtrack labels here, as I assume we might be able to add this to the next Beta of WordPress 5.9. @noisysocks |
@andrewserong not sure if relevant, but in case this helps: |
… align caption on resize
glendaviesnz
force-pushed
the
fix/image-caption-align
branch
from
e5cdfbf
to
834996c
Compare
|
@andrewserong I have pushed another change and it seems to work for placeholder, image with no size set and fixed size image, and in Safari - 🤞 |
There was a problem hiding this comment.
Great work @glendaviesnz, I think that's done the trick!
The issue of the disappearing image when switching alignments in Safari is no longer present, and the placeholder state looks correct, and I also tested it within a columns block, too 👍
I've re-tested in Safari, Chrome, Firefox, and Edge on macOS. I haven't been able to fault it (aside from the image resizer controls visibility in Safari which will be fixed in #37210).
The final thing to deal with now, I think, is that the Cover block e2e test appears to be failing because of this fix (where it attempts to convert an Image block to a Cover block). From running the following test manually, I think it's that the corrected position of the caption input field trips up how the test is written.
For example, here's how the Image block looks within the editor with a tiny image on trunk:
And here's how a tiny image looks while running the e2e test — note that the rich text controls for the caption field obscure the button to convert the image to a cover block:
The good news is that I don't think we need to adjust the CSS any further — the behaviour looks correct to me, but I think we might need an update to that e2e test, perhaps a similar fix to the Image block e2e test?
Here's what I'm running from the command line to observe the test run:
PUPPETEER_HEADLESS="false" npm run test-e2e specs/editor/blocks/cover.test.js
Thanks @andrewserong - I have pushed a fix for this. |
glendaviesnz
changed the title
glendaviesnz
changed the title
noisysocks
removed
the
Backport to WP 6.7 Beta/RC
…rly align caption on resize (#35787) Co-authored-by: Glen Davies <[email protected]>
|
Hey folks, I'm considering reverting this PR, it only impacts the editor which makes the style inconsistent with the frontend. If we ever want to change the behavior of resized images like done here, we should do it for both frontend and editor. See #38613 |
|
Thanks for the heads-up @youknowriad! That makes sense, the fix in this PR was to work around the inconsistency in the editor, and given that the other PR is moving in the direction of further removing the differences between the editor and front-end that sounds reasonable to me. I suppose a challenge with the image resizer is that there'll always be some difference between the editor and frontend DOM due to the injected inserter controls, but hopefully we can come up with a way to work around that without overall changing how the image block and caption are rendered? 🤔 |
I'd love to know if there's still an inconsistency in the editor compared to frontend. I do see that there's still an extra div due to the resized (which might be hard to get rid of) but I wasn't able to have different behaviors in terms of captions between frontend and backend (testing with empty theme without any theme styles that can mess things up) |
|
I just re-tested the image caption position while adjusting the image resizer in a few different blocks-based and classic themes, and it looks like the issue is resolved as of #38613 landing in trunk, so I think we're good here! Thanks so much again for the work in that PR, this looks a better situation all around 😀 |
Description
Currently the image caption does not align correctly when no alignment is set if the image is small, or resized smaller. This PR adds the same
tabledisplay property as is used when an explicit alignment is set.Fixes: #17162
To Test
Screenshots
Before:
After:
