Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] fix (tab) indentation within CodeMirror #4832

Merged
merged 7 commits into from
Dec 13, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions static/js/cm-decorations.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Decoration, DecorationSet, EditorView, GutterMarker, MatchDecorator, ViewPlugin, ViewUpdate, gutter, } from '@codemirror/view'
import { RangeSet, StateEffect, StateField } from '@codemirror/state'

import { IndentContext } from '@codemirror/language'

export const addErrorLine = StateEffect.define<{ row: number }>();
export const addErrorWord = StateEffect.define<{ row: number, col: number }>();
Expand Down Expand Up @@ -247,4 +247,20 @@ export const placeholders = ViewPlugin.fromClass(class {
provide: plugin => EditorView.atomicRanges.of(view => {
return view.plugin(plugin)?.placeholders || Decoration.none
})
})
})

export function basicIndent(context: IndentContext, pos: number) {
const nextIndentationSize = context.lineIndent(pos, -1);
let prevIndentationSize;
try {
prevIndentationSize = context.lineIndent(pos + 1, -1);
} catch (error) {
prevIndentationSize = 0;
}
const indentBy = Math.max(
prevIndentationSize,
nextIndentationSize
);
return indentBy;

}
14 changes: 11 additions & 3 deletions static/js/cm-editor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ import { EditorView, ViewUpdate, drawSelection, dropCursor, highlightActiveLine,
highlightActiveLineGutter, highlightSpecialChars, keymap, lineNumbers } from '@codemirror/view'
import { EditorState, Compartment, StateEffect, Prec } from '@codemirror/state'
import { EventEmitter } from "./event-emitter";
import { deleteTrailingWhitespace, defaultKeymap, historyKeymap } from '@codemirror/commands'
import { deleteTrailingWhitespace, defaultKeymap, historyKeymap, indentWithTab } from '@codemirror/commands'
import { history } from "@codemirror/commands"
import { indentOnInput, defaultHighlightStyle, syntaxHighlighting ,LanguageSupport } from "@codemirror/language"
import { indentOnInput, defaultHighlightStyle, syntaxHighlighting ,LanguageSupport, indentUnit, indentService } from "@codemirror/language"
import { highlightSelectionMatches, searchKeymap } from "@codemirror/search";
import {
errorLineField, debugLineField, decorationsTheme, addDebugLine,
Expand All @@ -14,7 +14,8 @@ import {
incorrectLineField,
removeIncorrectLineEffect,
addDebugWords,
placeholders
placeholders,
basicIndent
} from "./cm-decorations";
import {LRLanguage} from "@codemirror/language"
import { languagePerLevel } from "./lezer-parsers/language-packages";
Expand All @@ -24,6 +25,10 @@ import { error } from "./modal";
import { ClientMessages } from "./client-messages";
import { Tag, styleTags, tags as t } from "@lezer/highlight";


// CodeMirror requires # of indentation to be in spaces.
const indentSize = ' '.repeat(4);

export class HedyCodeMirrorEditorCreator implements HedyEditorCreator {
/**
* This function should initialize the editor and set up all the required
Expand Down Expand Up @@ -114,7 +119,10 @@ export class HedyCodeMirrorEditor implements HedyEditor {
...defaultKeymap,
...searchKeymap, // we need to replace this with our own search widget
...historyKeymap,
indentWithTab,
Copy link
Member

Choose a reason for hiding this comment

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

It was a explicit decision not to use this. Felienne and I discussed it when making the change, and we ultimetaly decided to leave the default behaviour due to accessibility concerns that the CodeMirror maintainer expresses here: https://codemirror.net/examples/tab/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See this #4830 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

If we do this, then we need to mention somewhere that this escape hatch is possible! Perhaps on the tutorial?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think users will know that naturally, but I do agree, that would be the perfect spot to include it.

Copy link
Member

Choose a reason for hiding this comment

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

I'd be inclined to agree if our users were computer literates, but since our age group is around 12, I'm not sure they'd know this. We can also add it in the Teacher Manual in a future PR and merge this, what do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree.

]),
indentUnit.of(indentSize),
indentService.of(basicIndent),
monokai,
this.readMode.of(EditorState.readOnly.of(isReadOnly)),
errorLineField,
Expand Down
Loading