VisualEditor fails to load when page contains an image inside an external link: imgWrapper is undefined
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Pols12
	Nov 5 2020, 1:27 AM

Description

Try to edit a page which contains an image inside an external link with VisualEditor.

It will fail to load, JavaScript console displays:

Uncaught TypeError: imgWrapper is undefined
    toDataElement URL1:1568
    createDataElements URL1:316
    getDataFromDomSubtree URL1:323
    getDataFromDomSubtree URL1:324
    getDataFromDomSubtree URL1:326
    getModelFromDom URL1:318
    createModelFromDom URL1:62
    createModelFromDom URL1:1044
    setupSurface URL1:1046
load.php:1568:419
    toDataElement URL1:1568
    createDataElements URL1:316
    getDataFromDomSubtree URL1:323
    getDataFromDomSubtree URL1:324
    getDataFromDomSubtree URL1:326
    getModelFromDom URL1:318
    createModelFromDom URL1:62
    createModelFromDom URL1:1044
    setupSurface URL1:1046

URL1: https://www.mediawiki.org/w/load.php?lang=fr&modules=ext.visualEditor.articleTarget,base,core,desktopArticleTarget,desktopTarget,diffing,icons,language,mediawiki,mwalienextension,mwcore,mwextensions,mwformatting,mwgallery,mwimage,mwlanguage,mwlink,mwmeta,mwsave,mwsignature,mwtransclusion,sanitize,switching,welcome|ext.visualEditor.core.desktop,utils|ext.visualEditor.mwextensions.desktop|ext.visualEditor.mwimage.core&skin=vector&version=1e9z7

Problematic wikitext:

[https://url.com text [[Image:Speaker Icon.svg|20px]]]

No problem if image is not linked:

[https://url.com text [[Image:Speaker Icon.svg|20px|link=]]]

Testing

Open this link
Verify that the visual editor loads completely, and doesn't just hang with the progress bar showing
When saving the page (or previewing changes), no dirty diffs occur (see examples in T64473 and T150196 for things that should not happen any more)

Details

	Project	Branch	Lines +/-	Subject
	mediawiki/extensions/VisualEditor	master	+24 -13	ve.dm.MWExternalLinkAnnotation: Alienate malformed links
	mediawiki/extensions/VisualEditor	master	+20 -12	ve.dm.MWInlineImageNode: Alienate malformed figures

Customize query in gerrit

Related Objects
Search...

Status	Assigned	Task
Resolved	matmarex	T64473 VisualEditor: Image as part of the anchor of a link is stripped on round-trip
Resolved	matmarex	T150196 VE silently alters the HTML of an invalid wikitext sequence of a link with an image in the label, which dirty-diffs
Resolved	ppelberg	T267282 VisualEditor fails to load when page contains an image inside an external link: imgWrapper is undefined

Event Timeline

Pols12 created this task.Nov 5 2020, 1:27 AM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 5 2020, 1:27 AM

I tried to reproduce T64473, which was maybe a better behavior than current one.

Change 639632 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] ve.dm.MWInlineImageNode: Alienate malformed figures

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

Change 639633 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] ve.dm.MWExternalLinkAnnotation: Alienate malformed links

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

Parsoid produces very broken output due to the misnesting, and VisualEditor can't handle it. The second <a> tag should be inside the <figure-inline> tag, but that's not possible in HTML, because <a> tags can't be nested.

It's not feasible to piece this back together in VisualEditor, and allow editing that image. But at least we can handle this case and "alienate" it (make it uneditable). The first patch above does it.

However, this reveals a second issue: the <a> tag that was pulled out is still there, and it's converted to a regular wikitext link when converting to wikitext. (The <img> can't be converted and results in a nowiki tag.)

It turns out that Parsoid actually has a mechanism that would piece the figure back together and avoid the dirty diff, but it doesn't trigger because VisualEditor does some cleanup to that node, intended for copy-pasting external links. The second patch above fixes this (and you could say it fixes T64473 too, although the exact kind of dirty diffs has changed since 2014).

(I probably should not have prioritized this… but here we are)

matmarex moved this task from Incoming to Code Review on the Editing-team (FY2021-22 Kanban Board) board.Nov 5 2020, 10:14 PM

Should this be fixed in Parsoid instead of VE?

There's T248369 filed for this, but note that if an empty link= parameter is provided, you won't get this misnesting. Not sure if that's a feature worth preserving though

For example, [http://some.thing Try me [[File:Test.png|link=]] out] renders as,

<p><a rel="mw:ExtLink" href="http://some.thing" class="external text">Try me <figure-inline class="mw-default-size" typeof="mw:Image"><span><img resource="./File:Test.png" src="//upload.wikimedia.org/wikipedia/commons/6/6a/PNG_Test.png" data-file-width="3120" data-file-height="3920" data-file-type="bitmap" height="3920" width="3120"/></span></figure-inline> out</a></p>

In T267282#6634192, @Arlolra wrote:

There's T248369 filed for this, but note that if an empty link= parameter is provided, you won't get this misnesting. Not sure if that's a feature worth preserving though

No, that just makes it plain confusing. It is better to simple-to-grok rules that MOS of wikis can adopt. "You should not nest images or wikilinks inside extlinks".

But, separately, we should try to add "mw:Placeholder" (maybe we need a better name going forward: mw:Uneditable) protection around such misnested blocks.

In T267282#6634203, @ssastry wrote:

In T267282#6634192, @Arlolra wrote:

There's T248369 filed for this, but note that if an empty link= parameter is provided, you won't get this misnesting. Not sure if that's a feature worth preserving though

No, that just makes it plain confusing. It is better to simple-to-grok rules that MOS of wikis can adopt. "You should not nest images or wikilinks inside extlinks".

Yet, this practice may be pretty common (especially for icon type images). I think a usage evaluation should be done first, if this syntax should become deprecated.
EDIT: my bad, this evaluation could be done thanks to T248369

The case where link= suppresses the <a> tag on the image is perfectly valid and should work. I believe this does work in Parsoid and round-trips. If there's an issue with that markup, it is a bug in VE and should be fixed there.

If the image is missing the link= attribute (so there's an <a> inside an <a>), that's a wikitext error. Parsoid should emit a linter error in this case (T248369). We *may* fix up the inner <a> but are not guaranteed to. VE won't have to deal with this case because <a>-inside-<a> can't be parsed after serialization. We should open a separate task to discuss the issue in general (<a>-inside-<a> can be generated via other means as well, eg via templates inside wikilink captions, and it might merit a generalized fixup to handle this -- ie, replace the inner <a> with a <span> and add some data-mw to indicate this, in roughly the same way we track spurious close tags etc).

Arlolra mentioned this in T248369: Add linting rule for detecting presence of image wikitext in extlinks.Nov 23 2020, 6:20 PM

In T267282#6634192, @Arlolra wrote:

if an empty link= parameter is provided, you won't get this misnesting. Not sure if that's a feature worth preserving though

An empty link is valid; one of the use cases is where an image serves as decoration; accessibility guidelines recommend no linking in such a case (and also setting the alt text to the empty string). (There's some chatter occasionally about whether that's a problem for attribution of the image, but that's a policy issue and doesn't affect images not requiring attribution.)

<a>-inside-<a> can be generated via other means as well, eg via templates inside wikilink captions

I believe this is already generally linted for, otherwise T242068: Linter does not detect link-in-link when wikilink is wrapped in italics would not have been written as so.

ssastry moved this task from Needs Triage to No Remaining Parsoid Work on the Parsoid board.Nov 24 2020, 12:07 AM

matmarex mentioned this in T150196: VE silently alters the HTML of an invalid wikitext sequence of a link with an image in the label, which dirty-diffs.Dec 2 2020, 11:19 PM

Change 639632 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] ve.dm.MWInlineImageNode: Alienate malformed figures

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

ReleaseTaggerBot added a project: MW-1.36-notes (1.36.0-wmf.21; 2020-12-08).Dec 3 2020, 12:00 PM

Esanders updated the task description. (Show Details)Dec 7 2020, 2:03 PM

Esanders updated the task description. (Show Details)

Change 639633 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] ve.dm.MWExternalLinkAnnotation: Alienate malformed links

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

Maintenance_bot removed a project: Patch-For-Review.Dec 7 2020, 3:10 PM

matmarex updated the task description. (Show Details)Dec 7 2020, 8:53 PM

matmarex added parent tasks: T64473: VisualEditor: Image as part of the anchor of a link is stripped on round-trip, T150196: VE silently alters the HTML of an invalid wikitext sequence of a link with an image in the label, which dirty-diffs.

matmarex moved this task from Code Review to QA on the Editing-team (FY2021-22 Kanban Board) board.

matmarex added a project: Editing QA.

MBinder_WMF moved this task from No Remaining Parsoid Work to Tracking on the Parsoid board.Dec 10 2020, 8:12 PM

MBinder_WMF edited projects, added Parsoid (Tracking); removed Parsoid.

Ryasmeen moved this task from QA to Ready for Sign Off on the Editing-team (FY2021-22 Kanban Board) board.Dec 15 2020, 11:01 PM

Ryasmeen edited projects, added Verified; removed Editing QA.Dec 15 2020, 11:03 PM

ppelberg closed this task as Resolved.Dec 16 2020, 2:22 AM

ppelberg claimed this task.

Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptDec 16 2020, 2:22 AM

	F32439315: image.png
	Nov 5 2020, 10:13 PM

	F32439297: image.png
	Nov 5 2020, 10:13 PM

VisualEditor fails to load when page contains an image inside an external link: imgWrapper is undefinedClosed, ResolvedPublicActions