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

Fix default passive option #2111

Merged
merged 1 commit into from
Oct 16, 2021
Merged

Conversation

mc1098
Copy link
Contributor

@mc1098 mc1098 commented Oct 12, 2021

Description

This PR sets the passive option explicitly in order to override any defaults the browsers might have - this is especially needed because with #1542 we attach listeners to the body element which is at the document level.
See this article section about default passive behaviour on some document level events.

Fixes #2110

Checklist

  • I have run cargo make pr-flow
  • I have reviewed my own code
  • I have added tests

@mc1098 mc1098 added the A-yew Area: The main yew crate label Oct 13, 2021
@NickHu
Copy link

NickHu commented Oct 13, 2021

Could we add a way to make the result from Callback::from explicitly non/passive?

@mc1098
Copy link
Contributor Author

mc1098 commented Oct 14, 2021

Callbacks created using From::from will not be passive by default, apart from these:

impl_passive! {
onscroll(Event)
ontouchmove(TouchEvent)
ontouchstart(TouchEvent)
}

We should add a way for users to create a passive Callback for function components, which I guess is your point but I think a factory function would be more clear then another impl of From::<T>::from, I'm not even sure what T would be in that impl.

Regardless, I think adding this should occur in a different PR and that PR should also include any other missing ways of constructing a Callback which is available through Scope.

@NickHu
Copy link

NickHu commented Oct 14, 2021

I am slightly confused by what you said, as it seems as though onwheel callbacks are passive; hence what I described in #2110. Maybe this is unintentional, and after the fix in this PR they won't be.

@mc1098
Copy link
Contributor Author

mc1098 commented Oct 14, 2021

I am slightly confused by what you said, as it seems as though onwheel callbacks are passive; hence what I described in #2110.

Oh sorry! I meant in terms of the defaults yew uses. This PR is really letting yew override whatever default the browser might have - I think we assumed passive would be false unless set otherwise, but this doesn't seem to be the case, which is why we need to explicitly set the flag.

Maybe this is unintentional, and after the fix in this PR they won't be.

So for these three after this PR, they will still default to passive, unless the user explicitly sets the passive flag to false :)

@mc1098
Copy link
Contributor Author

mc1098 commented Oct 15, 2021

@siku2 sanity check please 🙃

Copy link
Member

@siku2 siku2 left a comment

Choose a reason for hiding this comment

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

Weird, my memory tells me that the DOM Standard dictates the passive flag to init to false.
Does this actually fix the issue?

@mc1098
Copy link
Contributor Author

mc1098 commented Oct 16, 2021

Does this actually fix the issue?

It did for me - I used the example provided in #2110 and found that I got errors in both firefox and chrome, then implemented this change and it worked :)

Weird, my memory tells me that the DOM Standard dictates the passive flag to init to false.

"passive (a boolean, initially false)" - Your memory is correct 🙃. This feature seems to contradict what the standard says and this seems to apply to Edge as well, though I didn't test Edge.

@siku2
Copy link
Member

siku2 commented Oct 16, 2021

@mc1098 thanks a lot for checking!
Looks like this patch is definitely necessary then and I don't see a reason not to merge it.

@siku2 siku2 merged commit 98821c4 into yewstack:master Oct 16, 2021
@mc1098 mc1098 deleted the fix-default-passive branch October 16, 2021 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew Area: The main yew crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot call prevent_default on WheelEvents
3 participants