jitsi-meet icon indicating copy to clipboard operation
jitsi-meet copied to clipboard

Fix Keyboard Shortcuts clashing with whiteboard

Open Bloodiko opened this issue 1 year ago • 11 comments

Add canvas to blacklisted elements Add return if ctrl modifier is used on the keypress.

  • none of jitsis shortcuts use ctrl - so we should ignore those

Currently shortcuts like CTRL+C and CTRL + V and other system shortcuts will trigger jitsis shortcut handler, even if not intended.

The guard check in this commit will prevent interference with system shortcuts by ignoring events with ctrl completely.

Bloodiko avatar Oct 29 '23 20:10 Bloodiko

Hi, thanks for your contribution! If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

jitsi-jenkins avatar Oct 29 '23 20:10 jitsi-jenkins

The Second commit also adds the metaKey property to the check.

shortcuts with the windows-key or the mac cmd-key should also be ignored.

Bloodiko avatar Oct 29 '23 20:10 Bloodiko

LGTM at a glance. @mihhu can you PTAL?

saghul avatar Oct 30 '23 09:10 saghul

I'm not sure why the metaKey boolean doesn't work in this case, I tested it separately and my Chrome detects it correctly. Could you please add an extra check for the left and right command button? Maybe just checking for the key to be "Meta" would be enough. image

mihhu avatar Oct 31 '23 13:10 mihhu

I'm not sure why the metaKey boolean doesn't work in this case, I tested it separately and my Chrome detects it correctly. Could you please add an extra check for the left and right command button? Maybe just checking for the key to be "Meta" would be enough. image

I was relying on Mozilla MDN for that specific code, thus was unable to verify its functionality.

@mihhu did you try an actual shortcut with command ?

From the screenshot it seems that you pressed the command button, which from my perspective looks like intended behavior.

The boolean should be true, if the command key was already pressed before the next key event occurs.

Which means, that the first command key Event which will trigger the onKeyDown will not have the boolean set, but while holding it - crating another keyDown or KeyUo event ( CMD + c ) for example, should show code "c" in with the meta boolean on true.

Could you please verify if my assumption is correct ? ( due to my lack of apple devices. - is it possible to emulate this apple behaviour myself inside the browser - I'll check and add this info here. )

Bloodiko avatar Oct 31 '23 21:10 Bloodiko

@mihhu did you try an actual shortcut with command ?

Yes, cmd+A, cmd+C, cmd+V. It triggered the Jitsi shortcut, so my guess is it didn't return as it should've. I'll try to look into it too.

mihhu avatar Nov 01 '23 14:11 mihhu

I did an extensive test on some apple devices: Macbook and Mac-Mini - Both had the same results (I tested both devices with chrome and safari each):

I was unable to trigger Jitsi Shortcuts while holding the cmd key, but for a different reason than my code working.

TLDR: In my tests the boolean worked more or less intentionally as it is supposed to. There was no keyup event for any letter-key whenever a key combination included the cmd-key (e.g. CTRL + CMD + R )

Long Version:

I tested multiple key command combinations to figure out what happens inside the keyevents at specific times.

Those include - Keydown and Keyup events each:

  • Single Letters ( a, c, v)
  • Single Modifier Keys ( Ctrl, Meta, Alt, Shift) (including different keyboard sites, e.g. Ctrl-right, meta-right,... )
  • Simple Combinations ( Ctrl + a , ... )
  • Advanced Combinations ( Ctrl + Meta + C , ...)
key-combination keydown keyup
letter :heavy_check_mark: :heavy_check_mark:
single modifier :heavy_check_mark: :heavy_check_mark:
single combinations as intended - Meta Key Boolean is true if meta key is pressed :no_entry: never happend - probably system side blocked
advanced combinations as intended - Meta Key Boolean is true if meta key is pressed :no_entry: never happend - probably system side blocked

Findings

  • I was able to copy and paste from/to the whiteboard on the tested browsers, even without the added modification to the code above
  • the keyup event never happend when the cmd key was involved. - the keydown event happened as intended. ( this is contradicting to your test. - did you perhaps use firefox for the test ? - eventually its also a os mismatch which causes the difference in behaviour)
  • the keydown events do include the correct information

Result Questions:

While it may be possible to store cmd key being pressed in a variable to fix this, i'd personally not want to do that, as it might be too much of a code change for a relatively simple issue.

Alternatively we may be able to change the keyup to a keydown and filter out repeating keyevents with the e.repeat boolean ? - Which would result in a working key-event with working ctrl, meta, and other modifier booleans set.

Bloodiko avatar Nov 06 '23 18:11 Bloodiko

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions[bot] avatar Feb 05 '24 01:02 github-actions[bot]

@mihhu what's the state of this?

saghul avatar Feb 05 '24 07:02 saghul

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions[bot] avatar May 06 '24 01:05 github-actions[bot]

Ping @mihhu

saghul avatar May 06 '24 06:05 saghul

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions[bot] avatar Aug 05 '24 01:08 github-actions[bot]