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

bzimport set Reference to bz51150.
GWicke created this task.Via LegacyJul 11 2013, 1:18 AM
Catrope added a comment.Via ConduitJul 11 2013, 1:52 AM

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?

GWicke added a comment.Via ConduitJul 11 2013, 2:52 PM
  • Bug 51161 has been marked as a duplicate of this bug. ***
GWicke added a comment.Via ConduitJul 11 2013, 2:56 PM

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>

GWicke added a comment.Via ConduitJul 11 2013, 2:56 PM

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

Ironholds added a comment.Via ConduitJul 11 2013, 2:57 PM

Aha. Gotcha.

GWicke added a comment.Via ConduitJul 11 2013, 3:04 PM

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.

gerritbot added a comment.Via ConduitJul 11 2013, 4:43 PM

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

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

gerritbot added a comment.Via ConduitJul 11 2013, 4:53 PM

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

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

GWicke added a comment.Via ConduitJul 11 2013, 4:59 PM

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.

gerritbot added a comment.Via ConduitJul 12 2013, 12:47 AM

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

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

gerritbot added a comment.Via ConduitJul 12 2013, 12:48 AM

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

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

GWicke added a comment.Via ConduitJul 13 2013, 11:07 PM
  • Bug 51161 has been marked as a duplicate of this bug. ***
GWicke added a comment.Via ConduitJul 13 2013, 11:09 PM

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?

Catrope added a comment.Via ConduitJul 15 2013, 10:35 PM

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

GWicke added a comment.Via ConduitJul 15 2013, 10:54 PM

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

bzimport added a comment.Via ConduitJul 19 2013, 9:54 PM

wicke wrote:

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

Jdforrester-WMF added a comment.Via ConduitJul 24 2013, 3:36 AM

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

gerritbot added a project: Patch-For-Review.Via ConduitDec 16 2014, 7:53 PM

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

gerritbot added a comment.Via ConduitDec 16 2014, 11:13 PM

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

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

Ironholds removed a subscriber: Ironholds.Via WebDec 16 2014, 11:14 PM

Add Comment