Page MenuHomePhabricator

Find out why a <nowiki/> tag are causing edits to get mangled
Closed, ResolvedPublic

Description

On the Cyphomenes anisitsii page on the English Wikipedia, there was a superfluous <nowiki/> tag in the source—[[Venezuela]] <nowiki/>first described—which somehow caused certain kinds of edits to the page to get mangled. If you delete the <nowiki/> tag, which you can try in the below sandbox page, this doesn't happen.

Steps to reproduce:

  1. Go to https://en.wikipedia.org/wiki/User:Deskana_(WMF)/Cyphomenes_anisitsii, which is a copy of the page as it was when it still had the <nowiki/> tag in it.
  2. Edit with the visual editor
  3. In the second sentence of the first paragraph:
    1. Change 2018 to 2017
    2. Change one to some
    3. Change is to are
  4. Delete reference 3

Expected:
The sentence is As of 2017, some subspecies are listed in the Catalogue of Life, Cyphomenes anisitsii ornatissmus.

Actual:
The sentence is As of 2718somene subspecieareis listed in the Catalogue of Life, Cyphomenes anisitsii ornatissmus.

Other steps to reproduce:

  1. Go to https://eu.wikipedia.org/wiki/Ida_Kami%C5%84ska
  2. Edit it with the visual editor (i.e. click "Aldatu")
  3. Open the find dialogue
  4. Type "Varsovia"

Expected:
Varsovia is highlighted.

Actual:
oviako A is highlighted.

This problem does not occur if you edit a version that has the <nowiki/> tags removed.

Event Timeline

Deskana created this task.Mar 7 2018, 2:00 PM
Restricted Application added subscribers: jeblad, Aklapper. · View Herald TranscriptMar 7 2018, 2:00 PM
Deskana triaged this task as High priority.Mar 7 2018, 2:00 PM
Deskana moved this task from To Triage to Current work on the VisualEditor board.
Deskana edited projects, added VisualEditor (Current work); removed VisualEditor.
Deskana renamed this task from Find out why a <nowiki/> tag caused edits to get mangled on a specific page to Find out why a <nowiki/> tag are causing edits to get mangled.Mar 12 2018, 12:22 PM
Deskana raised the priority of this task from High to Unbreak Now!.
Deskana added subscribers: Inaki-LL, Theklan.

This is affecting more than one page, so this needs to be fixed fast.

Restricted Application added subscribers: Liuxinyu970226, TerraCodes. · View Herald TranscriptMar 12 2018, 12:23 PM
Deskana updated the task description. (Show Details)Mar 12 2018, 12:26 PM

I reopened T187699#4042805 mainly because it's also happening with the New Wikitext Mode even with articles that doesn't have <nowiki /> tags.

I reopened T187699#4042805 mainly because it's also happening with the New Wikitext Mode even with articles that doesn't have <nowiki /> tags.

If it's happening without the nowiki tags, then it is a separate bug. Let's discuss on T187699.

@Deskana Thanks for looking into this! Ok, I am continuing the thread here. I tried again with the year 1973 in the article ("1973an") Ida Kamiska, and it still happens. I select 1973, to link it to the year 1973. Still the link slides to the left out of the word. However, no "nowiki" tag shows as a result, see here in wikitext once I have performed the change in Visual Editor.

@Deskana Thanks for looking into this! Ok, I am continuing the thread here. I tried again with the year 1973 in the article ("1973an") Ida Kamiska, and it still happens. I select 1973, to link it to the year 1973. Still the link slides to the left out of the word. However, no "nowiki" tag shows as a result, see here in wikitext once I have performed the change in Visual Editor.

I think you've misunderstood what I said. The edit you make doesn't need to insert a nowiki tag to have this issue; it's the presence of the nowiki tags elsewhere in the paragraph that cause the problem.

At this stage, I think the problem is pretty well understood, so please give us a little time to work on it. :-)

Deskana added subscribers: matmarex, dchan.

From merged task:

This is a regression from rGVED21a5d55b85dd: [BREAKING CHANGE] Store metadata as block nodes in the main data list.

In VE, the <nowiki/> turns into an alienMeta node, and I guess that has no rendering in CE and that breaks stuff?

Looks like ve.ce.Document.prototype.getNodeAndOffset only handles nodes without a "DOM representation" (same thing as I meant by "no rendering in CE") when they appear elsewhere than inside ve.ce.ContentBranchNode – this was actually added recently in rGVEDc7e904bb49fc: Fix getNodeAndOffset for MetaItems to fix T186109. In this case though the alienMeta is inside a paragraph which is a ContentBranchNode :(

dchan claimed this task.Mar 13 2018, 8:51 AM

Thanks @matmarex (and oops) - taking a look.

Thanks @Deskana.

Continuing that investigation – it looks like meta nodes are really super not supposed to appear inside ContentBranchNodes. rGVED21a5d55b85dd: [BREAKING CHANGE] Store metadata as block nodes in the main data list added a bunch of code in Converter to move them out of there (this works, and can be verified with e.g. an empty comment node generated by <!---->). But the case where an empty annotation is turned into alienMeta was missed (<nowiki> generates an annotation).

Change 419121 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[VisualEditor/VisualEditor@master] ve.dm.Converter: Handle meta nodes for empty annotations same as normal

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

dchan reassigned this task from dchan to matmarex.Mar 13 2018, 9:03 AM

This fixes the exception, but I'm not really sure if the behavior makes a lot of sense… these empty annotations are actually getting lost now when saving pages. Prior to 21a5d55b85dd they were being preserved correctly.

(Note that this applies to any annotation, so a <b></b> would cause the same problem.)

dchan added a comment.Mar 13 2018, 9:15 AM

Hmmm, that makes sense, because since 21a5d55b85dd , we only round-trip the meta item back into the ContentBranchNode on serialization if the ContentBranchNode is unmodified - so clearly if the <nowiki/> is being used to separate out pieces of wiki syntax, the behaviour will now not be as intended.

I wonder, can we treat this sort of <nowiki/> differently? Is this the only such case, or are there others?

The <nowiki/> case will be fine, since Parsoid will generate them again if necessary, but any other empty annotations would cause dirty diffs, I guess.

matmarex added a comment.EditedMar 13 2018, 9:21 AM

So, never mind the lost empty annotations, I think it is perfectly fine to remove them.

I think deleting empty annotations is not a good idea, as they're used extensively in templates in order not to break lines but have the code clear.

@Theklan This code won't touch anything inside template transclusions.

When the empty annotation is actually needed in article content, it will be generated again by Parsoid, like in the example given on the other task (T187690#4011239): ''X-treme''<nowiki/>'s. At worst, something like <b></b> will be replaced by <nowiki/>.

Are there any other situations you're worried about?

Not really worried about anything!

Change 419121 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] ve.dm.Converter: Handle meta nodes for empty annotations same as normal

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

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

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

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

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

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

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

Scheduled for deployment: https://wikitech.wikimedia.org/wiki/Deployments#deploycal-item-20180314T2300

Note that I am only backporting to MediaWiki 1.31.0-wmf.25, and not wmf.24. This means the patch go live on all non-Wikipedia wikis now, and will reach Wikipedias tomorrow, per the usual schedule (https://www.mediawiki.org/wiki/MediaWiki_1.31/Roadmap).

Backporting to wmf.24 would conflict with other changes we merged in the last week, and while it would be possible to resolve that, it would not be much faster than just waiting about 23 hours for the regular deployment.

Change 419518 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@wmf/1.31.0-wmf.25] Update VE core submodule to master (dc5a65131)

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

matmarex closed this task as Resolved.Mar 15 2018, 12:46 AM
matmarex removed a project: Patch-For-Review.

The patch is live on all non-Wikipedia wikis now, and will reach Wikipedias tomorrow, per the usual schedule (https://www.mediawiki.org/wiki/MediaWiki_1.31/Roadmap).