Page MenuHomePhabricator

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

Description

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. https://en.wikipedia.org/wiki/User:Matma_Rex/sandbox):

{{1x|[[Y]]}}

Copy-paste from VE into an external editor, e.g. http://edg2s.github.io/content-editable-sandbox/.

Output:

<a rel="mw:WikiLink" href="https://en.wikipedia.org/wiki/User:Matma_Rex/Y" 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 https://en.wikipedia.org/wiki/User:Matma_Rex/Y instead of https://en.wikipedia.org/wiki/Y.

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

Event Timeline

matmarex created this task.Sep 6 2017, 2:33 PM
Restricted Application added a project: VisualEditor. · View Herald TranscriptSep 6 2017, 2:33 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

ve.ce.Surface.prototype.onCopy renders the HTML into the current document:

ve.dm.converter.getDomSubtreeFromModel( slice, this.$pasteTarget[ 0 ], true );

ve.dm.MWTransclusionNode.static.toDomElements generates some nodes and resolves their attributes with that document:

ve.resolveAttributes( $( els ), doc, ve.dm.Converter.static.computedAttributes );

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, ve.dm.Converter.prototype.getDomFromModel 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?

matmarex claimed this task.Sep 6 2017, 4:59 PM
[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 https://phabricator.wikimedia.org/T111927 ;)
[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

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

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

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

Deskana closed this task as Resolved.Sep 19 2017, 6:53 PM
Deskana set the point value for this task to 1.
Deskana moved this task from To Triage to TR1: Releases on the VisualEditor board.
Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptSep 19 2017, 6:53 PM