Page MenuHomePhabricator

VisualEditor: Preserve unmodified data-mw attributes to avoid corrupting templates' whitespace
Closed, ResolvedPublic

Description

data-mw.i seems to be dropped on edit in latest VE, which causes a loss of round-trip information for spaces etc.


Version: unspecified
Severity: major

Event Timeline

bzimport raised the priority of this task from to Unbreak Now!.Nov 22 2014, 1:42 AM
bzimport set Reference to bz51150.

I can't reproduce this, even if I edit the template. VE preserves unmodified data-mw attributes always.

Also, what is data-mw.i ? Is it new?

  • Bug 51161 has been marked as a duplicate of this bug. ***

It is either data-mw.i or data-mw.parts[<n>].i for templates. It is not new, but so far we have not used that information that heavily, so the fact that you drop the i probably fell under the radar. We use it to associate the public entry with private round-trip information such as the order of parameters and (crucially for this bug) whitespace.

echo -e '{{echo|a = foo\n|b = c}}| node parse
<body><p about="#mwt1" typeof="mw:Transclusion" data-mw='{"target":{"wt":"echo","href":"./Template:Echo"},"params":{"a":{"wt":"foo"},"b":{"wt":"c"}},"i":0}' data-parsoid='{"dsr":[0,27,null,null],"src":"{{echo|a = foo\n|b = c}}","pi":[[{"k":"a","spc":[""," "," ","\n"]},{"k":"b","spc":[""," "," ",""]}]]}'>{{{1}}}</p>
</body>

(In reply to comment #4)

https://en.wikipedia.org/w/index.php?title=Christchurch,
_Dorset&diff=prev&oldid=563716751
appears to be a pre-deployment occurrence.

This is post-Parsoid deployment.

Example on http://www.mediawiki.org/wiki/User:GWicke/TestDataMW?veaction=edit:

Original data-mw:
data-mw='{"target":{"wt":"echo","href":"../Template:Echo"},"params":{"1":{"wt":"foo"}},"i":0}'

data-mw through VE without edit:
data-mw="{&quot;target&quot;:{&quot;wt&quot;:&quot;echo&quot;,&quot;href&quot;:&quot;../Template:Echo&quot;},&quot;params&quot;:{&quot;1&quot;:{&quot;wt&quot;:&quot;foo&quot;}},&quot;i&quot;:0}"

data-mw through VE after changing 'foo' to 'bar':
data-mw="{&quot;target&quot;:{&quot;wt&quot;:&quot;echo&quot;,&quot;href&quot;:&quot;../Template:Echo&quot;},&quot;params&quot;:{&quot;1&quot;:{&quot;wt&quot;:&quot;bar&quot;}}}"

Note that the "i" member is gone.

Change 73187 had a related patch set uploaded by GWicke:
Workaround for VE bug 51150

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

Change 73187 merged by jenkins-bot:
Workaround for VE bug 51150

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

The Parsoid workaround above is now deployed. If the index went missing in VE, it assumes that a single-transclusion target was not swapped out and reinserts the lost index in that case. This should avoid corruptions in the common single-template case.

It will do nothing for multi-transclusion content (table start / row etc), and will also fail if the template was swapped out for another one. The latter case should be rare.

Change 73372 had a related patch set uploaded by Catrope:
Preserve unused Parsoid template properties

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

Change 73372 merged by jenkins-bot:
Preserve unused Parsoid template properties

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

  • Bug 51161 has been marked as a duplicate of this bug. ***

I'm assuming that this is not yet deployed, as a new report was added to 51161:


This is what happened:
http://en.wikipedia.org/w/index.php?title=Scarborough_railway_station&diff=next&oldid=557856562

This is what the user intended:
http://en.wikipedia.org/w/index.php?title=Scarborough_railway_station&diff=564074475&oldid=557856562

If removal of newlines really is necessary, please insert a space instead. If
that is not possible, please remove the spaces from both sides of each equals.
An arrangement like

name = Scarboroughsymbol = railcode = SCAimage_name =

ScarboroughRailwayStation.jpg|caption = The entrance to the station

gives the impression of associating a parameter name with the value immediately
preceding. An arrangement like

name=Scarboroughsymbol=railcode=SCA
image_name=ScarboroughRailwayStation.jpgcaption=The entrance to the station

would associate a parameter name with the value immediately following.

Normally this single-transclusion content should be covered by the Parsoid workaround. Did anything change in the VE handling recently that could have re-added the "i" property with a faulty value?

(In reply to comment #15)

I'm assuming that this is not yet deployed, as a new report was added to
51161:

It is deployed.

Normally this single-transclusion content should be covered by the Parsoid
workaround. Did anything change in the VE handling recently that could have
re-added the "i" property with a faulty value?

That sounds unlikely. When the user adds a new parameter, that parameter won't have an i value, but that seems reasonable to me.

I'll try to reproduce this later and see what's being sent back to Parsoid.

(In reply to comment #16)

(In reply to comment #15)

I'm assuming that this is not yet deployed, as a new report was added to
51161:

It is deployed.

Normally this single-transclusion content should be covered by the Parsoid
workaround. Did anything change in the VE handling recently that could have
re-added the "i" property with a faulty value?

That sounds unlikely. When the user adds a new parameter, that parameter
won't
have an i value, but that seems reasonable to me.

That infobox already existed. The 'i' property is per transclusion, not per parameter.

wicke wrote:

*** Bug 51175 has been marked as a duplicate of this bug. ***

We've not seen this recur; it was probably caused by mis-cached code. Closing, but please re-open if it does happen again.

Change 180240 had a related patch set uploaded (by Cscott):
Remove workaround for Visual Editor bug (T53150).

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

Patch-For-Review

Change 180240 merged by jenkins-bot:
Remove workaround for Visual Editor bug (T53150).

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