Toggle features visibility shortcut not working on OSx

[Crashillo]

How to reproduce the issue?

⌥ + W doesn't work on MacOS to toggle the map data, just do nothing

Which iD Editor versions do you see the issue on?

Released version at openstreetmap.org/edit

Which browsers are you seeing this problem on?

Firefox, Chrome

[tyrasd]

Unfortunately, I don't own a Mac to properly debug this. But I have a few questions: Do other shortcuts using ⌥ work? E.g. ⌥ + - to zoom out fast? Does the W key alone switch the "wireframe" rendering mode for you or does it also do nothing? What language locale do you run?

[Crashillo]

Ok, I did a couple of tests:

• singles keys as W, /, B... work as expected
• ⌥ + arrow keys work as expected
• ⌥ + - works as expected, but ⌥ + + doesn't
• ⌥ + shift + arrow keys work as expected
• both ⌥ + shift + - and ⌥ + shift + + work as expected
• ⌥ + w doesn't work

navitagor.language = "es-ES"

[tyrasd]

Thanks. Can you try the version of iD on https://ideditor-release.netlify.app/ please? Could it be that you perhaps just haven't put the focus on the editor on osm.org (e.g. by clicking somewhere on the map after opening it)? Which version of Friefox/Chrome and MacOS are you using?

[Crashillo]

Can you try the version of iD on https://ideditor-release.netlify.app/ please?

Same results.

Could it be that you perhaps just haven't put the focus on the editor on osm.org (e.g. by clicking somewhere on the map after opening it)?

Editor has the focus...

Which version of Friefox/Chrome and MacOS are you using?

Latest

May you share a link to the file who handles this? Perhaps I can give you some clues

[tyrasd]

Latest

Which are what versions, exactly?

I'm asking because a user on the OSM discord chat reported that they were not able to reproduce this issue on MacOS. So we somehow need to narrow down what could be causing it.

Btw: are you running any browser plugins or other system wide tool which might interfere with keyboard shortcuts?

May you share a link to the file who handles this?

Most of the relevant code would be in https://github.com/openstreetmap/iD/blob/develop/modules/util/keybinding.js

[Crashillo]

Currently I'm running FF97, but I updated this morning, so in FF96 was happening also. In Chrome is 98.x. I'm using MacOS Monterey, btw. I ask a workmate to test it in his MacOS Big Sur (Chrome 98.x), and the error persists, so it's not related with my environment.

are you running any browser plugins or other system wide tool which might interfere with keyboard shortcuts?

I disabled any extension I had. Still the same.

[Crashillo]

I'd been digging on this: the source of the problem is the keyboard distribution (not the browser locale), in english, when you type ⌥ + w you get ∑, unlike spanish distribution, where ⌥ + w displays æ

So, the issue comes up because the matches function returns false for spanish due to this snippet:

iD/modules/util/keybinding.js

Lines 56 to 80 in ad67f81

	if (event.key !== undefined) {
	tryKeyCode = (event.key.charCodeAt(0) > 255); // outside ISO-Latin-1
	isMatch = true;
	if (binding.event.key === undefined) {
	isMatch = false;
	} else if (Array.isArray(binding.event.key)) {
	if (binding.event.key.map(function(s) {
	return s.toLowerCase();
	}).indexOf(event.key.toLowerCase()) === -1) {
	isMatch = false;
	}
	} else {
	if (event.key.toLowerCase() !== binding.event.key.toLowerCase()) {
	isMatch = false;
	}
	}
	}
	// Fallback match on `KeyboardEvent.keyCode`, can happen if:
	// - browser doesn't support `KeyboardEvent.key`
	// - `KeyboardEvent.key` is outside ISO-Latin-1 range (cyrillic?)
	if (!isMatch && tryKeyCode) {
	isMatch = (event.keyCode === binding.event.keyCode);
	}

I actually don't understand what purpose has the first conditional block, as the better comparison possible stands at line 79, where it compares both keyCode, instead of the key which is distribution-dependent. Currently, in spanish it doesn't run through the last if-block since tryKeyCode is false (event.key.charCodeAt(0) is 230 for ⌥ + w combination)

IMO, I'd just replace the full bunch of code I beforementioned with a simple var isMatch = (event.keyCode === binding.event.keyCode);, what do you think?

[tyrasd]

Good catch! 👍

The whole keyboard shortcut handling is unfortunately a bit more complex than one would think.

just replace the full bunch of code I beforementioned with a simple var isMatch = (event.keyCode === binding.event.keyCode);

Interestingly, before #3572, it was basically implemented like this. But a keyCode-only approach has a few major downsides, especially in situations where a specific (mostly non-alphanumeric) character is behind a modifier on a specific keyboard layout (e.g. \ is AltGr + ß on a German keyboard and reports as keyCode 219 in a keyDown event on Chrome, but Firefox reports it as 63 🤷 ). The switch to KeyboardEvent.keyCode API improved this, but then we also wanted to support non-latin keyboard layouts, where at the keyCode approach used to be the only alternative (see #4618).¹

I've fixed the issue here by relaxing the fallback requirements in bb3ab92 (and 22bd628).

Footnotes

1. Nowadays, there is KeyboardEvent.code, which we could use now (since we don't need to support Internet Explorer anymore – Drop support for IE11 #8811), but that would be a bit more work to refactor than what I feel comfortable squeezing into this bug fix. ↩