Image: Fix resizer controls being hidden in Safari when switching between alignments #37210
Conversation
andrewserong
added
[Type] Bug
|
Size Change: +128 B (0%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
|
Update: this fix will need some adjustment, I just noticed it introduces a regression when dealing with external images when the external image is initially added to the block (the resizer is not visible). We might wind up needing to retain the |
andrewserong
added
[Status] In Progress
Another update: I think I've fixed up the issue by preserving the local component state for the loaded image's natural width / height. We then use that as a fallback value, and prioritise the real |
|
Hey @andrewserong |
andrewserong
added
the
Backport to WP 6.7 Beta/RC
@paaljoachim yes, we sure do! I've added the label. 👍 |
andrewserong
force-pushed
the
fix/image-block-resizer-controls-availability-in-safari-after-switching-alignments
branch
from
7cbb1b5
to
573e521
Compare
|
This tests well for me. An alternative, rather than using image onload method at all for this, would be to use const imgRef = useRef();
const imageOnload = () => {
setNaturalSize(
pick( imgRef.current, [ 'naturalWidth', 'naturalHeight' ] )
);
};
useEffect( () => {
if ( imgRef.current?.complete ) {
imageOnload();
}
}, [ imgRef.current?.complete ] );
let img = (
// Disable reason: Image itself is not meant to be interactive, but
// should direct focus to block.
/* eslint-disable jsx-a11y/no-noninteractive-element-interactions, jsx-a11y/click-events-have-key-events */
<>
<img
ref={ imgRef }
src={ temporaryURL || url }
alt={ defaultedAlt }
onError={ () => onImageError() }
/>
{ temporaryURL && <Spinner /> }
</>
/* eslint-enable jsx-a11y/no-noninteractive-element-interactions, jsx-a11y/click-events-have-key-events */
); |
Actually it only works for existing images - |
My bad, the useEffect in above should be: useEffect( () => {
if ( imgRef.current?.complete ) {
imageOnload();
}
}, [ imgRef.current?.complete ] );have updated the full example above, and now seems to test well for me for existing images on page load and new images added. |
|
Thanks for reviewing @glendaviesnz! Nice, I hadn't thought of using |
| loadedNaturalHeight || | ||
| undefined, | ||
| }; | ||
| }, [ loadedNaturalWidth, loadedNaturalHeight, imageRef.current ] ); |
There was a problem hiding this comment.
I don't think we need to include imageRef.current in the dependency, at least based on exhaustive-deps 😄
There was a problem hiding this comment.
Thanks! I've removed the useMemo call, since I think setting the naturalWidth and naturalHeight isn't quite worth it (because there's no real calculation going on). Let me know if you think it's better to leave it in, and I can add it back 🙂
There was a problem hiding this comment.
Update: I actually just added it back in, as removing it caused the resizer to intermittently fail to display in Safari. I've included imageRef.current?.complete in the dependency array — here I think we want the useMemo to recalculate slightly more frequently than with just the loadedNaturalWidth and loadedNaturalHeight values, but happy to remove it if it winds up not being needed.
I've found the behaviour in Safari to be a bit unreliable in testing, so working out what is and isn't needed has been a bit more challenging than usual 😅
…nstead of local component state
andrewserong
force-pushed
the
fix/image-block-resizer-controls-availability-in-safari-after-switching-alignments
branch
from
573e521
to
832d1c5
Compare
|
Thanks for the suggestion to move away from the
In this screengrab, I've added an external image, and once it's loaded, the resizer controls aren't available. I think this is due to: When inserting an image from an external URL, the To resolve that issue, adding an Let me know if you can think of a better way to handle all this, though, or if I missed something in your approach. Very happy to try alternatives! |
|
@andrewserong I didn't try the remote url scenario - I can't see a way around the CORs failure with my approach. The current change of your's seems to work well in both cases - although I did have one failure still in Safari, so just trying to see if I can replicate that |
|
I couldn't replicate the issue with Safari not showing the resize handles with this patch applied, so not sure what caused that. The only other thought I had was that maybe this extra code isn't needed if the |
|
Thanks for re-testing @glendaviesnz!
That's a good point. I can't think of a clean way to exclude it, either, but 🤞 the
Let me know if you can think of any other edges cases I should test, though! |
I can't think of any additional edge cases that we would need to check. |
|
Also LGTM! 🙇 |
noisysocks
deleted the
fix/image-block-resizer-controls-availability-in-safari-after-switching-alignments
branch
noisysocks
removed
the
Backport to WP 6.7 Beta/RC
…ween alignments (#37210) * Image: Update naturalWidth and naturalHeight to pull from image ref instead of local component state * Re-introduce onLoad setState behaviour to allow fallback while cropping images * Remove useMemo * Re-instate useMemo as removing it caused the resizer to disappear intermittently
Description
Fixes #24587
In Safari, when switching the Image block between not aligned and an alignment, the component appears to re-render however the
imgelement'sonloadcallback is never fired, resulting innaturalWidthandnaturalHeightbeingundefined. This means that the conditional on this line is reached, which means that the resizer isn't rendered.The fix is to update
naturalWidthandnaturalHeightto pull from an image ref (so that the values are always available) and fall back to local component state when not available. This should 🤞 give us the best of both worlds — the real current value is available for Safari, and the fallback local component state value is available when cropping and editing images so that the saving state doesn't unexpectedly fail due toundefinednatural width/height.How has this been tested?
Test in other browsers, and also test cropping / image editing to be sure this doesn't introduce any regressions.
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).