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 onerror to yew-macro listeners #1232

Closed
siku2 opened this issue May 14, 2020 · 6 comments · Fixed by #1244
Closed

Add onerror to yew-macro listeners #1232

siku2 opened this issue May 14, 2020 · 6 comments · Fixed by #1244
Labels
feature-request A feature request

Comments

@siku2
Copy link
Member

siku2 commented May 14, 2020

Is your feature request related to a problem? Please describe.
I was implementing an image component which falls back to a different image when it encounters an error while loading the primary one. I wanted to do this using the onerror event handler but it turns out that Yew doesn't support it.
The event is not listed in LISTENER_SET which makes for an interesting error message as Yew treats it like a normal attribute.

Describe the solution you'd like
I would love to see support for all global event handlers so that it's possible to use the onerror event handler.

Describe alternatives you've considered
I tried to manually construct the img VTag but that turned out to be quite annoying because there's no Wrapper implementing the Listener trait generated for it.
Instead I decided to use a NodeRef and manually call set_onerror on it. This is really verbose though so I think it's less than ideal.

@jstarry
Copy link
Member

jstarry commented May 16, 2020

@siku2 thanks for the feature request, you're the third person with this issue in the past month! I would also love to see support for all global event handlers. Agreed that the alternative you described is not ideal.

which makes for an interesting error message as Yew treats it like a normal attribute.

Was this confusing enough that we should address this and try to make the error message more dev friendly?

@siku2
Copy link
Member Author

siku2 commented May 16, 2020

@jstarry apologies for the duplicate issue. I was unable to find the other two issues using a simple keyword search.

The error reads as follows:

error[E0599]: no method named `to_string` found for enum `yew::callback::Callback<_>` in the current scope
   |
   |    <img onerror=self.link.callback(|_| {})/>
   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^ method not found in `yew::callback::Callback<_>`
   | 
   |
14 | pub enum Callback<IN> {
   | ---------------------
   | |
   | doesn't satisfy `yew::callback::Callback<_>: std::fmt::Display`
   | doesn't satisfy `yew::callback::Callback<_>: std::string::ToString`
   |
   = note: the method `to_string` exists but the following trait bounds were not satisfied:
           `yew::callback::Callback<_>: std::fmt::Display`
           which is required by `yew::callback::Callback<_>: std::string::ToString`
   = note: this error originates in a macro

Ideally there should be a note indicating that I'm passing a Callback to a normal attribute.

@siku2
Copy link
Member Author

siku2 commented May 16, 2020

@jstarry I would love to work on this but I feel like there are few things that need to be discussed first.

There are a lot of events which are specific to a particular tag. The biggest offender here are the media events. Even though some of these events are only applicable to a specific tag and some don't even bubble they can still be applied to any tag according to the spec (At least I think).
Since Yew doesn't distinguish between the various tags I think it's alright to allow these handlers everywhere, right?

There are also a lot of experimental and deprecated event handlers. One of the deprecated ones, onmousewheel, is actually supported by Yew (see here) while the replacement onwheel isn't yet.
How should we go about this?
Personally I think Yew should support all of them. If anything Yew could emit a warning for deprecated or unstable handlers but that's a different matter.

@jstarry
Copy link
Member

jstarry commented May 17, 2020

I would love to work on this but I feel like there are few things that need to be discussed first.

Excellent! We'd love to have your help!

Since Yew doesn't distinguish between the various tags I think it's alright to allow these handlers everywhere, right?

Yew can distinguish between various tags during the macro expansion phase if using the html! macro. Here's an example: #1217

There are also a lot of experimental and deprecated event handlers

This is a tough part of web development 🤕

One of the deprecated ones, onmousewheel, is actually supported by Yew (see here) while the replacement onwheel isn't yet.
How should we go about this?

Since onwheel is standardized, I think we should completely remove onmousewheel in favour of onwheel. I don't think there's any reason to support onmousewheel because according to caniuse.com, any browser version that supports it, also supports onwheel.

Personally I think Yew should support all of them

I mostly agree but I think there is good reason to have at least a little discretion. For instance, with onwheel vs onmousewheel if you use onmousewheel, it's almost certainly a mistake.

If there's ever ambiguity, we can err on the side of adding support.

@siku2
Copy link
Member Author

siku2 commented May 17, 2020

Thanks for taking the time to respond, @jstarry.

Here's an idea how to decide on which handlers to support:
Support all the event handlers dictated by the current HTML living standard and additionally support all experimental handlers that have at least one major browser supporting them.

Here are a few examples:

  • onmousewheel wouldn't be supported as it is no longer part of the HTML spec
  • ontouch events are supported even though they're still a draft because they're already supported by some browsers.
  • onsort isn't supported because no browser implements it

What do you think about this?


Regarding my second problem; whether or not to allow all event handlers on all tags.
I consulted the HTML Living Standard specification to confirm my previous statement

they can still be applied to any tag according to the spec

And the spec does indeed say "[...] must be supported by all HTML elements, as both event handler content attributes and event handler IDL attributes".
So it seems that the following is perfectly valid:

<span onplay="() => console.log('Hello World!')">
  Definitely not a video
</span>

I don't think it's necessary to spend an absurd amount of time researching which event handlers can be applied to which tags if it actually goes against the spec.

While reading through the spec I did however find out that there are two other groups of event handlers:

Since it's impossible to construct a window object using the html! macro I think it's fair to ignore the WindowEventHandlers entirely.
The 3 DocumentAndElementEventHandlers on the other hand can just be included with the other event handlers without and special handling unless you think it's necessary that I create a second group?

@jstarry
Copy link
Member

jstarry commented May 17, 2020

Support all the event handlers dictated by the current HTML living standard and additionally support all experimental handlers that have at least one major browser supporting them.

Sounds great to me

I don't think it's necessary to spend an absurd amount of time researching which event handlers can be applied to which tags if it actually goes against the spec.

Got it, I agree with this.

The 3 DocumentAndElementEventHandlers on the other hand can just be included with the other event handlers without and special handling unless you think it's necessary that I create a second group?

Agreed, that they can be included with the other event handlers 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants