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

Allow all global event handlers to be used as listeners #1244

Merged
merged 7 commits into from
May 19, 2020

Conversation

siku2
Copy link
Member

@siku2 siku2 commented May 17, 2020

Fixes #1232
Please refer to the issue for more information about the problem.

Added event handlers

  • All global event handlers as defined by the Living Standard for HTML.
  • The 3 event handlers oncopy, oncut, and onpaste which can only be applied to HTML elements
  • All experimental event handlers which have at least some browser support.

Removed existing event handlers

  • onmousewheel - deprecated, replaced by onwheel
  • ontouchenter - no longer part of the working draft (see here)

A note about stdweb

stdweb doesn't support a lot of the event handlers because it doesn't have structs representing their event data.
I'm not very experienced with stdweb so I'm unaware if there's another way to support them.
My brief research only uncovered this from another project with the same problem and it doesn't present a solution.

Currently, if a user tries to use an event which isn't implemented the error message looks something like this:

error[E0433]: failed to resolve: could not find `onplay` in `html`
 |
 | <video onplay=self.link.callback(|_| Msg::PlayStarted)>{ ... }</video>
 |        ^^^^^^ could not find `onplay` in `html`

Which, sadly, isn't very descriptive. Perhaps there is a way to improve this?
Using two separate LISTENER_SET declarations wouldn't help in my opinion because the error message for passing a callback to a non-listener attribute isn't very clear either (example).

I also wasn't sure how you're updating yew-stdweb so I didn't touch it.

Copy link
Member

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

Nicely done! Thank you

I'm not very experienced with stdweb so I'm unaware if there's another way to support them.

Since stdweb is not actively maintained, there isn't much we can do at this point. I'm not very interested in forking it since web-sys is a great alternative.

Which, sadly, isn't very descriptive. Perhaps there is a way to improve this?

We could add a std_web (Yew naming convention) cargo feature to the yew-macro crate and enable it in yew-stdweb/Cargo.toml. Then, use that macro to add a stdweb specific check for the unsupported listeners and return a nice syn::Error::new_spanned(..) error.

I also wasn't sure how you're updating yew-stdweb so I didn't touch it.

It's just an alias to yew with different default cargo features

yew-macro/src/html_tree/html_tag/tag_attributes.rs Outdated Show resolved Hide resolved
@siku2
Copy link
Member Author

siku2 commented May 18, 2020

@jstarry

We could add a std_web (Yew naming convention) cargo feature to the yew-macro crate and enable it in yew-stdweb/Cargo.toml. Then, use that macro to add a stdweb specific check for the unsupported listeners and return a nice syn::Error::new_spanned(..) error.

It seems like stdweb is on its way out but I went ahead and implemented it anyway because it's nice to have.
Here's what the error looks like now:

error: the listener `onplay` is only available when using web-sys
 |
 | <video onplay=self.link.callback(|_| Msg::PlayStarted)>{ ... }</video>
 |        ^^^^^^

What do you think?
If you think it's too invasive - it does, after all, require a new feature for the yew-macro crate - I can just remove the commit.


It's just an alias to yew with different default cargo features

Hah, I should've noticed it was just a symlink to Yew's src directory. That's clever!

@jstarry
Copy link
Member

jstarry commented May 19, 2020

It seems like stdweb is on its way out but I went ahead and implemented it anyway because it's nice to have.

Thanks! There are still quite a few Yew devs that are still using stdweb because of the convenience of cargo-web.

Here's what the error looks like now

Beautiful 👍

@jstarry jstarry merged commit 02b22d1 into yewstack:master May 19, 2020
@siku2 siku2 deleted the global-events branch May 19, 2020 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add onerror to yew-macro listeners
2 participants