Page MenuHomePhabricator

Investigation: Bracket matching
Closed, ResolvedPublic8 Estimated Story PointsSpike

Description

Proposed changes to CodeMirror ext

When cursor is either before, after, or within any bracket or bracket set, both that bracket/set and its pair are highlighted with a background color. Should include all types of brackets and tags that exist within wikitext: < > </ >, [[ ]], { }, {{ }}, {{{ }}}. If multiple brackets function as a group, the entire group should be highlighted.

Note: CodeMirror has an existing plug-in for this that can hopefully be built on edit/matchbrackets.js.

Brace Matching.png (900×1 px, 212 KB)

Open questions for investigation

The following questions apply to the CodeMirror extension within two different editors. Whatever implementation we choose must include both the 2017 Wikitext editor (source view inside VE) and the 2010 wikitext editor.

  1. Is it possible to highlight brackets as sets, not just individually? Ex. Double and triple curly brackets {{{ }}}
  2. Is it possible to highlight the brackets related to the current section if cursor is in the middle?

Scope

  • This ticket is not about implementing bracket matching yet, we would just like to understand better the opportunities and limitations related to the topic. By looking into what is possible and its potential difficulty, it can help in choosing what we will eventually implement within the template topic.
  • In particular, given the complication of implementing the changes to CodeMirror across two editors. We would like to better understand the difficultly of implementation across these different editors, particularly the complicating factor of the stacked surfaces in VE (users interact with the transparent ‘top’ surface of VE content but they see the CodeMirror content underneath).
  • The goal is to improve writing and editing templates, but any changes to CodeMirror will also affect syntax highlighting source code for other namespaces such as wikitext for articles. If this expands the needed work, would also be important to note that in this investigation.
  • It has already been raised that syntax highlighting does not work in RLT (see T170001). Exploring how bracket matching does or does not affect that, unfortunately expands the scope too much for this investigation. For now, set these issues aside.
  • It has also been raised that bracket matching might be particularly difficult in some non-latin writing systems, particularly Japanese and Chinese, which use IME. Solving this issue is also not part of the scope, but it would be good to understand how much this could complicate/block work in this area.

Notes
Related task: Request for bracket matching (from 2013) T15302

Related Objects

Event Timeline

Restricted Application changed the subtype of this task from "Task" to "Spike". · View Herald Transcript
Restricted Application added subscribers: Liuxinyu970226, Aklapper. · View Herald Transcript
Lena_WMDE set the point value for this task to 8.Jul 16 2020, 8:58 AM

First findings:

  • Yes, bracket matching is possibly with an add-on. See https://codemirror.net/doc/manual.html#addon_matchbrackets.
  • As far as I can tell our CodeMirror extension currently does not use any add-on.
  • I think it is possible to fork the existing add-on and change it to fit our use-case better. Notably: We want [[, {{{ and such to be highlighted as pairs/triples.
  • As mentioned in T254989#6337894, it looks like the syntax highlighter is the same for both editors that support syntax highlighting (in VE and in the classic editor).
  • As of T243835#6339118, there is a plan to upgrade to CodeMirror 6, which is an incompatible rewrite.
  • Existing bracket matching for version 5: https://codemirror.net/addon/edit/matchbrackets.js
  • Existing bracket matching for version 6: https://github.com/codemirror/codemirror.next/blob/master/matchbrackets/src/matchbrackets.ts
  • One possible way forward is to install the existing add-on for now, but don't touch it. If we want to invest time for pairs/tripples support, it might be better to do this after the version 6 update.
  • When we install the add-on, I suggest to do a minor version 5 update as well. See T258999.
  • It is definitely possible to highlight the brackets when the cursor is somewhere in the middle. However, we need to modify the existing add-on to make this possible. The idea is to not only look at the 2 characters before and after the cursor, but scan for brackets in both directions. When a bracket is found, and it points in the correct direction, we can start from there and highlight the pair. Warning, it might turn out this impacts the performance and overall experience so much that we can't ship it to all users. Not all users have powerful machines. There are ways to limit negative effects, e.g. by limiting the search radius. When the brackets are very far away (multiple lines above or below), they won't be highlighted any more.

Wow this is great! Thanks for writing such a clear summary.

Re: v6
We should definitely discuss further what is worth continuing with given the planned upgrade to version 6. Even without support for pairs/triples it could be worthwhile to implement, depending on effort. I think the 'plain' version of bracket matching already provides the bulk of the UX improvement. How much work do think it is to install the add-on without any modification? Is it something that can quickly be evaluated once the test instance is up and running? I would guess that our eventual scoping decision-making will depend on what the planned timeline is for updating to v6 -- if it makes sense to invest any effort now or just focus on it later after the other template-related projects.

Re: highlighting from the middle
This is good to know. I'm not sure if it's worth the effort if we have to limit the search radius. The usefulness of the feature actually increases the farther away the brackets are from each other, it's the exact situation where it's useful to have them highlighted. If it's quick to test, then it would be great to better understand the performance impact. If it is high, then I think we should turn to an alternate solution - improving the background /nesting highlighting. This is already something I had planned to look at and develop options for, so I'll keep it in mind to have an option without bracket matching.

[…] the 'plain' version of bracket matching already provides the bulk of the UX improvement.

I agree.

[…] can quickly be evaluated once the test instance is up and running?

Yes, I believe so.

highlighting from the middle […] If it's quick to test […]

I believe it will be fairly easy to fork the existing bracket matching add-on, modify it as described, and test it.

1 for trying the add-on, to see if it's helpful to deploy this as a first iteration.

Great! Then I would make two tickets answer these questions.

  1. Use test instance to understand scope of installing the bracket matching add-on without modification (note, ideally would also include tag matching add-on)
  2. Use test instance to understand the performance impacts of highlighting brackets from the middle of a large section.

On a separate note - I would guess though that since we have a lot to do in the template topic area, doing any work twice doesn't make much sense. Probably we will just work on VE first, and template syntax last. Even given this though, I think these investigations would still be helpful in making that decision and scoping the eventual work.

I did another quick review of the existing matchbrackets add-on.

  • There is a config maxHighlightLineLength, set to 1000 characters by default. Not only will any bracket matching be disabled in long lines. When the content between a pair of brackets contains a long line anywhere, the detection starts to behave bogus.
    • I checked, and even if templates often contain long lines, it's extremely rare to have >1000 characters.
  • I can't spot an obvious flaw in the bracket matching code. Nothing related to | characters, for example. The detection uses a stack to skip nested brackets, which sounds exactly how it should be done. It will go crazy when brackets are "interleaved", e.g. ([)], but that's expected.
  • I have not seen the bracket matching not working. All the examples discussed (e.g. this) work for me.
  • I have not seen the bracket matching not working. All the examples discussed (e.g. this) work for me.

Please see these examples.

Uh, thanks!

  • Is there a modified bracketRegex config?
  • Some of the glitches look like conflicts with the tag matching add-on.

I believe this is a conflict with how the Wikitext syntax highlighter is implemented (called the "mediawiki mode" in the code). I mean, this is the only code that does something special with pairs of {{, right? It looks like the bracket matching plugin assumes it gets brackets as individual characters, but get's {{ as a single token instead because of how the custom mediawiki mode is implemented. This seems to mess everything up in the add-on.

In other words: I believe we have to kind of rewrite the bracket matching add-on to make it compatible with Wikitext syntax highlighting. The existing code is very short, so it should not be terribly hard to change it. If it works, we might get highlighting for doubles and triples "for free"!

Uh, thanks!

  • Is there a modified bracketRegex config?
  • Some of the glitches look like conflicts with the tag matching add-on.

I believe this is a conflict with how the Wikitext syntax highlighter is implemented (called the "mediawiki mode" in the code). I mean, this is the only code that does something special with pairs of {{, right? It looks like the bracket matching plugin assumes it gets brackets as individual characters, but get's {{ as a single token instead because of how the custom mediawiki mode is implemented. This seems to mess everything up in the add-on.

In other words: I believe we have to kind of rewrite the bracket matching add-on to make it compatible with Wikitext syntax highlighting. The existing code is very short, so it should not be terribly hard to change it. If it works, we might get highlighting for doubles and triples "for free"!

Good find! Feel free to disable the tag matching addon, I'm pretty sure it didn't change the bracket matcher's behavior but toggling is easy enough: comment out line 130 here, https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CodeMirror/ /618536/5/resources/ext.CodeMirror.js

Great news if we already get pairs of brackets, IMO. I believe that you can tweak the regex by setting matchTags: {bracketRegex: "..."}.

I'm almost 100% sure the issue is what I described above. Tweaking the regex is necessary, yes, but probably not enough. I would like to actually work on this later after we made some more decisions.

Change 620657 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/CodeMirror@master] Remove confusing matchbrackets error checking in mediawiki mode

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

Change 620657 abandoned by Thiemo Kreuz (WMDE):
[mediawiki/extensions/CodeMirror@master] Remove confusing matchbrackets error checking in mediawiki mode

Reason:
Now part of Ib01d991.

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