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.

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

Add web_aspect_ratio example #2209

Merged
merged 2 commits into from
Sep 3, 2022

Conversation

rukai
Copy link
Contributor

@rukai rukai commented Feb 26, 2022

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

Adds a new example that creates a web canvas that fills an aspect-ratio'd div
The main point of this example is to demonstrate the functionality provided by #2074
The example can land before or after, but landing it before will make it easier to test #2074

Another benefit of this example is demonstrating how to create a web canvas that fills up the available space while maintaining the same aspect ratio.
Although this is slightly out of scope of winit I believe it would be extremely useful to provide as figuring out what combination of html elements and css configuration I needed to get this to work was surprisingly difficult for me even though I've been dabbling with web technologies for ages.

Pinging @Liamolucko for feedback as this relates to your PR.

Currently running cargo run-wasm --example web_aspect_ratio will give:
image
With #2074 and running RUSTFLAGS="--cfg=web_sys_unstable_apis" cargo run-wasm --example web_aspect_ratio --features css-size will give:
image

@maroider maroider added DS - web C - waiting on maintainer A maintainer must review this code labels Feb 26, 2022
Cargo.toml Outdated
@@ -34,6 +34,7 @@ mint = { version = "0.5.6", optional = true }
[dev-dependencies]
image = { version = "0.24.0", default-features = false, features = ["png"] }
simple_logger = "2.1.0"
web-sys = { version = "0.3.4", features = ['CanvasRenderingContext2d'] }
Copy link
Contributor

Choose a reason for hiding this comment

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

That's quite an old version of web-sys; why did you pick that one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure where I got that version from...
Its functionally identical but I updated to use the same version as the non dev-dep to make it less confusing.

Comment on lines 64 to 77
let parent_div = document.create_element("div").unwrap();
parent_div
.dyn_ref::<HtmlElement>()
.unwrap()
.style()
.set_css_text("margin: auto; width: 50%; aspect-ratio: 4 / 1;");
body.append_child(&parent_div).unwrap();

// Set a background color for the canvas to make it easier to tell the where the canvas is for debugging purposes.
let canvas = window.canvas();
canvas
.style()
.set_css_text("display: block; width: 100%; height: 100%; background-color: crimson;");
parent_div.append_child(&canvas).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't need to create a parent <div>; applying the CSS directly to the canvas works fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I thanks I didnt realize that.
I think I prefer to have the div separate in my actual application as it means I can have a generic canvas created by wasm that the html template can configure the size of by setting the css on the parent div.

However in this example it probably makes sense to just keep it simple and do all in one considering we cant control the raw html file anyway.
I've changed the PR to remove the div but am happy to revert if someone else prefers the original way.

Comment on lines 32 to 37
// A small default size is used to better demonstrate issues that come from failing to update the size
.with_inner_size(PhysicalSize::new(100, 100))
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a bit misleading to people reading the example, since you almost never want to use with_inner_size when sizing using ResizeObserver; it works in this case since the canvas' style gets overridden below, but that should probably be documented more clearly.

Something like: 'Note: this would normally fix the canvas' width and height to 100px, but since the canvas' style gets overridden below in create_canvas, it just sets the initial framebuffer size.'

@rukai
Copy link
Contributor Author

rukai commented Sep 2, 2022

I've resolved merge conflicts, would appreciate a review.

Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

I'm not sure I understand how it is supposed to look like now, and how it is supposed to look like after the ResizeObserver?

Here's a screenshot of what it looks like to me on Firefox:

screenshot

Cargo.toml Outdated Show resolved Hide resolved
@rukai
Copy link
Contributor Author

rukai commented Sep 3, 2022

I have addressed your feedback and added screenshots of both to the PR description.

Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Thanks, that was helpful! Would you mind noting in the example that the behaviour is not as it should be yet, but will be after the other PR?

@rukai
Copy link
Contributor Author

rukai commented Sep 3, 2022

done!

Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Thanks, that makes it much more understandable

@madsmtm madsmtm merged commit 97d2aaa into rust-windowing:master Sep 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - waiting on maintainer A maintainer must review this code DS - web
Development

Successfully merging this pull request may close these issues.

4 participants