Page MenuHomePhabricator

Integrate bracket matching for CodeMirror with VisualEditor
Closed, ResolvedPublic5 Estimated Story Points

Description

During prototyping, we found the VisualEditor requires some cursor synchronization in order for bracket matching to work correctly. This task is to integrate, behind the same feature flag as the parent task.

Existing patches

Event Timeline

awight created this task.Dec 1 2020, 10:14 AM
awight removed projects: Community-Tech, Epic.
awight updated the task description. (Show Details)
Restricted Application added a project: Community-Tech. · View Herald TranscriptDec 1 2020, 10:16 AM
Lena_WMDE set the point value for this task to 5.Dec 1 2020, 10:26 AM

Change 649593 had a related patch set uploaded (by Svantje Lilienthal; owner: Svantje Lilienthal):
[mediawiki/extensions/CodeMirror@master] Integrated bracket matching for CodeMirror with VisualEditor

https://gerrit.wikimedia.org/r/649593

thiemowmde added a subscriber: thiemowmde.EditedDec 21 2020, 5:32 PM

After adding dozens and dozens of console.log() everywhere I started to understand the issue a bit better.

  • The matchbrackets addon doesn't work in VisualEditor.
  • The addon runs. It is correctly initialized and called when it should be called.
  • But when the addon is called, the cursor position is always line 0, character 0. You can even see this when you edit a page and type something like [[…]] right at the start.
  • The reason for this misbehavior is that the focus is never inside of the <div class="CodeMirror cm-s-default CodeMirror-wrap"> element. But CodeMirror expects the focus to be there. Otherwise significant parts of CodeMirror don't work. (Well, obviously parts that have not been needed so far.)
  • Instead, VisualEditor puts an element <div class="ve-ce-branchNode ve-ce-documentNode ve-ce-attachedRootNode ve-ce-rootNode mw-content-ltr mw-parser-output ve-ce-documentNode-codeEditor-webkit-hide" contenteditable="true" spellcheck="true" tabindex="0" data-gramm="false" role="textbox" lang="en" dir="ltr"> on top of CodeMirror. That's where the focus is. The element is invisible via .ve-ce-documentNode-codeEditor-webkit-hide { -webkit-text-fill-color: transparent; }.

I realized to late that the later patch set 2 contains code to propagate the cursor position from the top element ("mirror") down to CodeMirror. With this bracket matching works! Unfortunately the performance impact is rather extreme:

Edit: I played around with different approaches in patch set 3. I temporarily left all code in the patch.

Change 651530 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[VisualEditor/VisualEditor@master] Improve SourceConverter.getSourceTextFromDataRange performance

https://gerrit.wikimedia.org/r/651530

Change 651530 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] Improve SourceConverter.getSourceTextFromDataRange performance

https://gerrit.wikimedia.org/r/651530

Change 652210 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (d0d57d2bc)

https://gerrit.wikimedia.org/r/652210

Change 652210 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (d0d57d2bc)

https://gerrit.wikimedia.org/r/652210

Change 649593 merged by jenkins-bot:
[mediawiki/extensions/CodeMirror@master] Integrated bracket matching for CodeMirror with VisualEditor

https://gerrit.wikimedia.org/r/649593

Change 654424 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/CodeMirror@master] Simplify onSelect handler implementation

https://gerrit.wikimedia.org/r/654424

lilients_WMDE added a subscriber: lilients_WMDE.
Lena_WMDE closed this task as Resolved.Wed, Jan 6, 9:39 AM
Lena_WMDE moved this task from Doing to Done on the WMDE-QWERTY-Sprint-2020-12-02 board.

Change 654424 merged by jenkins-bot:
[mediawiki/extensions/CodeMirror@master] Simplify onSelect handler implementation

https://gerrit.wikimedia.org/r/654424