Pill mentions can break when editing trailing colon

akien-mga commented 7 years ago

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

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.
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. (*)
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.
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:

(a). Input i:
(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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 - https://github.com/vector-im/riot-web/issues/4772. This leads to all sorts of exciting misbehaviour.

akien-mga commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

ara4n commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

Breakage with different symptoms:

https://vimeo.com/234174685

multiple issues affecting Riot

… around 01:18 on the timeline: …

lukebarnard1 commented 7 years ago

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

SGTM

lukebarnard1 commented 7 years ago

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

eternaleye commented 7 years ago

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

lukebarnard1 commented 7 years ago

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

eternaleye commented 7 years ago

@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 commented 7 years ago

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

alex-mayorga commented 7 years ago

¡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 commented 6 years ago

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

stormi commented 6 years ago

+1 too

ara4n commented 6 years ago

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

element-hq / element-web