Pill mentions can break when editing trailing colon

[akien-mga]

Description

After inputting a pill mention (start typing user name and tab-complete it), resulting in e.g.

if you attempt to edit the trailing colon to change it e.g. in a comma using backspace, you may end up breaking the pill and have instead:

I'll describe the exact steps to reproduce below. Note that it's only one breaking case, I've experienced similar (and more blocking, needing a refresh of Riot) issues in slightly different scenarios for which I haven't debugged the exact steps to reproduce yet. I may describe them later on here or in a new issue when I come to that.

Steps to reproduce

1. Tab complete a username into a pill mention, e.g. type red<TAB> and get it completed as:
   
   Note that the cursor is two columns after the pill, preceded by a colon and a space.
2. Press backspace twice, to delete the colon and space and be right next to the pill:
   
   Note that the cursor is not next to the pill as expected, but inside it. (*)
3. Input a comma:
   
   Note that the comma is placed after the cursor and the pill, and the cursor stays inside the pill as if expecting the user to append to the username.
4. Input a space, everything breaks:
   

(*) One could expect that the cursor is inside the pill to allow appending to the username (e.g. to mention @reduzio instead of @reduz, but in all evidence it does not work, as the first added character is inputted after the pill without moving the cursor, and the second one breaks the pill:
2. (a). Input i:

2. (b). Input o:

Also worth mentioning, waiting long enough (5 s) between keystrokes allows to continue inputting stuff according to the 2. (a) logic.

Version information

• Platform: web (in-browser)

For the web app:

• Browser: Firefox Nightly 20170823, Chrome 60.0.3112.90
• OS: Mageia 6 x86_64 (Linux), Windows 7
• URL: riot.im/develop as of 24.08.2017

[lampholder]

On my machine:

On Chrome 60.0.3112.78 on Ubuntu:

• the cursor goes 'into the pill'
• pressing , then space doesn't break the pill

On FF Nightly on Ubuntu:

• the cursor does not go into the pill
• pressing , then space doesn't break the pill

[akien-mga]

Thanks for testing, I'll have to recheck on Nightly - I wrote these steps to reproduce based on Chrome behaviour, and I remember having similar pill issues on Nightly, but maybe the exact behaviour differs.

[lukebarnard1]

This is probably because there's an "R" for the avatar of this person. Draft-js has a bug where it expects the text inside the pill to be exactly the text it decorates - #4772. This leads to all sorts of exciting misbehaviour.

[akien-mga]

I checked again on FF Nightly on Mageia 6 x86_64 (Linux), and the bug described above is worse. With the exact same steps to reproduce, the final output is:

And the text editor is then completely broken:

• The weird Rreduz ,, Rreduz ,, cannot be deleted
• Text can be inputted to the left of the existing text, but cannot be deleted either:
  
• Only a reload of the browser tab can fix it in my tests, leading to a delete-able Help!! Rreduz ,, (notice the absence of the second instance of this pill leftover).

The above actually covers what I had mentioned in the OP:

Note that it's only one breaking case, I've experienced similar (and more blocking, needing a refresh of Riot) issues in slightly different scenarios for which I haven't debugged the exact steps to reproduce yet. I may describe them later on here or in a new issue when I come to that.

[lukebarnard1]

A workaround for this could be to not have any text in the member avatar in pills. I'm not sure what we'd put there instead. Maybe no avatar? Maybe a placeholder image?

[eternaleye]

Could the issue be sidestepped by using an SVG "image" containing the text?

[ara4n]

This bug is super-annoying (as per #5002 - @turt2live thanks for spotting the dup :)
I suspect @eternaleye's solution is a fine workaround: i.e. rather than putting a <span>R</span> in the avatar, we can put a <svg><text>R</text></svg> instead, and hopefully Draft.js won't overenthusiastically try to parse text out of the SVG.

[eternaleye]

@ara4n: Even if it does, there's always <img src="data:..."></img> as a fallback option. That'd definitely behave more like an actual avatar.

[grahamperrin]

Breakage with different symptoms:

https://vimeo.com/234174685

multiple issues affecting Riot

… around 01:18 on the timeline: …

[lukebarnard1]

<svg><text>R</text></svg>

SGTM

[lukebarnard1]

OK so it turns out that the SVG <text/> is still a text node in the DOM, it's not just rasterized once.

[eternaleye]

@lukebarnard1: What about an <img> with a data: URI?

[lukebarnard1]

@eternaleye this would strictly be an option. But we do lose the CSS, which means themeing won't work.

[eternaleye]

@lukebarnard1: I thought the SVG was just for the letter itself, though, not even the background? At that point, we don't "lose the CSS" any more than we do when a user has an avatar set...

[lukebarnard1]

By CSS I meant we lose the ability to apply a different colour to the initial through the existing theme mechanism.

[alex-mayorga]

¡Hola @ara4n!

This just bite me yet again on https://riot.im/develop/ and annoyed me enough to comment.

Any hopes for a fix or workaround to this bugger?

¡Gracias!

[rschulman]

This bit me just now on Firefox 57.0 (Quantum) as well. So just a +1.

[stormi]

+1 too

[ara4n]

fixed in matrix-org/matrix-react-sdk#1890