Page MenuHomePhabricator

Regression: Desktop diff styles for moved paragraphs load alongside mobile
Closed, ResolvedPublic

Description

The structure data diff that displays @ https://commons.m.wikimedia.org/wiki/Special:MobileDiff/388741403 is confusing.

There might be two bugs here but I want to focus on the lead paragraph issue. I suggest the Structured-Data-Backlog team also look at this diff and create a new task if necessary for anything other than the move paragraph issue I am describing here.

The diff can somewhat be reproduced on desktop using diff-type=inline parameter.
https://commons.wikimedia.org/w/index.php?title=File:%D0%A4%D0%BE%D1%82%D0%BE_%D0%90%D0%BB%D0%B8%D1%88%D0%B5%D1%80_%D0%A2%D0%B0%D1%88%D0%BC%D0%B0%D1%82%D0%BE%D0%B2_%D0%91%D0%BE%D0%B3%D0%B0%D1%80%D0%B4%D0%B8%D0%B5%D0%B2%D0%B8%D1%87.jpg&diff=388741403&diff-type=inline

Related to this, on https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:MobileDiff/411429 the move paragraph doesn't seem to match the mocks in T197491 anymore.

Mock:
https://wikimedia.invisionapp.com/public/share/3HEQCYR25#/screens/266827752

Current behaviour:

Developer notes

I think the issue here is that the mediawiki.diff.styles module is loading and wasn't loaded before, so desktop and mobile styles are conflicting.

These rules (and possibly other) should be disabled on Minerva
Option 1) moved to skinStyles in core so that they are substituted by the Minerva ones; 2) override in Minerva:

.mw-diff-movedpara-right::after, .rtl .mw-diff-movedpara-left::after {
    content: '↩';
}
.mw-diff-movedpara-left::after, .rtl .mw-diff-movedpara-right::after {
    content: '↪';
}

QA steps

https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:MobileDiff/412207 should match the mock.

QA Results

ACStatusDetails
1T243235#5841699

Details

Related Gerrit Patches:
mediawiki/skins/MinervaNeue : masterDiff styles for moving paragraphs and empty lines

Related Objects

StatusSubtypeAssignedTask
Declineddchen
OpenNone
OpenNone
DuplicateNone
OpenNone
ResolvedAbit
OpenNone
OpenNone
OpenNone
OpenNone
DuplicateNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
Resolvedppelberg
ResolvedKrinkle
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
StalledNone
ResolvedPetrb
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
Resolvedovasileva

Event Timeline

Jdlrobson renamed this task from Possible Regression(?): Move paragraph diff doesn't seem to be rendering correctly. to Regression: Desktop diff styles for moved paragraphs load alongside mobile.Jan 20 2020, 7:05 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)

Change 566105 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Diff styles for moving paragraphs and empty lines

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

As I explained in,
T117279#5817692, I am seeing something like this on desktop mode. The diff is not shown unified even if I set diff-type=inline in the URL. The below screenshot is of https://commons.wikimedia.beta.wmflabs.org/w/index.php?title=File:Photo_on_14-01-2020_at_11.33.jpg&diff=216482&diff-type=inline&diffmode=source.


Similar problem on mobile. On mobile a structured data diff should be unified to be consistent with the normal wikitext diff. But I see side by side diffs. The below screenshot is of https://commons.m.wikimedia.beta.wmflabs.org/wiki/Special:MobileDiff/216482.

The diff-type inline parameter only works on content types that use TextSlotDiffRenderer. I'm not sure what SlotDiffRenderer this is using, but I'd hazard a guess it's got a custom diff renderer so won't support it. If this diff previously displayed differently in mobile this was likely not intentional.

Waiting on hearing back from StructuredData team about what's going on here as I don't have a knowledge of the inner workings of how these extensions work.

Demian added a subscriber: Demian.Jan 27 2020, 1:05 PM

Change 566105 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Diff styles for moving paragraphs and empty lines

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

phuedx added a subscriber: phuedx.Jan 28 2020, 6:10 PM

Related to this, on https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:MobileDiff/411429 the move paragraph doesn't seem to match the mocks in T197491 anymore.

The moved paragraph treatment now matches the mocks now that rSMIN2bf442db121b: Diff styles for moving paragraphs and empty lines has been merged.

Jdlrobson updated the task description. (Show Details)Jan 28 2020, 10:35 PM
Edtadros reassigned this task from Edtadros to Jdlrobson.Fri, Jan 31, 12:22 PM
Edtadros added a subscriber: Edtadros.

Test Result

Status: ✅ PASS
OS: iOS 13.3
Browser: Chrome, Safari
Device: iPhone 11 Max Pro

Test Artifact(s):

QA steps

✅ AC1: https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:MobileDiff/412207 should match the mock

@Jdlrobson I assigned this to you to take a look at the screen grabs. The elements and paragraphs match the mock so I passed it. What concerns me is the lower overlay that stays open and is actually cut off on the bottom. Let me know if there's something in my user settings that may be causing this.

Edtadros updated the task description. (Show Details)Fri, Jan 31, 12:22 PM

LGTM. I'll let @alexhollender have the final say on the design of https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:MobileDiff/412207 s

Please flag anything that looks wrong and move to needs more work OR ready for sign off - whichever is more appropriate.

alexhollender removed alexhollender as the assignee of this task.Tue, Feb 11, 3:21 PM

This looks correct to me. As somewhat of a side-note: I'm not sure if the original design is fully suited to deal with edits with multiple paragraphs moved. Perhaps someday we can create a task and try for a better design there 🙃

ovasileva closed this task as Resolved.Thu, Feb 13, 11:16 AM
ovasileva claimed this task.