Page MenuHomePhabricator

Copy-paste of template-generated link from a subpage in VE gives the wrong href
Closed, ResolvedPublic1 Estimated Story Points


Copy-paste of template-generated link from a subpage in VE gives the wrong href.

For example, put this on a page (it must be a subpage, e.g.


Copy-paste from VE into an external editor, e.g.


<a rel="mw:WikiLink" href="" title="Y" about="#mwt1" typeof="mw:Transclusion" data-mw="{&quot;parts&quot;:[{&quot;template&quot;:{&quot;target&quot;:{&quot;href&quot;:&quot;Template:1x&quot;,&quot;wt&quot;:&quot;1x&quot;},&quot;params&quot;:{&quot;1&quot;:{&quot;wt&quot;:&quot;[[Y]]&quot;}}}}]}" id="mwAg" data-ve-no-generated-contents="true" data-ve-attributes="{&quot;typeof&quot;:&quot;mw:Transclusion&quot;,&quot;rel&quot;:&quot;mw:WikiLink&quot;,&quot;about&quot;:&quot;#mwt1&quot;}">Y</a>

Note the link to instead of

Normal links are not affected, neither is copy-pasting from pages that are not subpages.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

ve.ce.Surface.prototype.onCopy renders the HTML into the current document: slice, this.$pasteTarget[ 0 ], true ); generates some nodes and resolves their attributes with that document:

ve.resolveAttributes( $( els ), doc, );

At this point the href is already irrecoverably broken.
ve.ce.Surface.prototype.onCopy half-heartedly tries to fix this but it can do nothing:

this.$pasteTarget.find( 'a' ).attr( 'href', function ( i, href ) {
	return ve.resolveUrl( href, htmlDoc );
} );

Side note: for T169675 the same issue occurs, except instead of using current document, creates a document with the default base:

var doc = ve.createDocumentFromHtml( '' );

I suppose we should remove all the resolveAttributes() calls in assorted nodes (e.g. MWExtensionNode also does that) and instead put one in ve.ce.Surface#onCopy. For T169675, ve.ui.PreviewElement already tries to do that. @Esanders?

[18:23]	<edsanders> I'm not sure what the use cases are for those resolve attribute things
[18:24]	<edsanders> I think it's more that just copy paste, but @RoanKattouw would probably know better
[18:27]	<RoanKattouw> Hmm not sure why we have attribute resolution in toDomElements
[18:28]	<RoanKattouw> We need it for rendering purposes to be sure, but I don't see how we need it for Parsoid output purposes
[18:29]	<RoanKattouw> CE deals with both rendering and copy, so I'm skeptical of DM doing any resolution at all
[18:29]	<RoanKattouw> But maybe we should git blame those calls and see if we can discover why they were introduced
[18:31]	<MatmaRex> RoanKattouw: resolveAttrivutes() in toDomElements() is only called for converting to clipboard
[18:31]	<MatmaRex> (except we also use the clipboard mode sometimes when not actually converting to clipboard)
[18:32]	<MatmaRex> RoanKattouw: edsanders: blame points to ;)
[18:32]	<RoanKattouw> Ha OK
[18:33]	<RoanKattouw> So maybe that was the wrong place to put that and uses the wrong document?
[18:33]	<RoanKattouw> Either way, moving that stuff around between toDomElements and ce.Surface.onCopy should be safe then
[18:34]	<RoanKattouw> In the sense that it shouldn't break anything else

Change 376507 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] Do not "resolve attributes" in toDomElements() overrides

Change 376507 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Do not "resolve attributes" in toDomElements() overrides

Deskana set the point value for this task to 1.
Deskana moved this task from To Triage to TR1: Releases on the VisualEditor board.