Page MenuHomePhabricator

VisualEditor: Image CE HTML should match Parsoid
Closed, ResolvedPublic

Description

When converting between block and inline images we hide the <figcaption> with CSS instead of removing it from the DOM tree. We need to preserve the caption in case the user converts from inline back to block, but this can be done in the model.


Version: unspecified
Severity: enhancement

Details

Reference
bz66610

Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 3:10 AM
bzimport set Reference to bz66610.

Discussion from Gerrit:

Mooeypoo Jun 13 8:25 PM

<figcaption> isn't destroyed and/or is recreated each time an image type changes, so it has to be hidden for the block image types that don't support it, like frameless and basic.
But yes, it could be added to the CSS instead, hiding the figcaption for the block image types that don't support it.
I'm not sure it should be generalized for Parsoid's use, though, since the Parsoid output for the figcaption is always correct (so, no caption for images that don't support it). We keep the caption in VE to let the user be able to change the images back and forth and retain the information in the caption even if the type changed.

Esanders Jun 13 8:29 PM

If it's not supposed to be in the tree when it's not in use, why don't we destroy it completely and recreate it when it's needed again?

Mooeypoo Jun 13 8:40 PM

Because the caption information is taken from <figcaption> and we want to retain it for (A) if the user regrets their type choice and edits again to another type that supports the caption (make sure that the caption isn't lost) and (B) so it won't be removed from the wikitext.
There's a tech debt to fix the way caption works in general, especially in light of VE allowing the switch from block to inline and the existence of alternate text (which gets the value of the caption if it exists) but right now it's on hold for a little bit waiting for the new Media Edit dialog redesign.

Esanders Jun 13 8:43 PM

If we need to preserve the caption that should be done in the model, not the view.

Mooeypoo Jun 13 8:48 PM

Yes, and it also must be preserved in wikitext, so the <figcaption> must be sent to parsoid in certain cases. And the caption surface in the editor should be selectively disabled and enabled, and exchange its information with the alt-text in a way that the user understands where and why things are replaced.
Hence the tech debt.

GWicke Jun 13 9:36 PM

Also see https://gerrit.wikimedia.org/r/#/c/139473/. There should be no need to manually hide the caption with inline CSS. Inline images have no caption in the DOM, as that can for example break a surrounding paragraph in case the caption contains a paragraph.

Current Parsoid HTML:

<figure class="mw-default-size" typeof="mw:Image/Thumb" data-parsoid="…">
<a href="./File:Smokeycover.jpg" data-parsoid="…">

		<img resource="./File:Smokeycover.jpg" src="//upload.wikimedia.org/wikipedia/en/thumb/7/73/Smokeycover.jpg/170px-Smokeycover.jpg" height="237" width="170" data-parsoid="…">

</a>
<figcaption data-parsoid="…">

		<a rel="mw:WikiLink" href="./Smokey_Stover" title="Smokey Stover" data-parsoid="…">Smokey Stover</a><!-- no italics, character’s name --> driving a "foomobile", with <i data-parsoid="…">phoo</i> visible on the front

</figcaption>
</figure>

Current VE HTML:

<figure class="ve-ce-mwBlockImageNode ve-ce-mwBlockImageNode-type-thumb mw-default-size ve-ce-branchNode ve-ce-mwBlockImageNode-borderwrap mw-halign-right ve-ce-focusableNode" typeof="mw:Image/Thumb" contenteditable="false" style="width: 172px; height: auto;">
<a class="image" href="https://en.wikipedia.org/wiki/File:Smokeycover.jpg">

		<img src="//upload.wikimedia.org/wikipedia/en/thumb/7/73/Smokeycover.jpg/170px-Smokeycover.jpg" class="ve-ce-mwBlockImageNode-thumbimage" style="width: 170px; height: 237px;">

</a>
<figcaption class="ve-ce-branchNode">

		<div class="magnify"><a class="internal"></a></div>
		<p class="ve-ce-paragraphNode ve-ce-generated-wrapper ve-ce-branchNode">
			<a class="ve-ce-linkAnnotation ve-ce-mwInternalLinkAnnotation new" href="http://en.wikipedia.beta.wmflabs.org/wiki/Smokey%20Stover" title="Smokey Stover">Smokey Stover</a><span class="ve-ce-leafNode ve-ce-commentNode oo-ui-indicatedElement-indicator oo-ui-indicator-alert oo-ui-indicatedElement ve-ce-focusableNode" contenteditable="false"> </span>&nbsp;driving a "foomobile", with <i class="ve-ce-textStyleAnnotation ve-ce-italicAnnotation">phoo</i> visible on the front
		</p>

</figcaption>
</figure>

The only structural difference is the "<div class="magnify"><a class="internal"></a></div>" which Parsoid is missing (and will go away soon enough anyway). Deeming this to be fixed.