Page MenuHomePhabricator

Regression: issues with MobileDiff
Open, HighPublic

Description

Following T117279 some changes were made to MobileFrontend (Special:MobileDiff) and Mediawiki core. However it introduced some regressions. I have found these two bugs for now. I suspect there could be other bugs.

1. Mark as patrolled link isn't being shown in this new Special:MobileDiff

Steps to Reproduce:

  • Go to a wiki where patrolling is enabled
  • Find an unpatrolled edit, you must also have patrol right to be able to see the mark as patrolled button
  • View and compare its diff in both mobile and desktop mode

Actual Results:

  • A "mark as patrolled" button used to appear below the mobile version of an unpatrolled diff. This button in mobilediff is hidden but it is still intact in the desktop diff.

Expected Results:

  • Mark as patrolled button is displayed.

QA steps

https://phabricator.wikimedia.org/T242310#5790108

2. Buttons to switch to a VisualDiff doesn't work

Steps to Reproduce:

  • Enable VisualDiff beta feature.
  • View a mobilediff
  • You'll see two grouped buttons. One enabled and one disabled.
  • Click on the enabled button since you can not click on the disabled button

Actual Results:

  • Nothing happens! You can not switch to a VisualDiff! It seems the JS code to load a visual diff isn't being loaded.

Expected Results:

  • Visual diff works as usual like in the desktop version.

3. Mobile diff width is broken

Steps to reproduce:

Details

Related Gerrit Patches:
mediawiki/skins/MinervaNeue : masterDiff styles for moving paragraphs and empty lines
mediawiki/core : wmf/1.35.0-wmf.14Enable mediawiki.page.patrol.ajax on mobile
mediawiki/extensions/MobileFrontend : wmf/1.35.0-wmf.14Hot fixes for mobile diff page
mediawiki/extensions/MobileFrontend : masterHot fixes for mobile diff page
mediawiki/core : masterEnable mediawiki.page.patrol.ajax on mobile

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
OpenNone
OpenNone

Event Timeline

Masumrezarock100 added a project: Regression.
Masumrezarock100 updated the task description. (Show Details)
Restricted Application added a project: VisualEditor. · View Herald TranscriptThu, Jan 9, 4:00 AM
Masumrezarock100 added a subscriber: Jdlrobson.

Adding the parent task's assignee as a subscriber.

Masumrezarock100 updated the task description. (Show Details)
Masumrezarock100 updated the task description. (Show Details)
Jdlrobson claimed this task.Thu, Jan 9, 4:33 PM
Jdlrobson triaged this task as Unbreak Now! priority.

Possibly deploy blocker? Seems like the core change I made had some unexpected side effects.

The width issue seems like the most important to fix here. Looks like the following may be all that's needed:

ins { display: block; }

As a stretch I'll see if I can also see why the patrol button doesn't show and hide the visual diff link button.

Masumrezarock100 added a comment.EditedThu, Jan 9, 4:42 PM

Possibly deploy blocker? Seems like the core change I made had some unexpected side effects.

IMHO, a big change like this should have been QAed first before deploying to production wikis.

Jdlrobson added a comment.EditedThu, Jan 9, 4:48 PM

It was QAed but we're only human :) There are lots of extensions and different settings, different user rights and different user preferences on different wikis and different users. It would take us a week to manually test all the combinations. Sadly we do rely on bug reports like this a lot of the time (so thanks for catching!)

The patrol link is particularly hard to QA and right now I'm struggling to find an example for testing on https://en.wikipedia.beta.wmflabs.org/ can you help me find one? I think I can see what happened here.

Masumrezarock100 added a comment.EditedThu, Jan 9, 4:57 PM

The patrol link is particularly hard to QA and right now I'm struggling to find an example for testing on https://en.wikipedia.beta.wmflabs.org/ can you help me find one? I think I can see what happened here.

Steps to reproduce:

I have granted you patrol rights on beta Commons so you should be able to see the patrol link/button.

Change 563226 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Hot fixes for mobile diff page

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

I have granted you patrol rights on beta Commons so you should be able to see the patrol link/button.

Thanks that's helpful. Patch above should fix all 3 issues.

Change 563230 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@wmf/1.35.0-wmf.14] Hot fixes for mobile diff page

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

Change 563232 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Enable mediawiki.page.patrol.ajax on mobile

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

Change 563234 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@wmf/1.35.0-wmf.14] Enable mediawiki.page.patrol.ajax on mobile

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

Change 563232 merged by jenkins-bot:
[mediawiki/core@master] Enable mediawiki.page.patrol.ajax on mobile

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

Change 563226 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hot fixes for mobile diff page

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

Change 563230 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@wmf/1.35.0-wmf.14] Hot fixes for mobile diff page

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

Change 563234 merged by jenkins-bot:
[mediawiki/core@wmf/1.35.0-wmf.14] Enable mediawiki.page.patrol.ajax on mobile

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

Mentioned in SAL (#wikimedia-operations) [2020-01-09T19:35:55Z] <urbanecm@deploy1001> Synchronized php-1.35.0-wmf.14/extensions/MobileFrontend/: SWAT: 31d3be7: Hot fixes for mobile diff page (T242310) (duration: 01m 09s)

Jdlrobson added a subscriber: Urbanecm.

should be all fixed now. Thanks @Urbanecm for the SWAT!

Jdlrobson lowered the priority of this task from Unbreak Now! to High.Thu, Jan 9, 7:37 PM

Mentioned in SAL (#wikimedia-operations) [2020-01-09T19:51:00Z] <urbanecm@deploy1001> Synchronized php-1.35.0-wmf.14/resources/Resources.php: SWAT: 39bc331: Enable mediawiki.page.patrol.ajax on mobile (T242310) (duration: 01m 05s)

4. Possible diff style issues

I am noticing some style issues. Background colors (red and green) extend to the whole line (covers up the spaces without any content). Is this intended or a bug?


Diff in https://commons.m.wikimedia.org/wiki/Special:MobileDiff/386331930

I am noticing some style issues. Background colors (red and green) extend to the whole line (covers up the spaces without any content). Is this intended or a bug?

Looks fine to me but I defer to @alexhollender if there is a styling problem here.

Masumrezarock100 added a comment.EditedThu, Jan 9, 8:10 PM

I am noticing some style issues. Background colors (red and green) extend to the whole line (covers up the spaces without any content). Is this intended or a bug?

Looks fine to me but I defer to @alexhollender if there is a styling problem here.

Sorry for not providing another screenshot to compare. Try to compare


(diff in https://commons.m.wikimedia.beta.wmflabs.org/wiki/Special:MobileDiff/216275)
with

Diff in http://www.shoutwiki.com/wiki/Special:MobileDiff/30942. <!-- I had to use the Shoutwiki sandbox because it runs on a older version of Mediawiki and all Wikimedia sites have the latest stable version installed. -->
I have no idea why there is extra red color space after the sentence "Note that content added here will not stay permanently and will be deleted regularly." in the screenshot. @alexhollender this looks like a minor issue.

Jdlrobson updated the task description. (Show Details)Thu, Jan 9, 8:23 PM

@Jdlrobson looks like all of the bugs listed here has been fixed except the design issue which will be reviewed by Alex. But I am unsure if it's OK to change the patrol button to a link. The link is smaller than the button, it's located under the diff, and it will probably go unnoticed if the diff is long. Also I don't see the point of scrolling all the way to the bottom just to mark a revision as patrolled. It would probably better if the patrol link is displayed at the top, just like the desktop version. Thoughts?

@Ammarpad I was wondering if you found any bugs. If you have, now might be the best time to fix them as part of this task.

Masumrezarock100 added a comment.EditedFri, Jan 10, 2:25 PM

QA

Device: Android 7.0 Chrome 79.0
Screen size: 600 x 1200 px
Status: PASS

Bug 1. Mark as patrolled link isn't being shown in this new Special:MobileDiff

  • Ensure that a "mark as patrolled" link appears at the bottom of the diff. ✅ OK.

Bug 2. Buttons to switch to a VisualDiff doesn't work

  • Ensure that the buttons to switch to visual and wikitext diffs doesn't appear in mobile diff if VisualDiff beta feature is enabled. ✅ OK.

Bug 3. Mobile diff width is broken

BeforeAfter

Moving to design review for Alex. QA is passed.

This comment was removed by Masumrezarock100.

@Jdlrobson another regression. Same as T117279#5817692

Steps to reproduce

Result

  • The diff is still shown side by side instead of showing an inline diff.

@Jdlrobson @Masumrezarock100 I am having trouble reviewing these fixes.

1. Mark as patrolled link — I am unable to see the patrolled link but this is maybe a configuration issue?
2. Buttons to switch to a VisualDiff — I am unable to enable this beta feature, I get an error: "The Wikimedia Commons database is temporarily in read-only mode for the following reason: The database is read-only until replication lag decreases.").
3. Mobile diff width — the width seems to be okay, however words are breaking in a way that seems unexpected?

4. Possible diff style issues

I am noticing some style issues. Background colors (red and green) extend to the whole line (covers up the spaces without any content). Is this intended or a bug?


Diff in https://commons.m.wikimedia.org/wiki/Special:MobileDiff/386331930

To me it seems easier to read when the background color covers the entire paragraph, rather than leaving white gaps between each line.

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

To me it seems easier to read when the background color covers the entire paragraph, rather than leaving white gaps between each line.

The patch above should take care of this.

@Jdlrobson @Masumrezarock100 I am having trouble reviewing these fixes.
1. Mark as patrolled link — I am unable to see the patrolled link but this is maybe a configuration issue?

I don't think it's a site configuration issue. The patrol right is available to patrollers, admins, and image reviewers on Commons. I assume https://commons.m.wikimedia.beta.wmflabs.org/wiki/Special:Contributions/Alex_Hollender_(admn) is your beta cluster account? I have granted you the patroller flag on beta Commons. You should be able see patrol links from your account now if you view an unpatrolled edit. Follow the steps in T242310#5790108.

@Jdlrobson @Masumrezarock100 I am having trouble reviewing these fixes.
2. Buttons to switch to a VisualDiff — I am unable to enable this beta feature, I get an error: "The Wikimedia Commons database is temporarily in read-only mode for the following reason: The database is read-only until replication lag decreases.").

You will likely be able to enable the beta feature once the lag decreases. Nothing abnormal.

ovasileva removed Jdlrobson as the assignee of this task.Thu, Jan 23, 6:07 PM