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

Prevent focus loss on mouseout for input elements inside toolbar #5344

Merged
merged 4 commits into from Sep 21, 2018

Conversation

@AlbertHilb
Copy link
Member

@AlbertHilb AlbertHilb commented Sep 20, 2018

This is a refinement of PR #5328.
The code introduced there prevents focus loss caused by mouseup, but not by mouseout.
This fix works in either case.

Cheers

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Sep 20, 2018

I am starting to think that the auto-defocusing of toolbars is more trouble than it is worth. It may also cause some accessibility issues. What if we removed this mouseup/mouseout logic alltogether?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 20, 2018

My concern is that we don't have any visual indication that the toolbar has focus, which is the original reason I added the de-focusing behavior.

@AlbertHilb
Copy link
Member Author

@AlbertHilb AlbertHilb commented Sep 20, 2018

I'm in favor of removing toolbar auto-defocusing.
In my understanding it is essentially intended to avoid clicking on buttons steals focus from main content.
The same can be achieved, for example, binding button action to mousedown event and preventing default.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 20, 2018

Ohh, nice, @AlbertHilb, do you mind updating this PR to add that behavior to ToolbarButton and remove the de-focus logic?

Bind toolbar button action to `mousedown` and prevent default action
to avoid stealing focus from main content.
@AlbertHilb
Copy link
Member Author

@AlbertHilb AlbertHilb commented Sep 20, 2018

Done.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 20, 2018

Nice, thanks! It looks like the apputils toolbar tests need updating.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 20, 2018

You can run just the toolbar tests as jlpm test --pattern=src/toolbar.spec.ts from tests/test-apputils.

@AlbertHilb
Copy link
Member Author

@AlbertHilb AlbertHilb commented Sep 20, 2018

I only updated non-commented-out tests. Is it ok?

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Sep 20, 2018

@AlbertHilb
Copy link
Member Author

@AlbertHilb AlbertHilb commented Sep 20, 2018

I noted also notebook has failing tests. I'm going to update those too.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Sep 20, 2018

Love to see all that deleted code!

@AlbertHilb
Copy link
Member Author

@AlbertHilb AlbertHilb commented Sep 21, 2018

Tests updated.

@blink1073 blink1073 removed this from the 1.0 milestone Sep 21, 2018
@blink1073 blink1073 added this to the 0.35 milestone Sep 21, 2018
@blink1073
Copy link
Member

@blink1073 blink1073 commented Sep 21, 2018

Thanks!

@blink1073 blink1073 merged commit 30fd58f into jupyterlab:master Sep 21, 2018
1 of 2 checks passed
@AlbertHilb AlbertHilb deleted the PreventFocusLoss2 branch Sep 21, 2018
@blink1073 blink1073 mentioned this pull request Sep 28, 2018
31 tasks
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants