Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

changes to image adding when using Image in GUI #15260

Merged
merged 1 commit into from
Jul 9, 2024
Merged

changes to image adding when using Image in GUI #15260

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion packages/dev/gui/src/2D/controls/image.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,10 +583,12 @@ export class Image extends Control {
this._domImage = engine.createCanvasImage();
// need to add to enforce rendering
const imgElement = this._domImage as HTMLImageElement;
if (imgElement.style) {
let addedToDom = false;
if (imgElement.style && this._source?.endsWith(".svg")) {
imgElement.style.visibility = "hidden";
imgElement.style.position = "absolute";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to have something like "top: 0" or this will change the page layout and increase height of parent container.

also quality check: ensure these images are not appearing overtop of the canvas. Prepend them to the parent container to ensure they are before the canvas/3d view. Because of native z layering which is determined by ascending order (afaik, from past experience), that will cause them to always be underneath the canvas.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

concern is that there'd be a scrollbar appearing briefly due to this. Not 100% sure if it would be observed by the user, but its highly likely if the onload is an async callback with time to render the page in between.
A screenshot regression test will not reveal the temporary layout change if its only taken at the end. A video would of course.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the add and remove happen on the same frame, synchronous. This should not change user layout

engine.getRenderingCanvas()?.parentNode?.appendChild(imgElement);
addedToDom = true;
}

if (value) {
Expand All @@ -602,10 +604,12 @@ export class Image extends Control {
waitingCallback();
}
cachedData.waitingForLoadCallback.length = 0;
addedToDom && imgElement.remove();
return;
}
}
this._onImageLoaded();
addedToDom && imgElement.remove();
};
if (value) {
Tools.SetCorsBehavior(value, this._domImage);
Expand Down
Loading