Page MenuHomePhabricator

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

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)

Event Timeline

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.

image.png (2×3 px, 428 KB)

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

image.png (1×3 px, 266 KB)

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)

Esanders added a subscriber: Esanders.

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>

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.

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

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.

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

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

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

ppelberg claimed this task.