Last modified: 2013-07-24 03:36:25 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T53150, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 51150 - VisualEditor: Preserve unmodified data-mw attributes to avoid corrupting templates' whitespace
VisualEditor: Preserve unmodified data-mw attributes to avoid corrupting temp...
Status: RESOLVED FIXED
Product: VisualEditor
Classification: Unclassified
Data Model (Other open bugs)
unspecified
All All
: Highest major
: VE-deploy-2013-07-25
Assigned To: Roan Kattouw
:
: 51161 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-11 01:18 UTC by Gabriel Wicke
Modified: 2013-07-24 03:36 UTC (History)
6 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Gabriel Wicke 2013-07-11 01:18:21 UTC
data-mw.i seems to be dropped on edit in latest VE, which causes a loss of round-trip information for spaces etc.
Comment 1 Gabriel Wicke 2013-07-11 01:21:15 UTC
This then causes diffs like http://en.wikipedia.org/w/index.php?title=Bj%C3%B6rk&diff=563745216&oldid=563123196
Comment 2 Roan Kattouw 2013-07-11 01:52:19 UTC
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?
Comment 3 Gabriel Wicke 2013-07-11 14:52:48 UTC
*** Bug 51161 has been marked as a duplicate of this bug. ***
Comment 4 Oliver Keyes 2013-07-11 14:54:10 UTC
https://en.wikipedia.org/w/index.php?title=Christchurch,_Dorset&diff=prev&oldid=563716751 appears to be a pre-deployment occurrence.
Comment 5 Gabriel Wicke 2013-07-11 14:56:03 UTC
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>
Comment 6 Gabriel Wicke 2013-07-11 14:56:41 UTC
(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.
Comment 7 Oliver Keyes 2013-07-11 14:57:39 UTC
Aha. Gotcha.
Comment 8 Gabriel Wicke 2013-07-11 15:04:39 UTC
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.
Comment 9 Gerrit Notification Bot 2013-07-11 16:43:30 UTC
Change 73187 had a related patch set uploaded by GWicke:
Workaround for VE bug 51150

https://gerrit.wikimedia.org/r/73187
Comment 10 Gerrit Notification Bot 2013-07-11 16:53:12 UTC
Change 73187 merged by jenkins-bot:
Workaround for VE bug 51150

https://gerrit.wikimedia.org/r/73187
Comment 11 Gabriel Wicke 2013-07-11 16:59:05 UTC
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.
Comment 12 Gerrit Notification Bot 2013-07-12 00:47:02 UTC
Change 73372 had a related patch set uploaded by Catrope:
Preserve unused Parsoid template properties

https://gerrit.wikimedia.org/r/73372
Comment 13 Gerrit Notification Bot 2013-07-12 00:48:30 UTC
Change 73372 merged by jenkins-bot:
Preserve unused Parsoid template properties

https://gerrit.wikimedia.org/r/73372
Comment 14 Gabriel Wicke 2013-07-13 23:07:23 UTC
*** Bug 51161 has been marked as a duplicate of this bug. ***
Comment 15 Gabriel Wicke 2013-07-13 23:09:56 UTC
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 = Scarborough|symbol = rail|code = SCA|image_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=Scarborough |symbol=rail |code=SCA
|image_name=ScarboroughRailwayStation.jpg |caption=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?
Comment 16 Roan Kattouw 2013-07-15 22:35:44 UTC
(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.
Comment 17 Gabriel Wicke 2013-07-15 22:54:17 UTC
(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.
Comment 18 Gabriel Wicke 2013-07-19 21:54:49 UTC
*** Bug 51175 has been marked as a duplicate of this bug. ***
Comment 19 James Forrester 2013-07-24 03:36:25 UTC
We've not seen this recur; it was probably caused by mis-cached code. Closing, but please re-open if it does happen again.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links