Change in arrow keys handling broke contract

[krassowski]

Description

#252 broke navigation with arrow keys, selecting cells with arrow keys and all other operations using arrow keys in JupyterLab (which for now is only visible on JupyterLab Desktop jupyterlab/jupyterlab-desktop#320). This is because the addition of:

lumino/packages/commands/src/index.ts

Lines 1138 to 1145 in 0f7378b

	} else if (token === 'ArrowLeft') {
	arrowLeft = true;
	} else if (token === 'ArrowUp') {
	arrowUp = true;
	} else if (token === 'ArrowRight') {
	arrowRight = true;
	} else if (token === 'ArrowDown') {
	arrowDown = true;

means that:

lumino/packages/commands/src/index.ts

Lines 1146 to 1148 in 0f7378b

	} else if (token.length > 0) {
	key = token;
	}

will not be executed for arrow keys, and all existing logic that relayed on arrow keys being there will fail.

Reproduce

See jupyterlab/jupyterlab-desktop#320.

Expected behavior

1. this was released in a minor version, so we should fix it quickly; we should not change the contract in minor versions
2. arrow keys are not proper modifiers and should not be excluded from key

[jasongrout]

I'm trying to trace down what happened in #252:

It seems that #252 was trying to fix #151. In #151, I suggested that a shortcut that contained an arrow key be formatted with a unicode arrow symbol rather than the text "ArrowUp", etc. Since an arrow key is not a modifier (strongly agree with @krassowski there), I was imagining that we would replace

lumino/packages/commands/src/index.ts

Line 1164 in 4f24322

return mods + parts.key;

with something that would just be a check on parts.key to see if parts.key was 'ArrowUp' (replace with unicode rightwards arrow), etc. In other words, we define a replacement map for parts.key that would replace the parts.key text with a unicode symbol if it was in the replacement map. I was not imagining we'd extend the modifier functionality to include arrows. I should have been clearer on that point.

Does extending the approach from #252 of making a separate flag for arrow keys make sense, or should we revert #252 and instead just check to see if we should return a unicode symbol for parts.key if parts.key is 'ArrowUp', 'ArrowDown', etc.?

[jasongrout]

Does extending the approach from #252 of making a separate flag for arrow keys make sense, or should we revert #252 and instead just check to see if we should return a unicode symbol for parts.key if parts.key is 'ArrowUp', 'ArrowDown', etc.?

I added some more context and thoughts at #151 (comment), since that seems like the best place to consolidate the conversation across #151, #252, #255, and #256.