fix: account for ephemeral focus in enter precondition #619
Conversation
This illustrates that the latest version, based on field-grid-dropdown, does not work with keyboard navigation enabled. This is because Enter propagates to the global handler, causing the field editor to be retriggered or block toast messages to be shown.
Currently with 6.0.2 this triggers the global shortcuts.
Previously bugs could occur if the field editor contained a button but did prevent keydown events propagating.
microbit-matt-hillsdon
requested review from
BenHenning
and removed request for
a team
| */ | ||
| canCurrentlyEdit(workspace: Blockly.WorkspaceSvg) { | ||
| return this.canCurrentlyNavigate(workspace) && !workspace.options.readOnly; | ||
| return this.canCurrentlyNavigate(workspace) && !workspace.isReadOnly(); |
There was a problem hiding this comment.
Changed for clarity as I had to read the code to learn that options.readOnly was mutated when setIsReadOnly was called.
|
I think this regressed in #597 when the relevant preconditions started just checking the state not calling the common canXXX methods. I think calling the navigate one as I have done maintains the needed behaviour, but it would be safe written either way with the changes to |
There was a problem hiding this comment.
Thanks @microbit-matt-hillsdon. This change makes sense, and the new test will hopefully keep this working if this code has to change again.
No concerns on my end, just one comment that may lead to a slight test improvement.
| await this.browser.waitUntil( | ||
| () => | ||
| this.browser.execute(() => | ||
| document.activeElement?.classList.contains('blocklyActiveFocus'), |
There was a problem hiding this comment.
| document.activeElement?.classList.contains('blocklyActiveFocus'), | |
| document.activeElement?.classList?.contains('blocklyActiveFocus'), |
Is this needed to avoid a potential null?
There was a problem hiding this comment.
I don't think so, e.g. testing in Node:
> document = { activeElement: null }
{ activeElement: null }
> document.activeElement?.classList.contains('foo')
undefined
There was a problem hiding this comment.
To add to matt's answer, if the activeElement exists, the classList will exist too (it might be an empty list, but it won't be null) https://developer.mozilla.org/en-US/docs/Web/API/Element/classList
There was a problem hiding this comment.
Ah. This behaves differently in Kotlin land (where I'm much more familiar). There the '?' actually cascades. In this case, document.activeElement?.classList would evaluate to either null or the class list which is why the additional ? would be needed (since it would be trying to call contains on null rather than avoiding the call altogether).
This will take me some time to get used to. :)
There was a problem hiding this comment.
Yeah, the way optional chaining works in JS is very pragmatic from the point of view of programmers but decidedly weird from the point of view of JS implementers. I found the parse trees that result from expressions with optional chaining to be extremely unintuitive—despite being (or perhaps because I was) already very familiar with ES5 parsing.
4 participants
Previously bugs could occur if the field editor contained e.g. a button but did not prevent keydown events propagating.
This is true of field-colour from 6.0.2 so this PR upgrades to demonstrate that.
I've tried to make the enter action's preconditions more similar to the others, using canCurrentlyNavigate/canCurrentlyEdit.
I've also changed where we account for ephemeral focus so that the Constants.STATE value is NOWHERE when ephemeral focus is in progress. I don't think anything good can come of the state value representing somewhere that doesn't have focus.
Demos:
Closes #618