Fix fit_canvas_to_parent #11278

Vrixyz · 2024-01-09T22:45:19Z

Follow up to #11057

Implemented suggestions from reviewers from: a simpler fit_canvas_to_parent leads to an explicit CSS setting to the canvas.

From my understanding, it has do be set after wgpu creation due to wgpu overriding the canvas width/height: https://github.com/gfx-rs/wgpu/blob/4400a5847080d1164bdca93a90622414963ed9f3/examples/src/utils.rs#L68-L74

Changelog

Re-enable a fit_canvas_to_parent, it's removal from Remove CanvasParentResizePlugin #11057 was problematic. Still, its inner working is more simple than before: bevy doesn't handle its resizing, winit does.

Migration Guide

Cancels the migration from Remove CanvasParentResizePlugin #11057

Vrixyz · 2024-01-09T22:53:13Z

crates/bevy_window/src/window.rs

+    /// **Warning**: this will not behave as expected for parents that set their size according to the size of their
+    /// children. This creates a "feedback loop" that will result in the canvas growing on each resize. When using this
+    /// feature, ensure the parent's size is not affected by its children.


I'm not too sure if this comment is still 100% correct

crates/bevy_winit/src/system.rs

Vrixyz · 2024-01-09T23:23:38Z

crates/bevy_winit/src/system.rs

+                    let canvas: HtmlCanvasElement = web_sys::window()
+                        .unwrap()
+                        .document()
+                        .unwrap()
+                        .query_selector(&format!("canvas[data-raw-handle=\"{}\"]", handle.id))
+                        .unwrap()
+                        .unwrap()
+                        .unchecked_into();


Is that too many unwraps ? We could use expects like https://github.com/gfx-rs/wgpu/blob/9f82504d8a3b6e234599b19cc35c299bcec8b138/wgpu-hal/src/gles/web.rs#L117

Fortunately you don't need this whole block (and the corresponding crate features in web-sys), you can just use WindowExtWebSys::canvas(), which only needs one unwrap() that checks if you are on the main thread, which Bevy always is.

daxpedda · 2024-01-10T08:36:48Z

tools/example-showcase/window-settings-wasm.patch

             transparent: false,
             focused: true,
             window_level: Default::default(),
+-            fit_canvas_to_parent: false,
+            fit_canvas_to_parent: true,
             prevent_default_event_handling: true,
 -            canvas: None,
 +            canvas: Some("#bevy".to_string()),


This will also not be needed if you use WindowExtWebSys::canvas().

Thanks for the insight!

You may be right about this specific example, but I think it's still ok to use that as a recommended way to use bevy, so the ability to target a specific canvas is in bevy user space opposed to the lower level winit functions.

For clarity, this "canvas" selector allows to declare the canvas at a specific place rather than relying on winit creating it

Vrixyz · 2024-01-10T09:35:07Z

examples/window/window_settings.rs

+                    // Tells wasm to resize the window according to the available canvas
+                    fit_canvas_to_parent: true,


Suggested change

// Tells wasm to resize the window according to the available canvas

fit_canvas_to_parent: true,

// Tells wasm to set the CSS width and height properties to 100% for the target canvas.

// This can be useful to support window resizing.

fit_canvas_to_parent: true,

Maybe an improvement ?

irate-devil · 2024-01-10T09:56:24Z

From my understanding, it has do be set after wgpu creation due to wgpu overriding the canvas width/height: gfx-rs/wgpu@4400a58/examples/src/utils.rs#L68-L74

No, wgpu only sets the html attributes width and height which are different from width and height inside the style attribute.
It's winit that overrides the style attribute's width and height whenever you request a resolution so this PR won't work.

I'd strongly prefer it if we could avoid having either winit or bevy modify the canvas style at all. That way, styling on the web page would just work without having to use !important.

edit: Commenting out the calls to with_inner_size stops winit from overriding the html styling.

Vrixyz · 2024-01-10T11:00:09Z

I tested this PR and it works on my machine ™️ (firefox/windows):

This PR

On main

Thanks for correcting me on my assumption that wgpu sets the CSS! I want to believe you're right and winit does it, still I'm not sure where ?
The code you linked is from an example, so although related to our analysis, it's not the root cause of the CSS change on main.

It's winit that overrides the style attribute's width and height whenever you request a resolution

Does it override the style ? I don't think that's the case since winit 0.29, from this PR, when I resize the window, only the canvas attribute width/height changes, not the style.

I'm not sure resizing is what you were referring to with "request a resolution", maybe scale factor is a risk too ?

If you mean user explicitly using API to set the resolution, I think it's intentional enough a user wouldn't be surprised the CSS is "modified" (but still could use docs, and support to set it to % rather than pixels).

In other case, if CSS gets overridden implicitly after window creation, isn't it a bug 🤔 ? I think it shouldn't even be modified during window creation but I'm not sure.

daxpedda · 2024-01-10T11:14:39Z

Winit doesn't touch width and height CSS properties unless you tell it to, e.g. WindowBuilder::with_inner_size() and Window::request_inner_size(). Even WindowEvent::ScaleFactorChanged doesn't do anything unless you actually modify the inner_size_writer. Everything else would be a bug.

Please let me know if Winit has any issue on that front, but from what I'm reading it seems to work correctly.

irate-devil · 2024-01-10T11:28:35Z

winit is only adding the style attributes when explicitly requested in my testing, so that's all good.

If you mean user explicitly using API to set the resolution, I think it's intentional enough a user wouldn't be surprised the CSS is "modified" (but still could use docs, and support to set it to % rather than pixels.

Yes, that's what I was referring to. Allowing the resolution to be a percentage sounds like a better alternative to fit_canvas_to_parent, but that would still always override user styling on the page.

Vrixyz · 2024-01-10T11:34:22Z

Thanks for the precision!

So our culprit is https://github.com/Vrixyz/bevy/blob/f168ae0451e693232591e0b72ddb78726dfcb22c/crates/bevy_winit/src/winit_windows.rs#L85 ; not too far from my change.

There's a bunch of called to Window::request_inner_size() which we might want to audit to see if we need to apply that CSS change 🤔, not sure of their behaviour in wasm:

when the window mode changes: https://github.com/Vrixyz/bevy/blob/f168ae0451e693232591e0b72ddb78726dfcb22c/crates/bevy_winit/src/system.rs#L177
when scale factor changes: https://github.com/Vrixyz/bevy/blob/f168ae0451e693232591e0b72ddb78726dfcb22c/crates/bevy_winit/src/lib.rs#L532

No bug within winit from my understanding so far :)

alice-i-cecile · 2024-02-29T16:13:52Z

@Vrixyz can you resolve merge conflicts so I can merge this in?

Vrixyz · 2024-02-29T16:57:30Z

I can do that ~sunday

Implemented suggestions from reviewers: a simpler fit_canvas_to_parent leads to an explicit CSS setting to the canvas. It has do be set after wgpu creation due to wgpu overriding the canvas width/height: https://github.com/gfx-rs/wgpu/blob/4400a5847080d1164bdca93a90622414963ed9f3/examples/src/utils.rs#L68-L74

For reference on wgpu: https://github.com/gfx-rs/wgpu/blob/9f82504d8a3b6e234599b19cc35c299bcec8b138/wgpu-hal/src/gles/web.rs#L117

Follow up to bevyengine#11057 Implemented suggestions from reviewers from: a simpler fit_canvas_to_parent leads to an explicit CSS setting to the canvas. From my understanding, it has do be set after wgpu creation due to wgpu overriding the canvas width/height: https://github.com/gfx-rs/wgpu/blob/4400a5847080d1164bdca93a90622414963ed9f3/examples/src/utils.rs#L68-L74 # Changelog - Re-enable a `fit_canvas_to_parent`, it's removal from bevyengine#11057 was problematic. Still, its inner working is more simple than before: bevy doesn't handle its resizing, winit does. ## Migration Guide - Cancels the migration from bevyengine#11057

Vrixyz changed the title ~~Follow up to #11057 ; fix fit_canvas_to_parent~~ Fix fit_canvas_to_parent Jan 9, 2024

Vrixyz added C-Bug An unexpected or incorrect behavior A-Windowing Platform-agnostic interface layer to run your app in labels Jan 9, 2024

alice-i-cecile added the O-Web Specific to web (WASM) builds label Jan 9, 2024

alice-i-cecile added this to the 0.13 milestone Jan 9, 2024

Vrixyz force-pushed the fix-fit_canvas_to_parent branch from c37cb7e to f0dd8b1 Compare January 9, 2024 22:48

Vrixyz commented Jan 9, 2024

View reviewed changes

crates/bevy_winit/src/system.rs Outdated Show resolved Hide resolved

Vrixyz commented Jan 9, 2024

View reviewed changes

daxpedda reviewed Jan 10, 2024

View reviewed changes

Vrixyz force-pushed the fix-fit_canvas_to_parent branch 3 times, most recently from 2a0b14e to f168ae0 Compare January 10, 2024 09:25

Vrixyz commented Jan 10, 2024

View reviewed changes

Vrixyz mentioned this pull request Jan 10, 2024

Winit 0.29 follow up #11052

Open

23 tasks

alice-i-cecile removed this from the 0.13 milestone Jan 24, 2024

hymm added this to the 0.14 milestone Feb 11, 2024

MeoMix mentioned this pull request Feb 23, 2024

Remove CanvasParentResizePlugin #11057

Merged

alice-i-cecile approved these changes Feb 23, 2024

View reviewed changes

NiklasEi approved these changes Feb 29, 2024

View reviewed changes

NiklasEi added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 29, 2024

NiklasEi mentioned this pull request Feb 29, 2024

Control canvas size thorugh game-container css class NiklasEi/bevy_game_template#104

Open

NiklasEi mentioned this pull request Feb 29, 2024

Fullscreen CSS tweak NiklasEi/bevy_game_template#102

Closed

Vrixyz added 3 commits March 3, 2024 14:58

use a better query selector, akin to wgpu

3925c22

For reference on wgpu: https://github.com/gfx-rs/wgpu/blob/9f82504d8a3b6e234599b19cc35c299bcec8b138/wgpu-hal/src/gles/web.rs#L117

use winit window::canvas() function

106b8d3

Vrixyz force-pushed the fix-fit_canvas_to_parent branch from f168ae0 to 106b8d3 Compare March 3, 2024 13:59

alice-i-cecile added this pull request to the merge queue Mar 3, 2024

Merged via the queue into bevyengine:main with commit 8cf5fbb Mar 3, 2024
29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix fit_canvas_to_parent #11278

Fix fit_canvas_to_parent #11278

Vrixyz commented Jan 9, 2024 •

edited

Vrixyz Jan 9, 2024

Vrixyz Jan 9, 2024

daxpedda Jan 10, 2024

daxpedda Jan 10, 2024

Vrixyz Jan 10, 2024 •

edited

Vrixyz Jan 10, 2024

Vrixyz Jan 10, 2024

irate-devil commented Jan 10, 2024 •

edited

Vrixyz commented Jan 10, 2024 •

edited

daxpedda commented Jan 10, 2024

irate-devil commented Jan 10, 2024

Vrixyz commented Jan 10, 2024

alice-i-cecile commented Feb 29, 2024

Vrixyz commented Feb 29, 2024

		// Tells wasm to resize the window according to the available canvas
		fit_canvas_to_parent: true,

Fix fit_canvas_to_parent #11278

Fix fit_canvas_to_parent #11278

Conversation

Vrixyz commented Jan 9, 2024 • edited

Changelog

Migration Guide

Vrixyz Jan 9, 2024

Choose a reason for hiding this comment

Vrixyz Jan 9, 2024

Choose a reason for hiding this comment

daxpedda Jan 10, 2024

Choose a reason for hiding this comment

daxpedda Jan 10, 2024

Choose a reason for hiding this comment

Vrixyz Jan 10, 2024 • edited

Choose a reason for hiding this comment

Vrixyz Jan 10, 2024

Choose a reason for hiding this comment

Vrixyz Jan 10, 2024

Choose a reason for hiding this comment

irate-devil commented Jan 10, 2024 • edited

Vrixyz commented Jan 10, 2024 • edited

daxpedda commented Jan 10, 2024

irate-devil commented Jan 10, 2024

Vrixyz commented Jan 10, 2024

alice-i-cecile commented Feb 29, 2024

Vrixyz commented Feb 29, 2024

Vrixyz commented Jan 9, 2024 •

edited

Vrixyz Jan 10, 2024 •

edited

irate-devil commented Jan 10, 2024 •

edited

Vrixyz commented Jan 10, 2024 •

edited