Bug 4865 - rewrite/clean up noVNC's keyboard handling
Summary: rewrite/clean up noVNC's keyboard handling
Status: CLOSED FIXED
Alias: None
Product: ThinLinc
Classification: Unclassified
Component: Web Access (show other bugs)
Version: trunk
Hardware: PC Unknown
: P2 Normal
Target Milestone: 4.2.0
Assignee: Samuel Mannehed
URL:
Keywords: hean01_tester, relnotes
: 5079 (view as bug list)
Depends on:
Blocks: 3523
  Show dependency treegraph
 
Reported: 2013-10-23 16:42 CEST by Pierre Ossman
Modified: 2014-05-13 10:36 CEST (History)
3 users (show)

See Also:
Acceptance Criteria:


Attachments
Test protocol of keyboards in different platform / browser combinations. (17.21 KB, text/x-markdown)
2014-05-12 14:15 CEST, Henrik Andersson
Details

Description Pierre Ossman cendio 2013-10-23 16:42:12 CEST
There is actually some standardisation effort going on for HTML's keyDown/keyPress/keyUp events:

http://www.w3.org/TR/DOM-Level-3-Events/#events-keyboardevents

We should make sure noVNC properly follows this and clearly separate out the compatibility hacks for older/non-conformant browsers. This should solve several known bugs in the current implementation.
Comment 1 Pierre Ossman cendio 2013-10-23 16:57:15 CEST
Also interesting for the hacky portions:

https://dvcs.w3.org/hg/dom3events/raw-file/tip/html/Note-KeyProps.html
Comment 2 Samuel Mannehed cendio 2013-10-24 11:42:24 CEST
Might be interesting:
https://github.com/kanaka/noVNC/pull/314
Comment 3 Pierre Ossman cendio 2013-11-06 14:47:53 CET
See bug 4808 for one fairly comprehensive list of keyboard bugs.
Comment 4 Pierre Ossman cendio 2013-12-11 09:11:25 CET
For now, this bug is about checking out the upstream work. If it turns out that they are not targeting the standard, then we'll have to split things up in to multiple bugs.
Comment 5 Samuel Mannehed cendio 2014-03-06 17:02:37 CET
Been trying the upstream code. A first glance says that it will not fix many issues, but it will also not introduce many new.

It seems like new browser versions have changed a few things in this as well. For example, Firefox on Android seems to not use keyEvents anymore which leads to the fix for bug 4602 (input event fallback implemented for Chrome on Android) being used. A problem with backspace seems to have been introduced in newer versions on both Chrome and Firefox on Android.

I have reported a bug for this upstream:
https://github.com/kanaka/noVNC/issues/344
Comment 6 Samuel Mannehed cendio 2014-04-04 19:57:54 CEST
(In reply to comment #5)
> A problem with backspace seems to have been introduced in
> newer versions on both Chrome and Firefox on Android.
> 
> I have reported a bug for this upstream:
> https://github.com/kanaka/noVNC/issues/344

This bug was extended to include fixing this backspace problem on Android.
I committed this work upstream and the fix is included in the vendordrop in commit 28807.
Comment 7 Samuel Mannehed cendio 2014-04-04 19:58:36 CEST
(In reply to comment #5)
> Been trying the upstream code. A first glance says that it will not fix many
> issues, but it will also not introduce many new.

The upstream code fixes Alt-Gr where it was broken before.

I have tested the following platforms and browsers:

Windows 7: IE10
Windows 8: IE11, Firefox 27, Chrome 33
Fedora 18: Firefox 27, Firefox 28, Chrome 33, Chrome 34
Mac OS X 10.9: Safari 7
Android 4.4: Firefox 27, Firefox 28, Chrome 33, Chrome 34
iOS 7: Safari 7, Chrome 32

The only regression that has been spotted so far with the upstream code is that keyboard input doesn't work properly on IE10. A fix seems to be in the works for this: https://github.com/kanaka/noVNC/issues/352
I tested the code mentioned in that issue and it does indeed seem to work.

Overall the keyboard code works well in all browsers on all client platforms that I have tested with a few exceptions, these are not regressions:

- Dead keys doesn't work in Firefox on Linux. Upstream-bug: https://github.com/kanaka/noVNC/issues/350
- When pressing the Enter/Go-key in Chrome on Android, the on screen keyboard is closed. Upstream-bug: https://github.com/kanaka/noVNC/issues/356
Comment 8 Samuel Mannehed cendio 2014-04-04 22:49:25 CEST
I reopen this and wait for the IE10 fix to be properly committed upstream. Will do a new vendor drop after that.
Comment 9 Samuel Mannehed cendio 2014-04-07 10:17:57 CEST
Commit 28811 - vnc.tmpl updated to reflect changes in vnc.html from the vendordrop.
Comment 10 Samuel Mannehed cendio 2014-04-08 17:10:05 CEST
(In reply to comment #7)
> - When pressing the Enter/Go-key in Chrome on Android, the on screen keyboard
> is closed. Upstream-bug: https://github.com/kanaka/noVNC/issues/356

It is more important to have a properly functioning Enter key on Chrome than to not have text suggestions. The vendordrop in commit 28823 fixes this.

The resulting problem with text suggestions is illustrated well by the screenshots in the upstream bug: https://github.com/kanaka/noVNC/issues/357
Comment 11 Samuel Mannehed cendio 2014-04-09 15:40:14 CEST
*** Bug 5079 has been marked as a duplicate of this bug. ***
Comment 12 Samuel Mannehed cendio 2014-04-09 15:54:53 CEST
(In reply to comment #11)
> *** Bug 5079 has been marked as a duplicate of this bug. ***

The problem found here was due to a mistake in the vendordrop in r28807. This problem was fixed in r28826.

Another merge miss was fixed in r28825.
Comment 13 Samuel Mannehed cendio 2014-04-14 16:32:12 CEST
(In reply to comment #8)
> I reopen this and wait for the IE10 fix to be properly committed upstream. Will
> do a new vendor drop after that.

Fixed in commit 28846.

Two additional fixes were also committed:

r28841 - keep vnc.tmpl in sync with vnc.html
r28840 - add missing import

Marking this as resolved now.
Comment 14 Henrik Andersson cendio 2014-05-12 14:15:36 CEST
Created attachment 542 [details]
Test protocol of keyboards in different platform / browser combinations.

Attached file includes test cases and protocol of issue for keyboard handling. Pierre has verified that the results is not worse than before.
Comment 15 Henrik Andersson cendio 2014-05-12 14:40:01 CEST
Opened a new bug #5135 with up-to-date keyboard issues, and closing this now when it shows better results then before.

Note You need to log in before you can comment on or make changes to this bug.