Page MenuHomePhabricator

List insertion by typing '#<space>', '*<space>' is broken
Closed, ResolvedPublic

Description

List insertion by typing # or * is broken. The typed text disappears, but a list number/bullet does not appear in its place.

(Technically, it does appear, but it has a slug inside, so it acts weird.)

Caused by https://gerrit.wikimedia.org/r/c/VisualEditor/VisualEditor/+/448068 (T199925).

[23:39]	<wikibugs_> (CR) Esanders: "Oops, this breaks the behaviour of the list generators: '* ' & '1. ' ('# ' in MW)." [VisualEditor/VisualEditor] - https://gerrit.wikimedia.org/r/448068 (https://phabricator.wikimedia.org/T199925) (owner: DLynch)
[23:39]	<edsanders> Kemayo: ^
[23:40]	<Kemayo> edsanders: yeah, I’d just noticed that as well. The change to containsContent, maybe. I’ll take a closer look.
[23:44]	<edsanders> Kemayo: blame said it was just that patch
[23:45]	<Kemayo> edsanders: I meant the change in that patch at the last minute to use containsContent rather than narrowly scoping it to paragraphs. I was sure I’d checked out some of the list before that. (But might be wrong.)
[23:45]	<Kemayo> *the list sequences
[23:46]	<edsanders> right
[23:46]	<edsanders> well list/listItem aren't contains content, but they do generate wrapper paragraphs inside
[23:47]	<Kemayo> I’m casting aspersions at my own testing procedures in response to feedback, mostly.
[23:51]	<edsanders> did someone say unit tests?
[23:54]	<edsanders> also I think you want .remove not .delete, the latter is for when you want the cursor to end up in a sane place, but this is a noAutoSelect fragment?
[00:04]	<edsanders> I think you may have to add another config param to the sequence, any attempt to guess what to do might just be too hacky
[00:18]	<MatmaRex> edsanders: Kemayo: my bad, i should have noticed it as well… shall we revert https://gerrit.wikimedia.org/r/c/VisualEditor/VisualEditor/+/448068 then, and backport that, or do you want to work on a fix now?

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change 451211 had a related patch set uploaded (by DLynch; owner: DLynch):
[VisualEditor/VisualEditor@master] ui.Sequence: fix post-sequence cleanup

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

Happy to apply this as a quick fix, however I think ultimately we should move the paragraph cleanup into the command or action code:

If I type abc<newline>{| I expect to get <p>abc</p><table>..., which your previous patch achieves.

However if I type abc<newline> then choose Insert>Table, I don't get the benefits of the newline cleanup, which I think you should.

In general, taking any action to insert a block level element into an empty paragraph should destroy that paragraph.

@Esanders: Makes sense, and I agree on the general case. If you merge it, I can make a new task for fixing it up more permanently.

Change 451211 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] ui.Sequence: fix post-sequence cleanup

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

Change 451530 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (7e57bf17b)

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

Change 451530 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (7e57bf17b)

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

I think we should backport this fix to wmf.16. On a closer look, it seems we actually place a text cursor inside the slug in the invisible list item, and you can type into it… I am not sure how broken this really is (maybe it's dumb but harmless), but it looks super broken.

Recording: https://drive.google.com/file/d/1zNdaJYDjPTmIn3C4XjleaPflEVOGUXlJ/view (too large for Phab)

Change 451543 had a related patch set uploaded (by Bartosz Dziewoński; owner: DLynch):
[VisualEditor/VisualEditor@wmf/1.32.0-wmf.16] ui.Sequence: fix post-sequence cleanup

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

Change 451543 merged by Reedy:
[VisualEditor/VisualEditor@wmf/1.32.0-wmf.16] ui.Sequence: fix post-sequence cleanup

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

Change 451545 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@wmf/1.32.0-wmf.16] Update VE core submodule to wmf/1.32.0-wmf.16

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

Change 451545 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@wmf/1.32.0-wmf.16] Update VE core submodule to wmf/1.32.0-wmf.16

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

Fixed and deployed to production wikis.

(…) I think ultimately we should move the paragraph cleanup into the command or action code:

If I type abc<newline>{| I expect to get <p>abc</p><table>..., which your previous patch achieves.

However if I type abc<newline> then choose Insert>Table, I don't get the benefits of the newline cleanup, which I think you should.

In general, taking any action to insert a block level element into an empty paragraph should destroy that paragraph.

@Esanders: Makes sense, and I agree on the general case. If you merge it, I can make a new task for fixing it up more permanently.

I also agree. Filed T201573.

Deskana triaged this task as Medium priority.Aug 17 2018, 9:28 AM