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

onsubmit should be a SubmitEvent, not a FocusEvent #2691

Closed
1 of 3 tasks
WorldSEnder opened this issue May 18, 2022 · 12 comments · Fixed by #2816
Closed
1 of 3 tasks

onsubmit should be a SubmitEvent, not a FocusEvent #2691

WorldSEnder opened this issue May 18, 2022 · 12 comments · Fixed by #2816
Labels

Comments

@WorldSEnder
Copy link
Member

Problem

onsubmit(FocusEvent)

That sounds.. wrong.

Questionnaire

  • I'm interested in fixing this myself but don't know where to start
  • I would like to fix and I have a solution
  • I don't have time to fix this right now, but maybe later
@siku2
Copy link
Member

siku2 commented May 19, 2022

Agreed. I think most of the event types are sourced from the MDN docs, which unfortunately don't appear to very trustworthy in this case.
We should reference a proper source like the HTML spec itself for the event types going forward.

@prockallsyms
Copy link

prockallsyms commented May 23, 2022

Would this be the spec we should reference? Also what would it take to correct this? I'd love to help.

@WorldSEnder
Copy link
Member Author

@Dom-1 oh yeah, that spec should serve fine. Following the links on the Event handler event types also takes one directly to the event type as specified by IDL. For reference, the event types are implemented in

// Unspecialized event type
impl_short! {
onabort
oncancel
oncanplay
oncanplaythrough
onclose
oncuechange
ondurationchange
onemptied
onended
onerror
onformdata // web_sys doesn't have a struct for `FormDataEvent`
oninvalid
onload
onloadeddata
onloadedmetadata
onpause
onplay
onplaying
onratechange
onreset
onresize
onsecuritypolicyviolation
onseeked
onseeking
onselect
onslotchange
onstalled
onsuspend
ontimeupdate
ontoggle
onvolumechange
onwaiting
onchange
oncopy
oncut
onpaste
onpointerlockchange
onpointerlockerror
onselectionchange
onselectstart
onshow
}
// Specialized event type
impl_short! {
onauxclick(MouseEvent)
onclick(MouseEvent)
oncontextmenu(MouseEvent)
ondblclick(MouseEvent)
ondrag(DragEvent)
ondragend(DragEvent)
ondragenter(DragEvent)
ondragexit(DragEvent)
ondragleave(DragEvent)
ondragover(DragEvent)
ondragstart(DragEvent)
ondrop(DragEvent)
onblur(FocusEvent)
onfocus(FocusEvent)
onfocusin(FocusEvent)
onfocusout(FocusEvent)
onkeydown(KeyboardEvent)
onkeypress(KeyboardEvent)
onkeyup(KeyboardEvent)
onloadstart(ProgressEvent)
onprogress(ProgressEvent)
onloadend(ProgressEvent)
onmousedown(MouseEvent)
onmouseenter(MouseEvent)
onmouseleave(MouseEvent)
onmousemove(MouseEvent)
onmouseout(MouseEvent)
onmouseover(MouseEvent)
onmouseup(MouseEvent)
onwheel(WheelEvent)
oninput(InputEvent)
onsubmit(FocusEvent)
onanimationcancel(AnimationEvent)
onanimationend(AnimationEvent)
onanimationiteration(AnimationEvent)
onanimationstart(AnimationEvent)
ongotpointercapture(PointerEvent)
onlostpointercapture(PointerEvent)
onpointercancel(PointerEvent)
onpointerdown(PointerEvent)
onpointerenter(PointerEvent)
onpointerleave(PointerEvent)
onpointermove(PointerEvent)
onpointerout(PointerEvent)
onpointerover(PointerEvent)
onpointerup(PointerEvent)
ontouchcancel(TouchEvent)
ontouchend(TouchEvent)
ontransitioncancel(TransitionEvent)
ontransitionend(TransitionEvent)
ontransitionrun(TransitionEvent)
ontransitionstart(TransitionEvent)
}
macro_rules! impl_passive {
($($action:ident($type:ident))*) => {
impl_action! {
$(
$action($type, true) -> web_sys::$type
=> crate::html::listener::cast_event
)*
}
};
}
// Best used with passive listeners for responsiveness
impl_passive! {
onscroll(Event)
ontouchmove(TouchEvent)
ontouchstart(TouchEvent)
}

@prockallsyms
Copy link

prockallsyms commented May 24, 2022

@WorldSEnder From my understanding, the current state of the issue is that web_sys doesn't provide the entirety of the Event types which we require for our event handlers. Should we consider building those into Yew for the sake of cohesion, or should work be done in web_sys to provide the necessary Event types?

@WorldSEnder
Copy link
Member Author

WorldSEnder commented Jun 2, 2022

@Dom-1 I hope we see a merge and web-sys release including rustwasm/wasm-bindgen#2922 before yew 0.20, otherwise I think the best strategy is to fallback to the unspecific Event for now.

@hamza1311
Copy link
Member

@Dom-1 I hope we see a merge and web-sys release including rustwasm/wasm-bindgen#2922 before yew 0.20, otherwise I think the best strategy is to fallback to the unspecific Event for now.

Seems like it has been merged and released as part of 0.2.81

@SpanishPear
Copy link
Contributor

Any issues with me (attempting) to pick this up? Would be my first non-documentation PR so I won't be as good as y'all 😝

@WorldSEnder
Copy link
Member Author

@Dom-1 are you still on this? I think the prerequisites are in place now

@prockallsyms
Copy link

My apologies, things have picked up in pace at my work and I haven't had the time to commit. If someone else would like to pick it up that's alright with me.

From what I understand, all you have to do is change FocusEvent to SubmitEvent and then include it in the Cargo.toml.

Also I'm not sure if this is the problem of the person sending this PR or perhaps another issue we should open, but the other event types should be checked for correctness.

@WorldSEnder
Copy link
Member Author

@Dom-1 ty for the response, no worries.

@SpanishPear if you want to take it up, go ahead. Double-checking the other types mentioned in the spec would be appreciated but just correcting the SubmitEvent would make a fine PR too.

@hamza1311
Copy link
Member

It seems @SpanishPear isn't taking up on this so I'll take this up

@SpanishPear
Copy link
Contributor

Oh I just hadn't gotten to finishing it 😅 no worries though!

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

Successfully merging a pull request may close this issue.

5 participants