Page MenuHomePhabricator
Create Task
Maniphest T328268

Dirty diffs in headings in edits made with reply tool
Closed, ResolvedPublic
Actions

Description

Over the last few days there was a lot of dirty diffs in edits made with reply tool, mostly within headings (e.g. adding/removing spaces, removing leading : in links). Examples:

And more found at https://dtcheck.toolforge.org/.

image.png (315×1 px, 44 KB)

Timing matches up suspiciously well with 884013 ApiDiscussionToolsEdit: Unwrap Parsoid sections before parsing, but it could be something else in 1.40.0-wmf.20.

Details

SubjectRepoBranchLines +/-
Don't add custom attributes in unwrapParsoidSections()mediawiki/extensions/DiscussionToolswmf/1.40.0-wmf.21+1 -14
Don't add custom attributes in unwrapParsoidSections()mediawiki/extensions/DiscussionToolsmaster+1 -14
Document need to remove data-mw-section-id attributes when savingmediawiki/extensions/VisualEditormaster+5 -0
Customize query in gerrit

Related Objects

Event Timeline

matmarex created this task.Jan 30 2023, 9:43 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 30 2023, 9:43 AM
Esanders subscribed.Jan 30 2023, 11:38 AM

Unwrap Parsoid sections seems like it would potentially change data-parsoid and trigger noramlisation, but also interesting that this isn't happening in VE (or is it?).

ssastry subscribed.Jan 30 2023, 3:16 PM
In T328268#8569122, @Esanders wrote:

Unwrap Parsoid sections seems like it would potentially change data-parsoid and trigger noramlisation,

Parsoid strips all section tags first thing in original and edited HTML before running dom diffs, etc. So, at first glance, my hunch is that this shouldn't matter (unless the section stripping in DT does something beyond stripping section tags).

but also interesting that this isn't happening in VE (or is it?).

It might be useful to try to repro this in VE by adding a test page with extra whitespace or something in headings and seeing if this gets normalized and check if this is common to both or isolated to DT.

matmarex added a comment.EditedJan 30 2023, 3:43 PM

VE also calls stripParsoidFallbackIds(), which sounds like it might be relevant: https://gerrit.wikimedia.org/g/mediawiki/extensions/VisualEditor/+/2fdac8851c74f9dbee6f7a6f117a1c1bdf9dc535/modules/ve-mw/init/targets/ve.init.mw.Target.js#230 (and also some other things that don't look relevant)

matmarex added a comment.Jan 30 2023, 4:08 PM

Consistently reproducible by adding a comment directly before a heading: https://en.wikipedia.beta.wmflabs.org/w/index.php?title=Talk%3AT328268&diff=prev&oldid=572654

I can also reproduce locally.

ppelberg subscribed.Jan 30 2023, 8:34 PM
matmarex added a comment.Jan 31 2023, 12:05 AM

I figured out why it happens.

It's because we do this:
https://gerrit.wikimedia.org/g/mediawiki/extensions/DiscussionTools/+/bbf5fe6287f6c08814b3e68e7e1f60ac03af505e/includes/CommentUtils.php#468
https://gerrit.wikimedia.org/g/mediawiki/extensions/VisualEditor/+/299110c446feca653a5603e9a39253f97455e3c8/modules/ve-mw/preinit/ve.utils.parsoid.js#61

But we don't do this:
https://gerrit.wikimedia.org/g/mediawiki/extensions/VisualEditor/+/299110c446feca653a5603e9a39253f97455e3c8/modules/ve-mw/preinit/ve.init.mw.ArticleTargetSaver.js#103

matmarex claimed this task.Jan 31 2023, 12:32 AM
matmarex edited projects, added Editing-team (Kanban Board); removed Parsoid.
gerritbot added a comment.Jan 31 2023, 12:32 AM

Change 885059 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/DiscussionTools@master] Don't add custom attributes in unwrapParsoidSections()

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

gerritbot added a project: Patch-For-Review.Jan 31 2023, 12:32 AM
matmarex moved this task from Incoming to Code Review on the Editing-team (Kanban Board) board.Jan 31 2023, 12:33 AM
gerritbot added a comment.Jan 31 2023, 3:17 PM

Change 885364 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/extensions/VisualEditor@master] Document need to remove data-mw-section-id attributes when saving

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

gerritbot added a comment.Feb 1 2023, 9:15 PM

Change 885364 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Document need to remove data-mw-section-id attributes when saving

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

ReleaseTaggerBot added a project: MW-1.40-notes (1.40.0-wmf.22; 2023-02-06).Feb 1 2023, 10:00 PM
gerritbot added a comment.Feb 2 2023, 9:23 AM

Change 885059 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@master] Don't add custom attributes in unwrapParsoidSections()

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

Maintenance_bot removed a project: Patch-For-Review.Feb 2 2023, 9:30 AM
matmarex moved this task from Code Review to Blocked / Needs More Work on the Editing-team (Kanban Board) board.Feb 2 2023, 4:46 PM

Review dtcheck after deployment.

matmarex mentioned this in T328895: Section titles with empty string are removed by Reply Tool.Feb 6 2023, 12:36 PM
matmarex merged tasks: T328895: Section titles with empty string are removed by Reply Tool, T329008: DiscussionTools moved includeonlys and link.Feb 7 2023, 12:32 PM
matmarex added subscribers: Xiplus, Izno, SunAfterRain and 4 others.
matmarex added a comment.Feb 7 2023, 12:35 PM

I think I misjudged the severity of this problem. I thought it was causing just harmless syntax changes, but these bug reports describe more serious page corruption. I will backport the fix today. Sorry about that.

gerritbot added a comment.Feb 7 2023, 12:35 PM

Change 886980 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/DiscussionTools@wmf/1.40.0-wmf.21] Don't add custom attributes in unwrapParsoidSections()

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

gerritbot added a project: Patch-For-Review.Feb 7 2023, 12:35 PM
matmarex moved this task from Blocked / Needs More Work to Doing on the Editing-team (Kanban Board) board.Feb 7 2023, 12:36 PM
gerritbot added a comment.Feb 7 2023, 3:30 PM

Change 886980 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@wmf/1.40.0-wmf.21] Don't add custom attributes in unwrapParsoidSections()

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

Stashbot added a comment.Feb 7 2023, 3:39 PM

Mentioned in SAL (#wikimedia-operations) [2023-02-07T15:39:38Z] <urbanecm@deploy1002> Started scap: 20a79c55b7073e791e297a5389fa66819f596178: Don't add custom attributes in unwrapParsoidSections() (T328268)

Stashbot added a comment.Feb 7 2023, 3:47 PM

Mentioned in SAL (#wikimedia-operations) [2023-02-07T15:47:13Z] <urbanecm@deploy1002> Finished scap: 20a79c55b7073e791e297a5389fa66819f596178: Don't add custom attributes in unwrapParsoidSections() (T328268) (duration: 07m 34s)

ReleaseTaggerBot edited projects, added MW-1.40-notes (1.40.0-wmf.21; 2023-01-30); removed MW-1.40-notes (1.40.0-wmf.22; 2023-02-06).Feb 7 2023, 4:00 PM
matmarex moved this task from Doing to Ready for Sign Off on the Editing-team (Kanban Board) board.Feb 7 2023, 4:17 PM
matmarex edited projects, added Skipped QA; removed Patch-For-Review.

The issue should be fixed on Wikimedia projects now.

Verfied at https://en.wikipedia.org/wiki/Wikipedia_talk:Sandbox:

ppelberg closed this task as Resolved.Feb 13 2023, 4:57 PM
Stang unsubscribed.Feb 16 2023, 10:33 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits