Page MenuHomePhabricator

Simply cloning diff header disrupts user scripts and causes other issues
Closed, ResolvedPublic

Description

For T187614: Metadata (author, edit comment, navigation links etc) not shown in historic visual diff, the Visual Diff clones the header of the diff. While this works, there are some problems with this approach:

  1. It will clone any tr.diff-title, even if it is some user-generated content from the article shown below.
  2. This results in duplicated IDs, e.g. #mw-diff-otitle1, which is both invalid and could cause problems with any of these and similar scripts.

These issues aren't just hypothetic ones, I struggled with both while updating my script https://de.wikipedia.org/wiki/Benutzer:Schnark/js/diff.js, which adds a third diff mode. (Though due to T171437 this script already has a lot of hacks, so it didn't really matter to add even more.)


Other bug reports:

Event Timeline

Schnark created this task.Apr 23 2018, 7:07 AM
Restricted Application added a project: VisualEditor. · View Herald TranscriptApr 23 2018, 7:07 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
matmarex claimed this task.EditedApr 24 2018, 5:43 PM
matmarex added a subscriber: matmarex.

I suppose we could move it instead of duplicating it, would that work?

Deskana triaged this task as Normal priority.Apr 24 2018, 6:55 PM
Deskana edited projects, added VisualEditor (Current work); removed VisualEditor.

Alternatively one could wrap the header in <thead>, and the actual diff in <tbody> and then only hide the body when switching to visual (and toggle the data-attributes).

Izno added a subscriber: Izno.Apr 28 2018, 8:27 PM

Many people complaining about missing twinkle links in the diff view. https://en.wikipedia.org/wiki/Wikipedia_talk:Twinkle#Missing_Rollback_Options_in_Diff_View

Thanks. This is in progress, so hopefully a fix should be here soon.

Deskana renamed this task from Simply cloning diff header doesn't seem like a good idea to Simply cloning diff header disrupts user scripts and causes other issues.Apr 30 2018, 1:25 PM
Deskana added a subscriber: Waddie96.

Change 429845 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] ve.init.mw.DiffPage.init: Do not duplicate diff table header

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

Sorry, I didn't anticipate this breaking other scripts, and I wasn't checking my bugmail over the weekend. I will try to get the fix deployed later today.

Change 429845 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] ve.init.mw.DiffPage.init: Do not duplicate diff table header

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

Change 429986 had a related patch set uploaded (by Jforrester; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@wmf/1.32.0-wmf.1] ve.init.mw.DiffPage.init: Do not duplicate diff table header

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

I've scheduled this for SWAT in 90 minutes' time.

matmarex updated the task description. (Show Details)Apr 30 2018, 9:35 PM
matmarex raised the priority of this task from Normal to High.Apr 30 2018, 10:24 PM

Change 429986 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@wmf/1.32.0-wmf.1] ve.init.mw.DiffPage.init: Do not duplicate diff table header

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

Mentioned in SAL (#wikimedia-operations) [2018-04-30T23:38:01Z] <catrope@tin> Synchronized php-1.32.0-wmf.1/extensions/VisualEditor/modules/ve-mw/init/ve.init.mw.DiffPage.init.js: T192755 (duration: 00m 59s)

Sorry, I didn't anticipate this breaking other scripts, and I wasn't checking my bugmail over the weekend. I will try to get the fix deployed later today.

It's fine. It's a beta feature, so things like this will break sometimes.

Deskana closed this task as Resolved.May 1 2018, 8:12 AM