Page MenuHomePhabricator

Generalize logic for inserting a block level element into an empty paragraph (it should replace that paragraph, not insert before it)
Closed, ResolvedPublic

Description

Follow-up to T199925 and T201472.

(…) 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.

Event Timeline

matmarex created this task.Aug 9 2018, 12:44 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 9 2018, 12:44 AM

This probably should be low priority… but it was interesting, so I investigated it for a bit, and ended up with working patches… so might as well submit them.

Change 451552 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[VisualEditor/VisualEditor@master] ve.dm.Document: Replace empty nodes after inserting non-text data

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

Change 451553 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[VisualEditor/VisualEditor@master] ve.ui.Sequence: Revert sequence stripping changes

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

Change 451554 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[VisualEditor/VisualEditor@master] ve.ui.Sequence: Strip sequence before running the command

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

Change 451552 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] ve.dm.Document: Replace empty nodes after inserting non-text data

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

Change 451553 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] ve.ui.Sequence: Revert sequence stripping changes

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

Change 451554 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] ve.ui.Sequence: Strip sequence before running the command

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

Change 451880 had a related patch set uploaded (by Jforrester; owner: Jforrester):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (e2ce88ff1)

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

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

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

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

I observe that with the current patches, insert-table still leaves an empty paragraph (either via sequence or menu action).

I note that is true in MW, but not core.

It looks like ve.dm.MWTableNode.static.suggestedParentNodeTypes is the culprit. The fact 'paragraph' isn't in that list is probably causing the table to get moved down earlier on in the process (during fixupInsertion?)

Looks like this is still happening, and should not be in QA column. Moving it to "Up Next" column.

Change 458534 had a related patch set uploaded (by DLynch; owner: DLynch):
[mediawiki/extensions/VisualEditor@master] dm.MWTableNode: include paragraph in suggestedParentNodeTypes

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

Esanders assigned this task to DLynch.Sep 7 2018, 12:53 PM

Change 458534 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] dm.MWTableNode: include paragraph in suggestedParentNodeTypes

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

Deskana closed this task as Resolved.Oct 1 2018, 1:09 PM
Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptOct 1 2018, 1:09 PM