-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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.
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
changes to image adding when using Image in GUI #15260
Conversation
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://babylonsnapshots.z22.web.core.windows.net/refs/pull/15260/merge/index.html#WGZLGJ#4600 Links to test babylon tools with this snapshot: https://playground.babylonjs.com/?snapshot=refs/pull/15260/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/15260/merge#BCU1XR#0 |
WebGL2 visualization test reporter: |
Visualization tests for WebGPU (Experimental) |
@@ -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"; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
This changes the behavior that was added to support svg files:
PG for testing: #XCPP9Y#21986