Change #1064419 merged by jenkins-bot:

[mediawiki/extensions/CodeMirror@wmf/1.43.0-wmf.19] ve.ui.CodeMirrorAction.v6: use infinity viewport to avoid misalignment

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

Mentioned in SAL (#wikimedia-operations) [2024-08-21T20:16:30Z] <cjming@deploy1003> Started scap sync-world: Backport for [[gerrit:1064419|ve.ui.CodeMirrorAction.v6: use infinity viewport to avoid misalignment (T357482)]]

Mentioned in SAL (#wikimedia-operations) [2024-08-21T20:21:11Z] <cjming@deploy1003> musikanimal, cjming: Backport for [[gerrit:1064419|ve.ui.CodeMirrorAction.v6: use infinity viewport to avoid misalignment (T357482)]] synced to the testservers (https://wikitech.wikimedia.org/wiki/Mwdebug)

Mentioned in SAL (#wikimedia-operations) [2024-08-21T20:29:44Z] <cjming@deploy1003> Finished scap sync-world: Backport for [[gerrit:1064419|ve.ui.CodeMirrorAction.v6: use infinity viewport to avoid misalignment (T357482)]] (duration: 13m 14s)

@MusikAnimal I am seeing some examples of cursor misalignment in templates and links on https://ar.wikipedia.beta.wmflabs.org/w/index.php?title=%D9%85%D8%AA%D8%B7%D9%84%D8%A8%D8%A7%D8%AA_%D8%A7%D9%84%D8%AD%D8%B5%D9%88%D9%84_%D8%B9%D9%84%D9%89_%D8%AA%D8%A3%D8%B4%D9%8A%D8%B1%D8%A9_%D9%84%D9%85%D9%88%D8%A7%D8%B7%D9%86%D9%8A_%D8%A8%D8%B1%D9%88%D9%86%D8%A7%D9%8A&veaction=editsource&cm6enable=1

Example showing the cursor in front of the W but text appearing behind it:

@dom_walden I can't repro that, but I don't doubt issues like this will pop up, as they have been since CodeMirror was added to the 2017 editor years ago. https://phabricator.wikimedia.org/maniphest/query/6Wt2PShel45J/#R

I will try to repro further, but I wanted to share this trick with you that will make the VE surface text display:

$(".ve-ce-documentNode-codeEditor-webkit-hide").css('-webkit-text-fill-color', 'black')

Run that in your JS console as you do your testing, and it should be easier to spot any instances of misalignment.

Investigating T373152, I realized that this is leaking a CSS rule to non-codemirror VE surfaces when v6 is enabled on beta. ve.ui.CodeMirror.v6.less does:

.ve-init-mw-desktopArticleTarget {
...
	.mw-content-ltr .ve-ce-paragraphNode {
		margin-left: 6px !important; /* stylelint-disable-line declaration-no-important */
	}

	.mw-content-rtl .ve-ce-paragraphNode {
		/* @noflip */
		margin-right: 6px !important; /* stylelint-disable-line declaration-no-important */
	}
...
}

It should probably be scoping this in some way so it doesn't affect the top-level non-codemirrored VE surface.

In T357482#10085614, @MusikAnimal wrote:

Run that in your JS console as you do your testing, and it should be easier to spot any instances of misalignment.

Thanks. I have found quite a few examples of misalignment on https://ar.wikipedia.beta.wmflabs.org/w/index.php?title=%D8%A7%D9%84%D8%B9%D8%B5%D9%88%D8%B1_%D8%A7%D9%84%D9%88%D8%B3%D8%B7%D9%89&veaction=editsource&cm6enable=1

Note, I only see the issue on Chrome (Chromium 120 locally and Chrome 128 on Windows 11).

I tried white-space: break-spaces following T347902 in my browser, and it solved the misalignment problem for me.

In T357482#10095727, @dom_walden wrote:

Thanks. I have found quite a few examples of misalignment on https://ar.wikipedia.beta.wmflabs.org/w/index.php?title=%D8%A7%D9%84%D8%B9%D8%B5%D9%88%D8%B1_%D8%A7%D9%84%D9%88%D8%B3%D8%B7%D9%89&veaction=editsource&cm6enable=1

F57303745

Note, I only see the issue on Chrome (Chromium 120 locally and Chrome 128 on Windows 11).

That is bizarre. I can see from the screenshot that each line starts at the same point, but evidently the character width is a tiny bit different so the end of each line (on the left) is misaligned.

In T357482#10102531, @Bhsd wrote:

I tried white-space: break-spaces following T347902 in my browser, and it solved the misalignment problem for me.

Were you able to reproduce the same issue Dom found above? You both are reporting to use Windows, so maybe that's why I can't reproduce it.

Regardless, I see r963065 (which adds white-space: break-spaces) was never ported to CM6, so I've done so with r1068888.

I have also discovered a new misalignment issue: If you take a large page (100+ lines long) and use ⌃+A to select all content, and ⌃+X to cut it, then paste it back, the change in the gutter width isn't accounted for. I've filed T373649 for that.

In T357482#10104945, @MusikAnimal wrote:

Were you able to reproduce the same issue Dom found above? You both are reporting to use Windows, so maybe that's why I can't reproduce it.

Yes, and it is fixed by styling the CM6 editor font-style: normal !important, similar to the font weight.

In T357482#10105280, @Bhsd wrote:

Were you able to reproduce the same issue Dom found above? You both are reporting to use Windows, so maybe that's why I can't reproduce it.

Yes, and it is fixed by styling the CM6 editor font-style: normal !important, similar to the font weight.

Awesome! So that's a CSS rule that needs to be added? Would you mind submitting a patch? If both you and Dom can verify the fix, I am happy to +2, as I have no means of testing it.

In T357482#10107235, @MusikAnimal wrote:

Would you mind submitting a patch?

Sorry, I won't have a coding device for some time. I am currently borrowing a public computer and playing with the browser developer tools.

I think it can be just one more line together with the font weight.

In T357482#10107235, @MusikAnimal wrote:

In T357482#10105280, @Bhsd wrote:

Were you able to reproduce the same issue Dom found above? You both are reporting to use Windows, so maybe that's why I can't reproduce it.

Yes, and it is fixed by styling the CM6 editor font-style: normal !important, similar to the font weight.

Awesome! So that's a CSS rule that needs to be added? Would you mind submitting a patch? If both you and Dom can verify the fix, I am happy to +2, as I have no means of testing it.

I added this to my global.css (which I hope is equivalent to the patch):

.ve-init-mw-desktopArticleTarget .cm-scroller, .ve-init-mw-desktopArticleTarget .cm-scroller *, .ve-init-mw-desktopArticleTarget .ext-codemirror-wrapper .ve-ce-paragraphNode {
	font-style: normal !important;
}

I think it has improved things on some articles and for some versions of Chrome, but hasn't completely eliminated misalignment (below screenshot for this article on Chrome 128 Windows 11).

Before CSS change:

After:

But, I have still seen examples of significant misalignment. See example of this article on Chrome 128.

It is hard to test because different versions of Chrome behave differently in this respect. I have also seen different behaviour depending on screen size, whether or not you resize your screen while using the 2017 Editor, and probably lots of other factors (e.g. what happens when you actually make edits, zoom in and out, etc.). I have yet to spend time testing other browsers systematically.

I have found examples of misalignment on Safari 17 and 16 as well. So far, nothing as bad as Chrome.

I have found some bad misalignment on Firefox 129, but only when I zoom in or out.

Example at 120% zoom:

I am surprised to see that white-space: pre-wrap is performing better than white-space: break-spaces in certain conditions. By the way, I notice that the misalignment also appears when switching between LTR and RTL.

Change #1070313 had a related patch set uploaded (by MusikAnimal; author: MusikAnimal):

[mediawiki/extensions/CodeMirror@master] ve.ui.CodeMirror.v6: force a normal font-style

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

I have also seen different behaviour depending on screen size, whether or not you resize your screen while using the 2017 Editor, and probably lots of other factors (e.g. what happens when you actually make edits, zoom in and out, etc.). I have yet to spend time testing other browsers systematically.

As have I. I found http://localhost:8080/w/index.php?title=BN2&veaction=editsource a particularly interesting example. There's a template argument name in there সংগ্রহের-তারিখ that contains a hyphen (-). If the word hugs the right edge, sometimes it gets line-wrapped just after the hyphen, other times the hyphen is included as part of the word that is line wrapped -- all depending on your browser, screen size, etc. Using hyphen: auto fixes it sometimes, other times the current rule hyphen: manual fixes it. It seems there's just too much being left to the browser for this "CodeMirror on top of invisible surface" thing to work reliably :(

I think we may just have to accept a degree of misalignment given the way this was implemented. I left a related rant at T184857#10083166.

e1070313 applies the font-style: normal as requested.

@dom_walden @Bhsd I think may be enough for this task, unless you're seeing a lot misalignment compared to CodeMirror 5? (You'll need to test CM5 on a production wiki like enwiki).

I've already scheduled our deployment to group0 for next week, so please inform me if you find anything that seems like a show-blocker (i.e. very broken and not broken in CM5). There is T373649: Copying and pasting large pages causes misalignment between CodeMirror and VE surfaces which I'm looking into today.

Change #1070313 merged by jenkins-bot:

[mediawiki/extensions/CodeMirror@master] ve.ui.CodeMirror.v6: force a normal font-style

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

In T357482#10114702, @MusikAnimal wrote:

@dom_walden @Bhsd I think may be enough for this task, unless you're seeing a lot misalignment compared to CodeMirror 5? (You'll need to test CM5 on a production wiki like enwiki).

I've already scheduled our deployment to group0 for next week, so please inform me if you find anything that seems like a show-blocker (i.e. very broken and not broken in CM5). There is T373649: Copying and pasting large pages causes misalignment between CodeMirror and VE surfaces which I'm looking into today.

I have seen examples of beta being worse than production. For example, this article on beta compared to production (I imported the production article to beta to make sure the content was the same. Of course, beta and production might be different in other ways.)

Production:

Beta:

I have tried to test on ar.wikipedia.org and ja.wikipedia.org (two sites whose beta versions showed alignment issues) but I could not enable syntax highlighting.

In T357482#10117523, @dom_walden wrote:

In T357482#10114702, @MusikAnimal wrote:

@dom_walden @Bhsd I think may be enough for this task, unless you're seeing a lot misalignment compared to CodeMirror 5? (You'll need to test CM5 on a production wiki like enwiki).

I've already scheduled our deployment to group0 for next week, so please inform me if you find anything that seems like a show-blocker (i.e. very broken and not broken in CM5). There is T373649: Copying and pasting large pages causes misalignment between CodeMirror and VE surfaces which I'm looking into today.

I have seen examples of beta being worse than production. For example, this article on beta compared to production (I imported the production article to beta to make sure the content was the same. Of course, beta and production might be different in other ways.)

Thanks. I see one difference is line numbering. T373649 might fix any issues with that (even if you're not just seeing the misalignment when copying/pasting).

I've been able to reproduce some misalignment in Sauce Labs, but it appeared to be a result of the same issue described at T357482#10114702, where a line wrap happens at a hyphen in CodeMirror, but not in the VE surface. Could you scroll up a bit on Beta and see if the issue is also due to line breaks and hyphens for you?

I'll keep debugging in Sauce Labs in hopes I figure something out.

I have tried to test on ar.wikipedia.org and ja.wikipedia.org (two sites whose beta versions showed alignment issues) but I could not enable syntax highlighting.

Indeed CodeMirror 5 is not available on RTL wikis. You should be able to test on ja.wikipedia.org though?

In T357482#10119540, @MusikAnimal wrote:

I've been able to reproduce some misalignment in Sauce Labs, but it appeared to be a result of the same issue described at T357482#10114702, where a line wrap happens at a hyphen in CodeMirror, but not in the VE surface. Could you scroll up a bit on Beta and see if the issue is also due to line breaks and hyphens for you?

I see differences in line breaks, but they are not always obviously related to hyphens.

For example, this article:

CM6:

VE surface:

Same article, CM6:

VE surface:

It is almost as if CM6 isn't wide enough.

I have tried to test on ar.wikipedia.org and ja.wikipedia.org (two sites whose beta versions showed alignment issues) but I could not enable syntax highlighting.

Indeed CodeMirror 5 is not available on RTL wikis. You should be able to test on ja.wikipedia.org though?

OK, it is working now on jawiki. For some reason I couldn't get it to working yesterday.

Using the same article that dom_walden mentions, I see misalignment with hyphen-related line wrapping on L174, and the problem is only present in CM6.

Okay, I'm going to postpone deployment, then. It sounds like these issues need to be fixed, first.

In T357482#10112821, @Bhsd wrote:

... By the way, I notice that the misalignment also appears when switching between LTR and RTL.

Thanks. I raised T374196.

I have been experimenting with this CSS in my global.css, and so far it has improved things (but not completely fixed things):

.ve-init-mw-desktopArticleTarget .cm-editor .cm-gutters {
	width: 38px;
}

In T357482#10121607, Bhsd wrote:

Using the same article that dom_walden mentions, I see misalignment with hyphen-related line wrapping on L174, and the problem is only present in CM6.

In T357482#10120694, dom_walden wrote:

I see differences in line breaks, but they are not always obviously related to hyphens.
…
It is almost as if CM6 isn't wide enough.

I am only seeing hyphen-related issues, both on Firefox and to a worse degree on Chrome. On my local after making a small code change, I can confirm the CM6 and VE contenteditables are exactly the same, yet the issue persists :(

For me, it almost seems like the two contenteditables are running by different rules as to how they interpret hyphens: manual.

In T357482#10124350, @dom_walden wrote:

I have been experimenting with this CSS in my global.css, and so far it has improved things (but not completely fixed things):

.ve-init-mw-desktopArticleTarget .cm-editor .cm-gutters {
	width: 38px;
}

The gutter width is variable (say there are 10 lines, then you add 90 more, now the gutter has to account for the additional digit in 100). Currently we set the margin on the VE surface to account for gutter width on every transaction, and we account for LTR/RTL. That's supposed to keep things aligned and sized properly.

This is so frustrating because I'm not able to repro most all of the issues you two are seeing (I only get hyphen-related misalignment).

I think what I will do next is attempt to faithfully replicate the same approach the old 2017 editor module used, where we use a special "padding" gutter. I don't know that this will help, but we'll at least be dealing with a DOM structure more similar to before, which hopefully will make it easier to spot what we're doing wrong when comparing to CM5.

I'd also like to invite @Esanders to this task now that he is back from sabbatical. He authored the original integration, so maybe he'll have some helpful insight. To Ed: rest assured that I am keeping my promise of not shipping a broken 2017 editor :)

Change #1085479 had a related patch set uploaded (by MusikAnimal; author: MusikAnimal):

[mediawiki/extensions/CodeMirror@master] ve.ui.CodeMirror.v6: don't auto-flip CSS rules affecting gutter

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

Change #1085479 merged by jenkins-bot:

[mediawiki/extensions/CodeMirror@master] ve.ui.CodeMirror.v6: don't auto-flip CSS rules affecting gutter

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

Change #1087618 had a related patch set uploaded (by MusikAnimal; author: MusikAnimal):

[mediawiki/extensions/CodeMirror@master] ve.ui.CodeMirror.v6: use DOMRect width when updating gutter width

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

Change #1087618 merged by jenkins-bot:

[mediawiki/extensions/CodeMirror@master] ve.ui.CodeMirror.v6: use DOMRect width when updating gutter width

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

@dom_walden @Bhsd Could I get you to test the scenarios where you saw misalignment once more? It for sure isn't completely resolved, but r1087618 should have helped.

The general issue of misalignment will probably never be completely resolved. I was able to reliably reproduce misalignment in CM5 as well. The Editing team has nonetheless given the go-ahead on the implementation, but we're going to try to automatically detect misalignment and disable CodeMirror entirely if it happens repeatedly (T379104).

The misalignment I reported earlier has been fixed.

In T357482#10301400, @MusikAnimal wrote:

@dom_walden @Bhsd Could I get you to test the scenarios where you saw misalignment once more? It for sure isn't completely resolved, but r1087618 should have helped.

I attempted to reproduce issues seen in T357482#10076432, T357482#10083880, T357482#10095727, T357482#10111614 and T357482#10120694.

I have detected some misalignment on

• https://ar.wikipedia.beta.wmflabs.org/w/index.php?title=%D8%A7%D9%84%D8%B9%D8%B5%D9%88%D8%B1_%D8%A7%D9%84%D9%88%D8%B3%D8%B7%D9%89&veaction=editsource&cm6enable=1
• https://bn.wikipedia.beta.wmflabs.org/w/index.php?title=%E0%A6%AB%E0%A7%87%E0%A6%B8%E0%A6%AC%E0%A7%81%E0%A6%95&cm6enable=1&veaction=editsource

Change #1121755 had a related patch set uploaded (by MusikAnimal; author: MusikAnimal):

[mediawiki/extensions/CodeMirror@master] CodeMirrorVisualEditor: new subclass for the 2017 editor integration

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

Change #1121755 merged by jenkins-bot:

[mediawiki/extensions/CodeMirror@master] CodeMirrorVisualEditor: new subclass for the 2017 editor integration

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

Change #1123919 had a related patch set uploaded (by MusikAnimal; author: MusikAnimal):

[mediawiki/extensions/CodeMirror@master] ve.ui.CodeMirrorAction.v6: disable inapplicable MW-specific Extensions

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

Change #1123919 merged by jenkins-bot:

[mediawiki/extensions/CodeMirror@master] ve.ui.CodeMirrorAction.v6: disable inapplicable MW-specific Extensions

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

Just found a patch connected to this task https://gerrit.wikimedia.org/r/c/mediawiki/skins/MinervaNeue/+/1064115
Please note for the future (cc: @MusikAnimal), not to set font sizes (or line-heights) to px values. We go above and beyond in other parts of the interface to get rid of those, as they are bad inclusion stoppers. See T385857: Respect user agent font size setting via relative CSS font-size units (in Wikimedia deployed extensions/projects) for more on this.
I will provide a patch to rely on rem instead with same value.

Change #1134092 had a related patch set uploaded (by VolkerE; author: VolkerE):

[mediawiki/skins/MinervaNeue@master] styles: Set VisualEditor + CodeMirror 6 relative unit explicit font-size

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

Change #1134092 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] styles: Set VisualEditor + CodeMirror 6 relative unit explicit font-size

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