-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
Conversation
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: |
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? |
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. |
OK, I've found that Firefox broke because we call matplotlib/lib/matplotlib/backends/web_backend/js/mpl.js Lines 617 to 621 in 76a5711
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? |
Did some test here on Safari: if the mouse event receiver is a canvas then commenting out If we want to make canvas_div the receiver while Firefox doesn't need
This may hopefully handle both cases while not introducing overhead to the mouse handler. |
1c3ce65
to
6ab46bf
Compare
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 |
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
6ab46bf
to
5950bad
Compare
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. |
running |
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. |
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 :) |
There was a problem hiding this 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).
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. |
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. |
…095-on-v3.6.x Backport PR #24095 on branch v3.6.x (nb/webagg: Move mouse events to outer canvas div)
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 thediv
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 atabindex
.[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
pytest
passes).flake8-docstrings
and runflake8 --docstring-convention=all
).Documentation
doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).