Page MenuHomePhabricator

Upstream or fork bracket matching for CodeMirror
Open, Needs TriagePublic3 Estimated Story Points

Description

We've made some small changes to the bracket matching plugin. These need to be upstreamed, or we need to rename the plugin and call it our own.

CSS changes should probably be pushed out to the MediaWiki-extension-CodeMirror, since they don't belong upstream. Accessible colors should be reverted (due to invalidation during test). Error highlighting should be suppressed but by introducing a configuration flag.

Our changes would ideally not make any difference to the behavior of the upstream library. Each feature, especially if it does make a behavioral change, needs to be isolated into its own patchset, ideally not dependent on any others. This makes it easier to submit patch requests and discuss orthogonally.

"Renaming" would be a fallback strategy. If we're unable to upstream all of our changes, then we need to visibly fork the plugin, e.g. "brackets-wmde.js", and include self-documentation explaining when and how we forked. This is necessary so that the people responsible for the future CodeMirror 6 upgrade aren't taken by surprise, and accidentally miss our customizations during the migration. (Note: T270317 and this ticket partly overlap.)

To do:

  • Document our customizations. Done via T270317.
  • Rename our customized version. Done: https://gerrit.wikimedia.org/r/656103
  • Check which of our changes we can upstream.
    • Introduce a config flag to disable the error highlighting. Done: https://github.com/codemirror/CodeMirror/pull/6565
    • We "fixed" a compatibility issue between matchbrackets addon and mediawiki mode by disabling a feature. Can we find a generic solution that works for everybody? Done: https://github.com/codemirror/CodeMirror/pull/6572
    • We scan for pairs even if the cursor is not on a bracket. While it would be possible to submit this upstream, it's a new feature that would dramatically change what the addon does. It would need to be disabled by default. The cost to create a patch and maintain this feature (being the only user) is probably not worth it. Let's live with the fact that we have a fork.
  • Reverting the accessibility colors is not necessary. This was exclusively on GitLab.

Event Timeline

awight created this task.Dec 1 2020, 10:16 AM
Lena_WMDE set the point value for this task to 3.Dec 1 2020, 10:28 AM
awight updated the task description. (Show Details)Dec 15 2020, 1:06 PM

Change 656103 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/CodeMirror@master] Rename our customized matchbrackets addon

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

thiemowmde updated the task description. (Show Details)

Change 656119 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/CodeMirror@master] [WIP] Disable non-matching highlighting via config flag

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

thiemowmde updated the task description. (Show Details)Thu, Jan 14, 11:42 AM
thiemowmde updated the task description. (Show Details)Thu, Jan 14, 2:57 PM

Change 656119 merged by jenkins-bot:
[mediawiki/extensions/CodeMirror@master] Disable non-matching highlighting via config flag

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

thiemowmde updated the task description. (Show Details)Tue, Jan 19, 2:59 PM
thiemowmde added a comment.EditedTue, Jan 19, 3:20 PM

I finally understood the incompatibility issue between our mediawiki mode and the matchbrackets addon.

The addon is written in a way that it does not only check if the characters match (e.g. ( and )), it also checks if these two characters have the same style a.k.a. the same CSS class names. You can test this on codemirror.net with code like this:

<script>
function(; //comment)
</script>

This pair of brackets won't match because they have two different styles. This is intentional and fine.

Our problem is that the mediawiki mode uses an empty string for the default black text. This empty string ends in the addon in a line style || null where it is turned into null. This later fails because '' != null.

I don't really get why this || null is there. The value on the left can't be anything but a string or null. The only thing the || does is turning the empty string into null. But why? The only reason to do this is to intentionally not have any bracket matching when a mode does not apply a class name to basic text. But again, why? Most modes do apply a class name even to basic text, they just don't style it (i.e. it's still the default black, but does have a class name for optional styling). In other words: It's not like this distinction understands when text is styled and when not. The distinction is made based on a technical aspect that is not even guaranteed to be user-facing.

The hack we did removed the extra check entirely. This does have two effects:

  • In an example like the one above where two brackets have different colors, they will still be highlighted.
  • It significantly improves the performance.

I suggest to leave our hack as it is because of the performance implications, but suggest to remove the "|| null" upstream. We can come back and change this later when users complain about unexpected highlighting.

I don't want to change our mediawiki mode to apply a non-empty default style because I suspect this might affect the performance.

thiemowmde updated the task description. (Show Details)Tue, Jan 19, 3:35 PM