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

Properly give focus to canvas #916

Merged
merged 7 commits into from Oct 18, 2017

Conversation

Projects
None yet
2 participants
@CendioOssman
Copy link
Member

commented Oct 6, 2017

Possible fix for #914, #915 and #692. Needs testing.

@samhed
Copy link
Member

left a comment

Nice! Looks good, and I look forward to testing IE and Edge when I get to work on monday!

@CendioOssman

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2017

Need to check how it works on mobile devices where that extra input field is used as well.

@CendioOssman

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2017

Works well with Chrome and Firefox on Pixel C with on screen keyboard. Physical keyboard has some quirks though:

a) Touching the canvas doesn't move focus there (need to add a touchbegin listener)
b) It's not readily apparent that the focus is on input elements

@samhed

This comment has been minimized.

Copy link
Member

commented Oct 9, 2017

Running this makes IE and Edge work well! Can verify that it fixes #692.

One should also note that this PR brings a change in behavior for users. If the user clicks in the settings and then proceeds to type with the keyboard, the input will no longer be sent to the server. The user has to change focus by clicking on the canvas first.

@samhed

samhed approved these changes Oct 9, 2017

@samhed samhed referenced this pull request Oct 12, 2017

Closed

Finalize public API #924

@samhed

This comment has been minimized.

Copy link
Member

commented Oct 13, 2017

You have only seen issues with a physical keyboard on android so far right?

@CendioOssman

This comment has been minimized.

Copy link
Member Author

commented Oct 13, 2017

Yup. But there's also more testing to be done.

I also realised it changed the behaviour of the "touch" input field in that we'll no longer listen to key events on that. IOW it becomes the normal path rather than a fallback.

@samhed samhed added this to the v0.7.0 milestone Oct 16, 2017

@CendioOssman CendioOssman force-pushed the CendioOssman:focus branch from 07b56a1 to 4f904bc Oct 16, 2017

Only grab key events on canvas
Give the canvas proper focus handling. This avoids messy logic that
needs to disable and enable event handling when we want to interact
with other UI elements.

It also makes sure we can properly inhibit the browser from triggering
local actions on key presses.

@CendioOssman CendioOssman force-pushed the CendioOssman:focus branch from 4f904bc to 1b61c87 Oct 18, 2017

CendioOssman added some commits Oct 5, 2016

Add focus state for control bar buttons
The focus can now move to the canvas so it is no longer a source of
confusion. It is also important to indicate that they have focus now
that we actually respect it.
Stop giving host field default focus
It's a field that isn't shown by default and rarely changed.
Include .js for anonymous scripts
It is required for syntax highlighting in at least Firefox' debugger.
Make sure control bar stays visible on Tab
Avoid the deprecated keypress event in favour of the keydown event.
It has the benefit of triggering for all keys, not just those that
produce symbols.
Restore handling of key events for virtual keyboard
We broke handling of keydown/keyup when we moved the focus to the
canvas, as events from our input element would then no longer be
caught when they bubbled up to the document object (where we
previously caught events).

Restore the previous behaviour in a cleaner manner by creating a
second Keyboard object to handle this extra input variant.
Don't let the hidden input field be a tab stop
It's very confusing if you tab between elements and all of a sudden
something hidden gets focus.

@CendioOssman CendioOssman force-pushed the CendioOssman:focus branch from 1b61c87 to 3e093af Oct 18, 2017

@CendioOssman

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2017

Works well on all browsers and platforms now. Even tested physical keyboard on Android and iOS.

@CendioOssman CendioOssman merged commit 3e093af into novnc:master Oct 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@samhed

This comment has been minimized.

Copy link
Member

commented Oct 20, 2017

nice!

@CendioOssman CendioOssman deleted the CendioOssman:focus branch Jul 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.