Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
ksunden merged 1 commit intomatplotlib:mainfromQuLogic:resize-webkit
Oct 31, 2022

Conversation

QuLogic
Copy link
Member

@QuLogicQuLogic commentedOct 5, 2022
edited
Loading

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 theirz-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 samediv. Due to the same bugs in WebKit, wedo 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 canvasdiv, 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 shouldfix#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 (andpytest passes).
  • IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

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

@ghost
Copy link

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 commentedOct 5, 2022
edited by ghost
Loading

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
MemberAuthor

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
MemberAuthor

OK, I've found that Firefox broke because we callpreventDefault 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

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

Did some test here on Safari: if the mouse event receiver is a canvas then commenting outpreventDefault 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 needpreventDefault, 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.

@QuLogicQuLogic modified the milestones:v3.6.1,v3.6.2Oct 6, 2022
@QuLogicQuLogic changed the titlenb/webagg: Ignore pointer events on rubberband canvasnb/webagg: Move mouse events to outer canvas divOct 27, 2022
@QuLogic
Copy link
MemberAuthor

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 thepreventDefault stuff. This also works in#23540.

This fixes the resize grip on WebKit, which is a bit buggy [1, 2]. Weadditionally need to ignore pointer events on both canvases, even thoughtheir `z-index` puts the `div` on top.For mouse events, we no longer prevent the default handler in mostbrowsers, because that will block the resize grip from working now thatit is handling events from the same `div`. Due to the same bugs inWebKit, we _do_ still need to prevent the default handler there, or elseany mouse drags (i.e., for pan and zoom), will cause the browser to tryand 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
MemberAuthor

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

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

@QuLogic
Copy link
MemberAuthor

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

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

@tacaswelltacaswell 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

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.

@ksundenksunden merged commit638483f intomatplotlib:mainOct 31, 2022
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull requestOct 31, 2022
@QuLogicQuLogic deleted the resize-webkit branchOctober 31, 2022 23:56
tacaswell added a commit that referenced this pull requestNov 1, 2022
…095-on-v3.6.xBackport PR#24095 on branch v3.6.x (nb/webagg: Move mouse events to outer canvas div)
@ksundenksunden mentioned this pull requestFeb 20, 2023
6 tasks
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@tacaswelltacaswelltacaswell approved these changes

@ksundenksundenksunden approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.6.2
Development

Successfully merging this pull request may close these issues.

[Bug]: Resizing does not work in WebAgg backend in Safari
3 participants
@QuLogic@tacaswell@ksunden

[8]ページ先頭

©2009-2025 Movatter.jp