Skip to content

nb/webagg: Move mouse events to outer canvas div #24095

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

Merged
merged 1 commit into from
Oct 31, 2022

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Oct 5, 2022

PR Summary

This fixes the resize grip on WebKit, which is a bit buggy [1, 2]. We additionally need to ignore pointer events on both canvases, even though their z-index puts the div on top.

For mouse events, we no longer prevent the default handler in most browsers, because that will block the resize grip from working now that it is handling events from the same div. Due to the same bugs in WebKit, we do still need to prevent the default handler there, or else any mouse drags (i.e., for pan and zoom), will cause the browser to try and select the canvas.

The mouse position calculation is somewhat simplified as well.

To ensure keyboard events make it to the canvas div, it now has a tabindex.

[1] https://bugs.webkit.org/show_bug.cgi?id=144526
[2] https://bugs.webkit.org/show_bug.cgi?id=181818

This works for me in Chromium, Firefox, and Midori (WebKit-based).

It should fix #24089, but the testing in #23540 didn't pass on macOS, so I'm not totally sure. Would be good if @zhizheng1 could test this.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • [n/a] New features are documented, with examples if plot related.
  • [n/a] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [n/a] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • [n/a] Documentation is sphinx and numpydoc compliant (the docs should build without error).

@QuLogic QuLogic added this to the v3.6.1 milestone Oct 5, 2022
@ghost
Copy link

ghost commented Oct 5, 2022

Hi, I tested on macOS - it still doesn't work on Safari. However in light of your idea I tested a little bit more and this seems to fix it on Safari:
Both canvas and rubberband_canvas have to be "pointer-events: none;" - this is needed for Safari to make the div resizable. Then I changed all event listeners in your commit from canvas to canvas_div (essentially going one layer deeper, letting the div be the receiver of mouse events. Fortunately a closure is used so no other change is needed).
I added ".mpl-canvas{ pointer-events: none; }" to boilerplate.css. After all these I tested briefly and it seemed to be finally working now.

@ghost
Copy link

ghost commented Oct 5, 2022

I added ".mpl-canvas{ pointer-events: none; }" to boilerplate.css.

To be precise: I added this line in the dev tool temporarily just for testing. Adding this line to the actual file backends/web_backend/css/boilerplate.css somehow didn't work (canvas just doesn't have that style). Is there some sort of caching?

@QuLogic
Copy link
Member Author

QuLogic commented Oct 5, 2022

Well, that's interesting, but unfortunately doing so breaks the resize in Firefox because the mouse event handlers steal it from the resize if it's on the same element. I'll have to see if there's some additional setting we can do to work everywhere.

@QuLogic
Copy link
Member Author

QuLogic commented Oct 5, 2022

OK, I've found that Firefox broke because we call preventDefault in the mouse event handler:

/* This prevents the web browser from automatically changing to
* the text insertion cursor when the button is pressed. We want
* to control all of the cursor setting manually through the
* 'cursor' event from matplotlib */
event.preventDefault();

But the comment does not make sense to me as to why we'd want to do that. The cursor does not change to the text insertion cursor on a canvas.

The only reason to prevent default mouse event handling I can think of is for the context menu, but we do that in the 'contextmenu' event now. Can you think of any other reason to keep this?

@ghost
Copy link

ghost commented Oct 6, 2022

OK, I've found that Firefox broke because we call preventDefault in the mouse event handler:

Did some test here on Safari: if the mouse event receiver is a canvas then commenting out preventDefault has no effect, but if it's a div then commenting it out will indeed make the cursor an "I" when pressing the button.

If we want to make canvas_div the receiver while Firefox doesn't need preventDefault, do you think it makes sense if as a last resort we do something like

    function on_mouse_event_closure(name) {
        if(is Firefox) 
            return function (event) { 
                return fig.mouse_event(event, name); 
            };
        else 
            return function (event) {
	        event.preventDefault();
                return fig.mouse_event(event, name);
            };
    }

This may hopefully handle both cases while not introducing overhead to the mouse handler.

@QuLogic QuLogic modified the milestones: v3.6.1, v3.6.2 Oct 6, 2022
@QuLogic QuLogic changed the title nb/webagg: Ignore pointer events on rubberband canvas nb/webagg: Move mouse events to outer canvas div Oct 27, 2022
@QuLogic
Copy link
Member Author

QuLogic commented Oct 27, 2022

After several tries, I've given up on trying to make a universal setup, due to the bugs [1, 2] I've found above in WebKit. So this does end up doing a bit of browser sniffing, but it's just to prevent the selection from appearing over everything in WebKit. If that breaks, everything will still work but with a slightly annoying selection that can be dismissed after pan/zoom.

I confirmed this fixes the problem in Midori (WebKit-based) without breaking Firefox or Chromium. I was able to reproduce the cursor problem in WebKit as well, so that's why I had to do the preventDefault stuff. This also works in #23540.

This fixes the resize grip on WebKit, which is a bit buggy [1, 2]. We
additionally need to ignore pointer events on both canvases, even though
their `z-index` puts the `div` on top.

For mouse events, we no longer prevent the default handler in most
browsers, because that will block the resize grip from working now that
it is handling events from the same `div`. Due to the same bugs in
WebKit, we _do_ still need to prevent the default handler there, or else
any mouse drags (i.e., for pan and zoom), will cause the browser to try
and select the canvas.

The mouse position calculation is somewhat simplified as well.

To ensure keyboard events make it to the canvas `div`, it now has a
`tabindex`.

[1] https://bugs.webkit.org/show_bug.cgi?id=144526
[2] https://bugs.webkit.org/show_bug.cgi?id=181818
@QuLogic
Copy link
Member Author

QuLogic commented Oct 28, 2022

I double-checked on a HiDPI screen today and noticed this broke it, so pushed a change that should work again, and added a comment so that it's clear why that code is there.

@tacaswell
Copy link
Member

running exapmles/user_interfaces/embedding_webagg_sgskip.py this does work, but the handle is not always visible.

@QuLogic
Copy link
Member Author

QuLogic commented Oct 28, 2022

Unfortunately, nothing I can do about that due to the above bugs in WebKit. We might want to consider adding an overlay sort of triangle ourselves, maybe.

@ghost
Copy link

ghost commented Oct 28, 2022

As a user I think I can live with not having the handle (rather than not functioning at all, or having extra overhead just for visual purpose).. Of course, just my $0.01 the decision is yours :)

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

Lets take the fix of it working (but at the cost of the user having to know the feature exists).

@tacaswell
Copy link
Member

As a user I think I can live with not having the handle

Fair, but I almost posted "does not work in safari". But as a matter of completeness I also installed chrome (to check that my check was actually working at all) and when I connected to the same server with that I saw the handle flash in safari.

@ghost
Copy link

ghost commented Oct 28, 2022

the handle flash in safari.

Yes, it's also what I observed- I guess there's no easy way around this than pushing WebKit to fix the bugs.

Besides, I still recall the earlier versions when it worked (pre-3.3.0-ish, using jQuery); but I don't recall or I didn't pay attention to whether the handle and the cursor change worked in Safari back then.

@ksunden ksunden merged commit 638483f into matplotlib:main Oct 31, 2022
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Oct 31, 2022
@QuLogic QuLogic deleted the resize-webkit branch October 31, 2022 23:56
tacaswell added a commit that referenced this pull request Nov 1, 2022
…095-on-v3.6.x

Backport PR #24095 on branch v3.6.x (nb/webagg: Move mouse events to outer canvas div)
@ksunden ksunden mentioned this pull request Feb 20, 2023
6 tasks
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.

[Bug]: Resizing does not work in WebAgg backend in Safari
3 participants