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

Column Prototype
This is a very early prototype of a persistent column. It is not expected to work yet, and leaving it open will activate other new features which will break things. Press "\" (backslash) on your keyboard to close it now.