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 docs about how to get more fine-grained control of event.preventDefault() calls on web #3451

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

ddfisher
Copy link

@ddfisher ddfisher commented Feb 2, 2024

The example is a bit verbose, unfortunately, but I'm hoping it's relatively clear and it's also quite close to being directly useful.

See #2831 for more context.

@ddfisher ddfisher requested a review from daxpedda as a code owner February 2, 2024 20:28
Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

Thank you, this is great!

Let me know if you need any help with anything.

src/platform/web.rs Outdated Show resolved Hide resolved
src/platform/web.rs Outdated Show resolved Hide resolved
src/platform/web.rs Outdated Show resolved Hide resolved
/// event_type,
/// prevent_default_listener.as_ref().unchecked_ref(),
/// );
/// prevent_default_listener.forget();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// prevent_default_listener.forget();
/// prevent_default_listener.into_js_value();

See Closure::into_js_value().

Copy link
Author

Choose a reason for hiding this comment

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

Oh, interesting -- thanks!

Copy link
Member

Choose a reason for hiding this comment

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

This is still missing.

src/platform/web.rs Outdated Show resolved Hide resolved
Comment on lines 79 to 80
/// This example calls `event.preventDefault()` for all relevant events except for ctrl-c,
/// ctrl-x, and ctrl-v.
Copy link
Member

Choose a reason for hiding this comment

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

I think we should pick a different example because I consider the whole preventing clipboard interactions a Winit bug.

E.g. ctrl-p for printing? Maybe also hook up beforeprint just to make the example complete.

Copy link
Author

Choose a reason for hiding this comment

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

Switched to ctrl-p.

Copy link
Member

Choose a reason for hiding this comment

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

Hooking up beforeprint would look something like this:

canvas
    .add_event_listener_with_callback(
        "beforeprint",
        &Closure::<dyn Fn()>::new(|| {
            // This event is now triggered!
        })
        .into_js_value()
        .unchecked_into(),
    )
    .unwrap();

ddfisher and others added 2 commits April 5, 2024 15:49
Co-authored-by: daxpedda <daxpedda@gmail.com>
Co-authored-by: daxpedda <daxpedda@gmail.com>
@madsmtm madsmtm added S - docs Awareness, docs, examples, etc. DS - web labels Jun 24, 2024
@ddfisher
Copy link
Author

Let me know if this looks good now! Sorry for the slow turnaround here!

@@ -80,8 +80,138 @@ pub trait WindowExtWeb {
/// For example, by default using the mouse wheel would cause the page to scroll, enabling this
/// would prevent that.
///
/// Specifically, this will call `event.preventDefault()` for the following event types:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Specifically, this will call `event.preventDefault()` for the following event types:
/// # Implementation Notes
///
/// Specifically, this will call `event.preventDefault()` for the following event types:

Comment on lines +84 to +85
/// `touchstart`, `wheel`, `contextmenu`, `pointerdown`, `pointermove` (for chorded button
/// interactions only), `keyup`, and `keydown`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// `touchstart`, `wheel`, `contextmenu`, `pointerdown`, `pointermove` (for chorded button
/// interactions only), `keyup`, and `keydown`.
/// `touchstart`, `wheel`, `contextmenu`, `pointerdown`, `pointermove` (for
/// [chorded button events only]), `keyup`, and `keydown`.

And link to https://www.w3.org/TR/2019/REC-pointerevents2-20190404/#chorded-button-interactions.

Copy link
Member

Choose a reason for hiding this comment

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

Lets link `event.preventDefault()` to https://developer.mozilla.org/en-US/docs/Web/API/Event/preventDefault as well.

Comment on lines +94 to +95
/// # Example
/// This example calls `event.preventDefault()` for all relevant events except for ctrl-p.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// # Example
/// This example calls `event.preventDefault()` for all relevant events except for ctrl-p.
/// # Example
///
/// This example calls `event.preventDefault()` for all relevant events except for Ctrl-P.
///

/// # }
/// #
/// # impl ApplicationHandler for App {
/// # fn resumed(&mut self, event_loop: &ActiveEventLoop) {
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be moved to can_create_surfaces().
The actual issue of course is that documentation tests are not tested on Web, so I went ahead and fixed that in #3799.

Feel free to rebase this PR on #3799 so you can test it yourself as well.

Comment on lines +118 to +119
/// # window.set_prevent_default(false);
/// # add_prevent_default_listeners(window.canvas().unwrap());
Copy link
Member

Choose a reason for hiding this comment

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

This part is essential to the example, so it should be visible.
I don't think there is anything wrong with showing the whole struct App and impl ApplicationHandler for App.

Comment on lines +130 to +132
/// # WindowEvent::RedrawRequested => {
/// # self.window.as_ref().unwrap().request_redraw();
/// # }
Copy link
Member

Choose a reason for hiding this comment

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

This is probably not a recommended setup, especially for Web.
I think we can get away with only handling WindowEvent::CloseRequested.

/// event.prevent_default();
/// });
///
/// let _ = canvas.add_event_listener_with_callback(
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't ignore the error, just unwrap() here.

/// event_type,
/// prevent_default_listener.as_ref().unchecked_ref(),
/// );
/// prevent_default_listener.forget();
Copy link
Member

Choose a reason for hiding this comment

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

This is still missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DS - web S - docs Awareness, docs, examples, etc.
Development

Successfully merging this pull request may close these issues.

3 participants