Page MenuHomePhabricator

Changing a table Content Cell into a Header Cell removes all cell content
Open, HighPublicBUG REPORT

Description

Steps to replicate the issue:

What happens?:

  • The content in that cell is gone.

image.png (663×1 px, 71 KB)

What should have happened instead?:

  • The content remained, and just the preceding | has been converted to a ! (or similar)

Event Timeline

Esanders added a project: Regression.

The culprit code is probably in ve.ce.MWTableNode.js.

That file only handles the rendering of the ContentEditable (CE) table node.

The problem here is how we are observing changes in the surface in general, and generating transactions.

Change #1032418 had a related patch set uploaded (by Esanders; author: Esanders):

[VisualEditor/VisualEditor@master] Failing test case for table cell conversion

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

Looks like our MutationObserver code can't handle more complex removal with nested nodes. Another way to trigger this bug is to try and convert a bullet list to a numbered list. The whole list is deleted.

Change #1032431 had a related patch set uploaded (by Esanders; author: Esanders):

[VisualEditor/VisualEditor@master] Revert "Use mutation observer to detect structural deletions"

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

Change #1032431 merged by jenkins-bot:

[VisualEditor/VisualEditor@master] Disable MutationObserver in ve.ce.Surface

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

Change #1032516 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (27296e0e3)

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

Change #1032516 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (27296e0e3)

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

Change #1032571 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/extensions/VisualEditor@wmf/1.43.0-wmf.5] Update VE core submodule to master (27296e0e3)

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

Change #1032571 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@wmf/1.43.0-wmf.5] Update VE core submodule to master (27296e0e3)

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

Mentioned in SAL (#wikimedia-operations) [2024-05-16T21:06:07Z] <jsn@deploy1002> Started scap: Backport for [[gerrit:1032571|Update VE core submodule to master (27296e0e3) (T230323 T365052)]]

Mentioned in SAL (#wikimedia-operations) [2024-05-16T21:08:49Z] <jsn@deploy1002> jsn and esanders: Backport for [[gerrit:1032571|Update VE core submodule to master (27296e0e3) (T230323 T365052)]] synced to the testservers (https://wikitech.wikimedia.org/wiki/Mwdebug)

Mentioned in SAL (#wikimedia-operations) [2024-05-16T21:31:18Z] <jsn@deploy1002> Finished scap: Backport for [[gerrit:1032571|Update VE core submodule to master (27296e0e3) (T230323 T365052)]] (duration: 25m 10s)

matmarex moved this task from To Triage to Triaged on the VisualEditor board.
matmarex moved this task from Incoming to QA on the Editing-team (Kanban Board) board.
matmarex edited projects, added Editing QA; removed Patch-For-Review.

This happens when the transaction swaps out the DOM node (from <td> to <th>), but only changes an attribute on the DM tableCell node (from style=data to style=header).

The same happens when changing a bullet list to a numbered list: the DOM node gets swapped out (from <ul> to <ol>), but the DM node only gets an attribute change (from style=bullet to style=number).

The mutation observer sees the removal of the <p> inside the <td> and deletes the corresponding DM paragraph, or sees the removal of the <li> inside the <ul> and deletes the corresponding DM listItem. It does not realise the nodes are being reused.

Change #1033718 had a related patch set uploaded (by Divec; author: Divec):

[VisualEditor/VisualEditor@master] Make ve.ce.Surface#afterMutations understand updateTagName

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

@Esanders, Is the double modal an expected experience?

Screenshot 2024-05-20 at 17.59.23.png (550×1 px, 61 KB)

@Esanders, Is the double modal an expected experience?

Screenshot 2024-05-20 at 17.59.23.png (550×1 px, 61 KB)

Great spot, @AEakinloose. Per today's discussion. Behavior you spotted is expected even if not ideal.

Potential solve (out of scope for this ticket) @nayoub shared: add some kind of "global" affordance so that row/cell specific context item and global table context items are not shown simultaneously.

Change #1032418 merged by jenkins-bot:

[VisualEditor/VisualEditor@master] Run TableAction/ListAction tests with a full view

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

Change #1035815 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (e3298c4a5)

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

Change #1035815 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (e3298c4a5)

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