Page MenuHomePhabricator

When editing source, sometimes characters get put in strange places if you have too many characters on a line (due to space collapsing)
Open, HighPublic1 Estimated Story Points

Description

Reproduction steps:

  1. Open a blank page for source editing, with your surface width being 1663 pixels - this is what I encountered it on using Ubuntu 16.04 Unity on a 1920x1080 screen and the Vector skin, I suspect this problem may be likely to show up relative to surface width? I could be wrong about this
  2. Copy+paste this in (via a text editor), or type some other string of characters of the same length:
    1. a a
  3. Type 3 spaces
  4. Type another character - I used 'a'

What I would expect to happen:
My cursor should still be at the end and crazyness should not occur

Instead:
The cursor is before the last character I typed:

image.png (47×1 px, 1 KB)

That's the first thing I notice that goes wrong, but really if you keep typing stuff you see it's really just where the madness begins. I've managed to do things like get characters stuck after the end of the document where you can no longer remove them, overwrite one character and it changes a different one, etc.

The visual editor appears to handle this okay, or at least I've not seen it break during use yet.

Event Timeline

This makes NWE borderline unsuable - there are phantom characters that don't show up in the diff, there are apparent spaces at the end of the line which are not actually there etc.

I think this started happening this week, I don't remember encountering it before but now it happens fairly frequently.

Can't reproduce with a plain text paste, but it sounds a lot like T182402, where the cursor ends up in the wrong part of the DOM after a paste.

I can reproduce it without pasting at all, I just fill the line with some character ('a'), press space three times, and then it breaks when I put another character in.

More typically, the cursor ends up where I'd expect it and the characters elsewhere. E.g. I have

foo|
next line

in the edit box (| being the cursor) and type bar, and end up with

foo   |
barnext line

but saving the page results in

foobar
next line

like it should.

(Not sure if this is the same bug @Krenair is thinking about; it sounds similar but not obviously related to line length in my experience.)

Deskana subscribed.

I can't reproduce this bug exactly, but the problem did manifest for me in a slightly different way. The cursor was about half a screen width off from where the characters I was typing ended up. In the below screenshot, anything I type next ends up after the @@£@!£!@!444 despite the cursor being well into the left half of the screen.

Screen Shot 2018-03-06 at 19.51.06.png (348×2 px, 58 KB)

Deskana set the point value for this task to 1.Mar 6 2018, 7:53 PM
Deskana moved this task from To Triage to Current work on the VisualEditor board.
Deskana edited projects, added VisualEditor (Current work); removed VisualEditor.

Not sure if this is the same bug @Krenair is thinking about; it sounds similar but not obviously related to line length in my experience.

Yeah, it's entirely possible that you can trigger this in other ways. I just attempted to replicate the way in which I originally encountered this.

I have experienced this same (or similar) issue in the Basque wikipedia in the last 2-3 weeks, only when using New Wikitext Mode. It can happen in very different ways, so I couldn't find a pattern at all. It is more usual when I copy a text, and very usual if I have a wikilink open with [[ but not closed it yet. And I think that it could be more frequent with the syntax highlighter turned on.

Oh, I can reproduce what might be the same problem, or at least related: I think it's to do with browser rules for wrapping spaces.

Steps to reproduce:

  1. In Chrome/Chromium, open VE standalone (e.g. https://rawgit.com/wikimedia/VisualEditor/master/demos/ve/mobile.html#!pages/empty.html ).
  2. Click "Show model" and "Update on changes".
  3. Press 'x', then press Space repeatedly until the cursor has crossed the screen. Observe that the following happens with each key press:
    1. The on-screen cursor visibly advances.
    2. The model selection (shown where it says "Linear: 1-1") advances by one.
    3. Another green box is inserted in "Linear model data".
  4. When the cursor has crossed the screen, press Space again a few times.

Expected behaviour: The line of spaces wraps to a second line, and the on-screen cursor moves to the start of the second line and continues advancing.

Actual behaviour: The on-screen line does not wrap, and the cursor remains in the same place.

However, you can see that the model selection continues to advance, and the green boxes get inserted into "Linear model data". Furthermore, if you then type 'y' (or any non-whitespace character), the line wraps. In fact this is because of browser wrapping rules - you can see exactly the same issue if you follow the steps above after opening this data URL:

data:text/html,<h1 contenteditable style="white-space: pre-wrap">x

A hotfix for the problem I describe is to open the console (CTRL+SHIFT+i), then type

$( '.ve-ce-surface' ).css( 'word-break', 'break-all' )

then press <ENTER>, then close the console (CTRL+SHIFT+i again).

Obviously this has the drastic side effect of making line breaking happen within words, But could you check whether you can still reproduce the issues you describe after applying that hotfix? If the hotfix fixes your issues, then they're probably due to whitespace wrapping or at least they interact with it.

I can reproduce this, and I think @dchan's explanation is correct. So the problem is that with white-space: pre-wrap, sequences of multiple spaces at the end of a line are collapsed. I think we can proceed with this assumption.

(Adding word-break: break-all; helps on Chrome, but not on Firefox.)

In the draft spec https://drafts.csswg.org/css-text-3/#propdef-white-space there is apparently a white-space: break-spaces property, which would be exactly what we need:

break-spaces
The behavior is identical to that of pre-wrap, except that:

  • Any sequence of preserved white space always takes up space, including at the end of the line.
  • A line breaking opportunity exists after every preserved white space character, including between white space characters.

But it seems like a newly proposed addition, because this is the first time I've heard of it and I'm pretty sure I've needed to check the spec before. It's not even listed on https://developer.mozilla.org/en-US/docs/Web/CSS/white-space yet, and not supported by Chrome or Firefox.

In another draft spec https://drafts.csswg.org/css-text-4/#white-space-collapsing this is called text-space-collapse: preserve. No idea which of these might be closer to standardization and implementation.


One workaround to this would be to use &nbsp; for every other space. (Effectively, reverting rGVEDc6b338ad252c: Use white-space: pre-wrap instead of using &nbsp; for spaces.) That would cause T96487 again though. Maybe we could enable that for NWE, but not VE?

I agree with @matmarex 's proposal that we shouldn't revert to the &nbsp; behaviour for visual editing (because we want nbsp to represent actual non-breaking spaces). It's a different issue in wikitext because non-breaking spaces are inferred heuristically.

@matmarex , do you want to go forward with the solution you suggested? (I.e. use &nbsp; for every other space, on NWE only)

I've just thought that we could also do the opposite and change CodeMirror to collapse spaces like a browser would. Apparently, it currently prevents that using exactly the approach I suggested for VE, by changing some spaces to nbsp… (see function splitSpaces() in codemirror.js).

CodeMirror has no option to control this, and I don't want to bother upstream with patches to add more options, nor maintain a local hack. And, on second thought, preserving rather than collapsing the space is clearly the better behavior for a code editor, so let's try to do that.

However, I proposed a CodeMirror patch to enable this behavior for Firefox as well: https://github.com/codemirror/CodeMirror/pull/5549 (otherwise, we'd have to do browser detection in our code to match it).

As a side note, this means that this issue can only occur on browsers other than Firefox (e.g. Chrome, IE, Edge). If anyone thought you experienced this in Firefox, then it's probably a different bug and we should file a task and investigate that separately…

I have this issue and I use Firefox.

2018 abu. 28 11:16 PM erabiltzaileak hau idatzi du (matmarex <no-reply@phabricator.wikimedia.org>):
matmarex added a comment.

CodeMirror has no option to control this, and I don't want to bother upstream with patches to add more options, nor maintain a local hack. And, on second thought, preserving rather than collapsing the space is clearly the better behavior for a code editor, so let's try to do that.

However, I proposed a CodeMirror patch to enable this behavior for Firefox as well: https://github.com/codemirror/CodeMirror/pull/5549 (otherwise, we'd have to do browser detection in our code to match it).

As a side note, this means that this issue can only occur on browsers other than Firefox (e.g. Chrome, IE, Edge). If anyone thought you experienced this in Firefox, then it's probably a different bug and we should file a task and investigate that separately…

TASK DETAIL
https://phabricator.wikimedia.org/T188839

EMAIL PREFERENCES
https://phabricator.wikimedia.org/settings/panel/emailpreferences/

To: matmarex
Cc: matmarex, Theklan, Deskana, Esanders, dchan, Tgr, Krenair, Aklapper, iamjessklein, marcella, Necroarcano, Robinma, merbst, Wess, Dvorapa, Lynhg, Srdjan_m, Jrf, Husun1297, jeblad, Swainr, TheDJ, Jay8g

Change 456053 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[VisualEditor/VisualEditor@master] [BROKEN] ve.ce.TextNode: Prevent browsers from collapsing trailing spaces in source mode

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

I tried to implement this approach (use &nbsp; for every other space), but overlooked something obvious in hingsight: our node-rendering code doesn't run on the node that you're currently typing in (to avoid breaking IMEs), so the hack doesn't work when you're typing/backspacing space characters at end of line (it works when they already exist on the page).

@dchan Maybe we could re-render the node only for every keypress that inserts or removes a space? If that sounds like something we could safely do, then I'll need you to implement it, because I couldn't figure out how to make it happen. :)

If we can't do that, then I guess we can try to change CodeMirror to collapse spaces normally.

matmarex renamed this task from When editing source, sometimes characters get put in strange places if you have too many characters on a line (?) to When editing source, sometimes characters get put in strange places if you have too many characters on a line (due to space collapsing).Aug 29 2018, 12:30 AM

@matmarex Ohhh that's a really good point. And unfortunately you're right to be suspicious; some IMEs can add / remove whitespace without committing all candidate text so we can't safely rerender.

Your suggested CodeMirror approach sounds like it could be an idea, but I'm not really the best person to judge.

Change 456053 abandoned by Bartosz Dziewoński:
[BROKEN] ve.ce.TextNode: Prevent browsers from collapsing trailing spaces in source mode

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

Change 456198 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/CodeMirror@master] Make CodeMirror collapse spaces normally (never "split spaces")

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

Change 456198 abandoned by Bartosz Dziewoński:
Make CodeMirror collapse spaces normally (never "split spaces")

Reason:
This should probably be implemented as an option, and should probably be submitted upstream, and I don't really have time to work on this in the next few weeks. The bug is also not encountered very often in real articles (there are some worse ones too…). If anyone wants to work on this, please feel free.

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

matmarex edited projects, added VisualEditor; removed VisualEditor (Current work).