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

Toolbar buttons fire on right-click down #6691

Closed
vidartf opened this issue Jun 22, 2019 · 10 comments · Fixed by #6692
Closed

Toolbar buttons fire on right-click down #6691

vidartf opened this issue Jun 22, 2019 · 10 comments · Fixed by #6692
Labels
status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.

Comments

@vidartf
Copy link
Member

vidartf commented Jun 22, 2019

When using the toolbar buttons, I notice that they fire their actions on:

  • Right-click events (possibly also other mouse buttons?)
  • Mouse down

Both of which I would consider unexpected, and annoying (right click should give context menu, and I should be able to abort my action mid-click by dragging the mouse off the button before releasing).

@jasongrout
Copy link
Contributor

Agreed about right click. We investigated the mousedown vs mouseup behavior, and it turns out that lots of buttons in toolbars actually take action on mousedown.

@gnestor
Copy link
Contributor

gnestor commented Jun 23, 2019

Reference: https://github.com/jupyterlab/jupyterlab/blob/master/packages/apputils/src/toolbar.tsx#L462

Is there any reason why we can't use mouseup?

@vidartf
Copy link
Member Author

vidartf commented Jun 23, 2019

After Jason's comment, I remember there having been a discussion about this (with AlbertHilb I think). If somebody knows where that was and can crossref it, that would be appreciated :)

@gnestor
Copy link
Contributor

gnestor commented Jun 24, 2019

Related issues:
#6077
#5757

Related PRs:
#5344
#6083

@gnestor
Copy link
Contributor

gnestor commented Jun 24, 2019

@AlbertHilb To fix this, we could use mouseup vs. mousedown. Do you have any objections to that?

@jasongrout
Copy link
Contributor

There are ux questions around using mouse up vs mouse down. @ellisonbg and I looked at a few existing patterns, and there was definitely precedence for mouse down.

@AlbertHilb
Copy link
Contributor

The default browser action on mousedown event is to move the focus to the event target (in our case to the button). On the other side, preventing the default action on mousedown stops click and mouseup events from firing. So just binding action to mouseup, without other adjustments, causes the editor to blur when buttons are pressed.

@vidartf
Copy link
Member Author

vidartf commented Jun 25, 2019

@AlbertHilb Thanks for the detailed explanation. Losing editor focus seems like a good enough motivation for me. Maybe we should include a comment next to the event binding in the code to prevent this from coming up again 😅

@ellisonbg
Copy link
Contributor

We have wrestled previously with actions (toolbar buttons, menu items) that blur editor focus. Interacting with elements outside the editor should in fact blur the editor as those other elements should receive focus. Disrupting that will only cause accessibility problems and weird side effects.

A separate question is whether we should trigger on up/down. I don't remember off hand which UI toolkits handle it which way, but it was not at all consistent between them, or between keyboard and mouse.

@lock
Copy link

lock bot commented Aug 6, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related discussion.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019
@jasongrout jasongrout added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
5 participants