Web support via stdweb and web-sys #26

ryanisaacg · 2019-09-03T20:46:52Z

Old PR:
This will probably not be the final change, because it:

A) will probably require bikeshedding as to the proper way to choose between stdweb / web-sys at compile time
B) does not implement 'empty'
C) most importantly, removes 'Copy' from the public API of the RawWindowHandle

Some aspect of web support does need to make it into raw-window-handle for web support in winit, but I wanted to put up this PR to start discussion on what that should look like.

Edit: For more context, the Canvas objects in stdweb and web-sys are Clone but not copy, so a struct containing them may not be Copy either. I'm not sure of the best-practice way to work around this.

New PR:

A backend-agnostic numerical ID for web has been added as an option to the window handle enum, with documentation referring users to custom data attributes on HTML elements.

Osspial

Thanks for the PR! I've added comments where I think changes should be considered, but I'm excited to get support for this.

Osspial · 2019-09-04T19:43:13Z

src/lib.rs

@@ -46,7 +46,10 @@ pub mod windows;
 #[cfg_attr(feature = "nightly-docs", doc(cfg(target_os = "ios")))]
 #[cfg_attr(not(feature = "nightly-docs"), cfg(target_os = "ios"))]
 pub mod ios;
-// pub mod wasm;
+


There shouldn't be a line break here for consistency reasons.

Osspial · 2019-09-04T19:49:15Z

src/web.rs

+    #[cfg(feature = "web-sys")]
+    pub canvas: web_sys::HtmlCanvasElement,
+    #[cfg(feature = "stdweb")]
+    pub canvas: stdweb::web::html_element::CanvasElement,


I really don't like this design for conditionally selecting the canvas type. APIs that behave differently should look different, and overloading the canvas name will make this type significantly harder to use, understand, and document.

Ideally we can follow the same approach we use for selecting the X11/Wayland backend: have different variants in the top-level enum for each backend (assuming we don't switch the HTMLCanvasElement representation we're exposing). That being said, I assume there's a reason you made this design choice over the one I just outlined - is there any technical reason why that's a bad idea?

My reasoning was 'Winit and X11 are both valid choices in the same binary, but stdweb and web-sys are not.' With that in mind, if they should still be separate I have no problem separating them.

I sympathize but since they're not actually the same API, as a user I would prefer that they be entirely different things in the struct.

src/web.rs

Osspial · 2019-09-04T20:01:18Z

Cargo.toml

+[target.'cfg(target_arch = "wasm32")'.dependencies.stdweb]
+version = "0.4.18"
+optional = true
+


I'm not entirely comfortable with re-exporting these types in our public API, since it opens us up for breaking changes if the ~~down~~upstream crates ever bump their major version.

Looking at the stdweb source code, it looks like the Canvas struct just wraps around an i32 that specifies the HTMLCanvasElement JS object. Could we expose that i32 instead of the higher-level wrappers, than have downstream users reconstruct the object from that integer?

I'll try to look into ways to remove the stdweb and web-sys types from the public API, which should also possibly help with my problem that they aren't 'Copy'.

Would using Box::into_raw and Box::from_raw on the canvas objects make sense as a design? Then the handle can just be a void*.

The downside is a serious loss of safety, but the upside is that it's more in line with the rest of the API.

@ryanisaacg I'm not entirely comfortable with doing that, since it seems like an unnecessary layer of indirection over exposing the JS reference integer directly.

stdweb's Reference type exposes unsafe from_raw_unchecked methods for internal use. Could we see about exposing those methods in the public API? From a light investigation it seems the main reason they aren't exposed is because the authors didn't think there was much reason to expose them (based on koute/stdweb#339), but I think we've got an acceptable reason here.

Ok, I'll propose upstream methods to stdweb and wasm-bindgen to have unsafe methods for converting back and forth between the integer ID's.

Setting ourselves up for either a breaking change or forcing our users to use legacy code would fundamentally undermine the stability and common ground that this library is supposed to provide, and as such is an unacceptable solution.

In the thread you linked, there's a point in @alexcrichton's comment here (rustwasm/wasm-bindgen#999 (comment)) which strikes me as strange (emphasis mine):

I think you're right that taking advantage of future wasm feature "in their fullest" is likely a breaking change, but what I was hoping for is that it's a largely cosmetic breaking change rather than "rewrite the world" breaking change. For example switching some types around is pretty easy, but losing a fundamental capability (like "O(1) conversion to an integer from JsValue") runs the risk of making it much harder to migrate in the future.

Wouldn't changing an unsafe JsValue::from_raw(i32) method into JsValue::from_raw(anyref) just be a cosmetic breaking change? You aren't losing the ability to convert arbitrary integers into JsValue, because such an API would never provide that guarantee to begin with. Unsafe code can require additional semantics for proper use, and as long as a from_raw(i32) method requires handling the raw i32 similarly to how you'd handle an anyref variable you shouldn't run into any major compatibility issues when you switch the types over.

Well, let's think about this. The whole point of this is to expose window handle stuff as primitive types that are NOT bound to a particular version of a library. And the problem is that JsValue's are not primitive types, quite, or rather wasm-bindgen doesn't want to be able to treat them as such because the fact that they're an i32 under the hood is an implementation detail that quite realistically may change. So, if we DID expose wasm-bindgen's JsValue as integers... it would be subject to breaking changes anyway.

So, we can't expose the raw types under JsValue because it wouldn't be stable, any more than we can expose the layout of whatever structure a HWND actually points at. One solution is that raw-window-handle could expose handles based on the version of wasm-bindgen, such as WasmHandle02(JsValue), WasmHandle03(JsValue), and so on, and use feature flags to offer the right ones at compile time. It would work. But it would rather suck.

I brought this up for discussion again here: rustwasm/wasm-bindgen#1766

As discussed on the Rust Community discord, if we're willing to limit ourselves to canvases that exist within the DOM, we can provide some sort of UUID that windowing libraries attach to canvas elements. Then, clients can find the canvas via the UUID.

Upsides:

We're not tied to stdweb or web-sys under the hood

Downsides:

Requires a bit more work on behalf of the windowing libraries (probably not too bad)

Does not account for canvases not inserted into the DOM (probably not important, because this library is for interop from windowing libraries)

Would this work for everyone?

I would be okay with that.

It's kinda a hack IMO, but it's a hack that should work and make it possible to use right now without needing to wait for someone else to figure their stuff out. Which is partially the point of this library.

ryanisaacg · 2019-09-23T18:34:39Z

Ok, I've pushed up a change with a new design that doesn't tie us into the API of web-sys or stdweb.

Osspial · 2019-09-24T15:21:10Z

src/web.rs

+pub struct WebHandle {
+    /// An ID value inserted into the data attributes of the canvas element
+    ///
+    /// Each canvas created by the windowing system shoudl be assigned their own unique ID.


Osspial · 2019-09-24T15:21:33Z

src/web.rs

+/// ```
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub struct WebHandle {
+    /// An ID value inserted into the data attributes of the canvas element


This doesn't specify the name of the canvas attribute.

Good point, fixed

grovesNL

Thanks! Looks good to me after the fact 👍

ryanisaacg changed the title ~~Do the initial work for web support~~ Web support via stdweb and web-sys Sep 3, 2019

Osspial suggested changes Sep 4, 2019

View reviewed changes

ryanisaacg mentioned this pull request Sep 7, 2019

Web support tracking issue rust-windowing/winit#1072

Closed

10 tasks

ryanisaacg and others added 2 commits September 23, 2019 14:29

Add a web module based on a numerical ID system

6d1ad49

Merge branch 'master' into add-web-support

d748710

Cargo fmt

c747aa9

Osspial suggested changes Sep 24, 2019

View reviewed changes

ryanisaacg and others added 3 commits September 24, 2019 12:26

Document a name to use as the data attribute

c366778

Clarify docs

9e2beb5

Fix typo

c037d5a

Osspial merged commit 3bb8d19 into rust-windowing:master Sep 26, 2019

grovesNL reviewed Sep 26, 2019

View reviewed changes

parasyte mentioned this pull request Oct 1, 2022

Canvas ID not unique on a web page rust-windowing/winit#2259

Closed

grovesNL mentioned this pull request Oct 2, 2022

Feature for web_sys::HtmlCanvasElement or web_sys::OffscreenCanvas #102

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Web support via stdweb and web-sys #26

Web support via stdweb and web-sys #26

ryanisaacg commented Sep 3, 2019 •

edited

Loading

Osspial left a comment

Osspial Sep 4, 2019

Osspial Sep 4, 2019 •

edited

Loading

ryanisaacg Sep 5, 2019

icefoxen Sep 12, 2019

Osspial Sep 4, 2019 •

edited

Loading

ryanisaacg Sep 5, 2019

ryanisaacg Sep 5, 2019

Osspial Sep 5, 2019

ryanisaacg Sep 5, 2019

Osspial Sep 10, 2019

icefoxen Sep 12, 2019

ryanisaacg Sep 18, 2019

Osspial Sep 18, 2019

icefoxen Sep 19, 2019

ryanisaacg commented Sep 23, 2019

Osspial Sep 24, 2019

Osspial Sep 24, 2019

ryanisaacg Sep 24, 2019

grovesNL left a comment

Web support via stdweb and web-sys #26

Web support via stdweb and web-sys #26

Conversation

ryanisaacg commented Sep 3, 2019 • edited Loading

Osspial left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Osspial Sep 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Osspial Sep 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanisaacg commented Sep 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grovesNL left a comment

Choose a reason for hiding this comment

ryanisaacg commented Sep 3, 2019 •

edited

Loading

Osspial Sep 4, 2019 •

edited

Loading

Osspial Sep 4, 2019 •

edited

Loading