www.cendio.com
Bug 6152 - Web Access keyboard code is needlessly complex
: Web Access keyboard code is needlessly complex
Status: CLOSED FIXED
: ThinLinc
Web Access
: 1.3.1
: PC Unknown
: P2 Normal
: 4.11.0
Assigned To:
:
:
: 7365
: 3523 5135 7417 7423 7429
  Show dependency treegraph
 
Reported: 2017-01-30 09:43 by
Modified: 2019-11-11 16:05 (History)
Acceptance Criteria:


Attachments


Note

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


Description From cendio 2017-01-30 09:43:23
The HTML keyboard code is very complex and fragile, making it difficult to do
any fixes or improvements to it. It has several problems:

 a) It is overly abstracted, with a complex pipeline model even though the
steps in the pipeline are fixed. The steps aren't even independent of each
other.

 b) The code has grown organically as a series of individual fixes and lacks an
overall algorithm.

 c) Large parts of the code is there to deal with old browsers that aren't in
use and do not fulfil noVNC's other requirements anyway.

A major cleanup is needed.
------- Comment #1 From cendio 2017-01-30 10:35:36 -------
One symptom of this is that when fixing support for Unicode characters (needed
for many virtual keyboards), Enter and Backspace on IE/Edge stopped working:

https://github.com/novnc/noVNC/issues/734
------- Comment #2 From cendio 2017-02-02 15:51:03 -------
(In reply to comment #1)
> One symptom of this is that when fixing support for Unicode characters (needed
> for many virtual keyboards), Enter and Backspace on IE/Edge stopped working:
> 
> https://github.com/novnc/noVNC/issues/734

The code that caused this was reverted in r32158. 

When the PR below is merged and we are going to include it in thinlinc, we need
to revert the above commit.

https://github.com/novnc/noVNC/pull/766
------- Comment #3 From cendio 2017-02-03 13:42:31 -------
We decided to not do this right now.
------- Comment #4 From cendio 2017-02-03 13:43:11 -------
(In reply to comment #2)
> 
> https://github.com/novnc/noVNC/pull/766

This PR seems to solve most of the issues in bug 5135, reducing them to just a
handful specific things.
------- Comment #5 From cendio 2018-02-14 09:23:55 -------
The PR mentioned here has been merged upstream.
------- Comment #6 From cendio 2019-10-23 12:56:23 -------
For a test protocol we can use TigerVNC's again:

https://github.com/TigerVNC/tigervnc/blob/master/doc/keyboard-test.txt
------- Comment #8 From cendio 2019-10-31 13:06:45 -------
We've now retested everything on every platform using:

 * Firefox 70 (68 on Android/Chromebook)
 * Chrome 78   
 * IE 11       
 * Edge 18     
 * Safari 13   

The following already existing issues remain:

 * Keys grabbed by the browser (bug 4755)
 * Keys grabbed by the system (bug 7414)
 * Dead keys (bug 7415)
 * AltGr broken for virtual Windows keyboard (bug 7276)
 * F14/F15 do not work on macOS (bug 5934)
 * Pressing both shifts on Windows (bug 7416)
 * Modifier shuffle on iOS (bug 7417)
 * Right shift sends left shift on iOS (bug 7418)
 * KP_Begin broken in Firefox (bug 7419)
 * Shift+Numpad broken in Edge (bug 7420)
 * Some broken media keys (bug  7421)
 * Dead keys and backspace extra broken on Android (bug 7422)
 * NumLock problems on macOS (bug 7423)
 * Help broken in Firefox on macOS (bug 7424)
 * Alt+Space in Safari (bug 7425)
 * Alt in Firefox (bug 7426)
 * Input methods on Android (bug 7427)
 * Focus from address bar in IE (bug 7428)
 * Ctrl+AltGr in IE and Edge (bug 7429)
 * åäö with Android (bug 7430)
 * AltGr with Android (bug 7431)

New issues (i.e. regressions):

 * Shift-Alt (i.e. Meta) broken on Linux
 * KP_Delete broken in Chrome on Linux
 * AltGr+\ broken in Edge on Windows (Swedish keyboard)
 * Impossible to keep key pressed in iOS
 * Ctrl+Alt+<key> has inconsistent behaviour on macOS
------- Comment #9 From cendio 2019-10-31 13:49:04 -------
(In reply to comment #8)
> 
>  * Shift-Alt (i.e. Meta) broken on Linux
> 

This used to work because we just blindly looked at KeyboardEvent.keyCode. Now
we examine what the browser gives us, and it seems the browsers have gotten
this just horribly wrong. "Meta" is what HTML calls the Windows key, so it has
gotten mixed up with the original Meta key.

Reported to Chrome here:

https://bugs.chromium.org/p/chromium/issues/detail?id=1020141

Firefox already has a discussion about this here:

https://bugzilla.mozilla.org/show_bug.cgi?id=1232918
------- Comment #10 From cendio 2019-10-31 13:58:10 -------
Also see bug 7281 for issues with Meta in the native client.
------- Comment #11 From cendio 2019-10-31 14:39:42 -------
Meta fixed upstream:

https://github.com/novnc/noVNC/commit/758399050deda24132a8c9e24ef9d552283abe99
------- Comment #12 From cendio 2019-10-31 14:52:50 -------
(In reply to comment #8)
>  * KP_Delete broken in Chrome on Linux

Reported to Chrome:

https://bugs.chromium.org/p/chromium/issues/detail?id=1020158

And workaround in noVNC here:

https://github.com/novnc/noVNC/commit/9d956e919834ca0877f993d30a05cd8e3d182aca
------- Comment #13 From cendio 2019-10-31 15:37:55 -------
(In reply to comment #8)
>  * Impossible to keep key pressed in iOS

This is an explicit behaviour because iOS gives us broken keyup events:

https://github.com/novnc/noVNC/commit/9e99ce126ca8f6f350fa015c0e58d35103c62f7e

However retesting on iOS 13 shows that this bug has been fixed. So the
workaround is now removed:

https://github.com/novnc/noVNC/commit/8c51e9a8a28afb0a46570fa557dab511d4af6d15
------- Comment #14 From cendio 2019-10-31 15:51:30 -------
(In reply to comment #8)
>  * Ctrl+Alt+<key> has inconsistent behaviour on macOS

This is a change in behaviour, but not necessarily a regression. The previous
code had poor handling of layouts (breaking e.g. AZERTY layouts), but it would
result in:

 * Ctrl+Alt+e sends Ctrl+Alt+e
 * Ctrl+Cmd+e sends Ctrl+Super+e

But you have to remember that Alt behaves like AltGr on macOS. So that's why
we've done the modifier shuffle there (see bug 7417). If you look at the native
client you get:

 * Ctrl+Alt+e sends Ctrl+ModeSwitch+é
 * Ctrl+Cmd+e sends Ctrl+Alt+e

This is now also how Firefox and Chrome behaves, so things are getting more
consistent across our clients. However Safari gives us a plain "e" when we are
expecting a "é". And Safari doesn't give us any information that this key
really is é, so we'll have to wait for them to fix things. Moving this to bug
7432.
------- Comment #15 From cendio 2019-11-01 10:01:05 -------
(In reply to comment #8)
>  * AltGr+\ broken in Edge on Windows (Swedish keyboard)

AltGr+| was also affected. Fixed upstream here:

https://github.com/novnc/noVNC/commit/5736ea0bd50db477ac501ffc016b0b4ca3268543
------- Comment #17 From cendio 2019-11-05 10:10:39 -------
Fixes in place. What needs retesting:

 * Shift+Alt on Linux (should give XK_Meta_*
 * Windows keys
 * Numpad decimal/delete key
 * That no key gets stuck down in iOS (i.e. missing KeyRelease)
 * AltGr in IE and Edge
 * General keyboard testing in IE and Edge
------- Comment #18 From cendio 2019-11-06 13:30:15 -------
(In reply to comment #17) 
>  * Shift+Alt on Linux (should give XK_Meta_*

Tested on Fedora 30 in Firefox 69 and Chrome 78, works fine.
------- Comment #19 From cendio 2019-11-06 13:38:45 -------
I retested the following parts from comment #17: 

(In reply to comment #17)
> 
>  * Windows keys
>  * Numpad decimal/delete key
>  * AltGr in IE and Edge
>

Tested these on:
 * Microsoft Edge 18
 * Internet Explorer 11 
 * Chrome 77
 * Firefox 70

Windows keys works for all browsers. 

Numpad decimal/delete key also works as intended for all browsers. Tested with
UK layout which sends Decimal when pressing the delete key and Swedish layout
sends Separator.

AltGr in Edge and IE sends ISO_Level3_Shift.

I also tested that AltGr+/ sends "braceleft" and AltGr+| sends "bar".
------- Comment #20 From cendio 2019-11-06 14:23:36 -------
(In reply to comment #17)
>  * That no key gets stuck down in iOS (i.e. missing KeyRelease)

Tested on iOS 13 in Safari 13.
The only key that I can find that gets stuck is Caps Lock. It sends only a key
down or key up for each press which it shouldn't.
------- Comment #21 From cendio 2019-11-06 14:26:58 -------
General testing on Edge and IE consisted of testing:

* Left/right shift identification
* French layout
* Local changes are respected
* CapsLock, Shift, ALtGr affects symbol lookup
* Arrow keys
* Menu

Everything works fine for both browsers.
------- Comment #23 From cendio 2019-11-08 09:47:04 -------
CapsLock has been fixed. Needs retesting on macOS and iOS.
------- Comment #24 From cendio 2019-11-11 16:05:25 -------
(In reply to comment #23)
> CapsLock has been fixed. Needs retesting on macOS and iOS.

Verified that I only got either keydown OR keyup events for CapsLock on iOS
with build 6285. Verified that I do indeed get both down AND up events for
CapsLock on both iOS and macOS now with build 6292. Both events come at the
time of keydown, but that's acceptable.

Tested using Safari on iOS 13 and Safari on macOS 10.14. Works well.