Make "become background" include "expand to fill space" (BL-16117)#7804
Make "become background" include "expand to fill space" (BL-16117)#7804JohnThomson wants to merge 1 commit intomasterfrom
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| theOneCanvasElementManager.setActiveElement( | ||
| bgImageCe, | ||
| ); | ||
| theOneCanvasElementManager.expandImageToFillSpace(); | ||
| }); |
There was a problem hiding this comment.
🚩 Potential race between expandImageToFillSpace and deferred adjustBackgroundImageSize from updateCanvasElementForChangedImage
When haveRealBgImage is true, updateCanvasElementForChangedImage(bgImg) at line 1904 calls adjustBackgroundImageSize with useSizeOfNewImage=true, which sets up a deferred adjustment (~100ms timeout or image load listener) at CanvasElementManager.ts:7276-7312. Then expandImageToFillSpace() runs in the next animation frame (~16ms). If expandImageToFillSpace succeeds first (setting bgCanvasElement to fill the canvas), the deferred adjustment computes imgAspectRatio from the now-filled canvas element dimensions (CanvasElementManager.ts:7317-7319), which equals the container aspect ratio, so it re-sets the same dimensions — effectively a no-op. The cropping adjustment at CanvasElementManager.ts:7424 also computes a scale of ~1, preserving the fill. So the race resolves correctly. If expandImageToFillSpace fails (image not loaded), the deferred adjustment puts the image in 'contain' mode (not 'fill'), meaning the fill intent of this PR wouldn't take effect for the haveRealBgImage case. This is unlikely since the image was previously displayed as an overlay and should be browser-cached.
(Refers to lines 1904-1919)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This seems to be a classic Devin "if something changed you might have a problem". I don't think it's worth addressing.
JohnThomson
left a comment
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson made 1 comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion.
| theOneCanvasElementManager.setActiveElement( | ||
| bgImageCe, | ||
| ); | ||
| theOneCanvasElementManager.expandImageToFillSpace(); | ||
| }); |
There was a problem hiding this comment.
This seems to be a classic Devin "if something changed you might have a problem". I don't think it's worth addressing.
1 participant
This change is