Skip to content

Add a hint explaining how to leave the editor - #428

Open
fiorsaoirse wants to merge 4 commits into
developfrom
bugfix/codeEdiorTrap-CMEM-7687
Open

fiorsaoirse wants to merge 4 commits into
developfrom
bugfix/codeEdiorTrap-CMEM-7687

Conversation

@fiorsaoirse

@fiorsaoirse fiorsaoirse commented Sep 10, 2026

Copy link
Copy Markdown
Contributor
  • show a panel with the navigation hint while focused
  • deprecate enableTab and remove its override
  • add tests and update changelog
  • add "actions" prop to CodeEditor to render actions within the scrollable container (instead of rendering actions as a separated container, see AutoSuggestion component)
  • improved Esc + Tab hotkey for AutoSuggestion component

* If the <Tab> key is enabled as normal input, i.e. it won't have the behavior of changing to the next input element, expected in a web app.
* @deprecated No longer affects Tab key behavior. Use `tabIntentStyle` and `tabForceSpaceForModes` instead.
*/
enableTab?: boolean;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property allowed to override Tab key setting that are set via tabIntentStyle - so it was possible to set tabIntentStyle="space" or set a mode that is listed in tabForceSpaceForModes and set enableTab - so as result we get indention using the Tab key. That's confusing and may lead to unexpected result. I'd suggest deprecating the property and making tabIntentStyle/tabForceSpaceForModes configuration the single source of indention.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is an explicit feature by CodeMirror. Is is possible to set how existing tab characters are displayed but not to support tabbing by itself.

Btw, even if you deprecate a property it need to be work like before until it is removed, otherwise this would be a breaking change. Please also add a version to the deprecation flag, e.g. @deprecated (v27).

Comment thread src/extensions/codemirror/CodeMirror.tsx Outdated
@fiorsaoirse
fiorsaoirse requested a review from haschek September 10, 2026 13:11
setView(view);

if (view?.dom) {
if (height) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the editor's inline height because the container now controls the total height, including the panel (if it exists)

@fiorsaoirse fiorsaoirse changed the title Add shortcut to leave the code editor and a hint explaining it Add a hint explaining how to leave the editor Sep 14, 2026
@fiorsaoirse
fiorsaoirse marked this pull request as ready for review September 14, 2026 11:09
@fiorsaoirse
fiorsaoirse marked this pull request as draft September 14, 2026 11:27
@fiorsaoirse
fiorsaoirse force-pushed the bugfix/codeEdiorTrap-CMEM-7687 branch from 590a165 to 04b5dcb Compare September 14, 2026 11:27
@fiorsaoirse
fiorsaoirse force-pushed the bugfix/codeEdiorTrap-CMEM-7687 branch from 8874756 to 0670547 Compare September 14, 2026 14:25
@fiorsaoirse
fiorsaoirse marked this pull request as ready for review September 14, 2026 14:29
*/
focusHint?: React.ReactElement;
/** Actions displayed inside the scrollable editing area, to the right of the content. */
actions?: React.ReactNode;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks unrelated to the issue. Maybe done to generalize the way how to add the clear button used in AutoCompleteField. Good idea but this adds more complexity to the change set, should be managed in its own feature branch because the it should be aligned/combined with the existing leftElement/rightElement properties, and maybe moved all together to CodeEditor.

actions?: React.ReactNode;
}

const FALLBACK_WARNING = "Press Escape then Tab to leave the editor.";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion FALLBACK_WARNING is misleading in its intention. Should be something like BLUR_HINT or LEAVE_EDITOR_MSG.

{hasToolbarSupport && editorToolbar(mode)}
{shouldIndentWithTab ? (
<ApplicationViewability hide="screen">
<span lang={focusHint ? undefined : "en"}>{focusHint ?? FOCUS_HINT}</span>

@haschek haschek Sep 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message cannot be focused by Tab because it has no tab index. I think, we can remove this info completely after we decided to display permanently the info in a footer panel how to leave the editor.

() =>
addExtensionsFor(
shouldIndentWithTab,
showPanel.of(createKeyboardHintPanel),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have an own class as extension to the default cm-panel class name here?

<div
className={`${eccgui}-codeeditor__keyboard-hint`}
data-testid="code-editor-warning"
style={{ visibility: focused ? "visible" : "hidden" }}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the footer panel is always visible we probably do not need to hide this message about leaving the editor until the editor is focused.

};

const modeRequiresSpaces = !!(mode && tabForceSpaceForModes?.includes(mode));
const shouldIndentWithTab = tabIntentStyle === "tab" && !modeRequiresSpaces;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure on this? Also if spaces are used then tab handling must be activated explicitly. The difference is that spaces used as soft tab, not \t. Using indentWithTab is independent from the actual chars used to display the indentation.

If we really want to remove enableTab then we probably should extend tabIntentStyle by an "off" value.

Currently we have an regression for CodeAutocompleteField because enableTab is ignored now.

PS: the old tabIndent check (you removed it) was probably also wrong regarding the check for tab/space on indentWithTab.

* If the <Tab> key is enabled as normal input, i.e. it won't have the behavior of changing to the next input element, expected in a web app.
* @deprecated No longer affects Tab key behavior. Use `tabIntentStyle` and `tabForceSpaceForModes` instead.
*/
enableTab?: boolean;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is an explicit feature by CodeMirror. Is is possible to set how existing tab characters are displayed but not to support tabbing by itself.

Btw, even if you deprecate a property it need to be work like before until it is removed, otherwise this would be a breaking change. Please also add a version to the deprecation flag, e.g. @deprecated (v27).

gap: $eccgui-size-inline-whitespace;
align-items: center;
justify-content: flex-end;
min-height: $eccgui-size-codeeditor-keyboard-hint-height;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably do not need to use more space than is needed.

align-items: center;
justify-content: flex-end;
min-height: $eccgui-size-codeeditor-keyboard-hint-height;
padding: 0.25rem $eccgui-size-inline-whitespace;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also use 0.5 * $eccgui-size-inline-whitespace to have small widths based on config vars.

justify-content: flex-end;
min-height: $eccgui-size-codeeditor-keyboard-hint-height;
padding: 0.25rem $eccgui-size-inline-whitespace;
font-size: 0.875rem;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use scss config vars here, we have font size and lineheight defined also for "caption".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants