Page MenuHomePhabricator

Regression: issues with MobileDiff
Closed, ResolvedPublic

Assigned To
Authored By
Masumrezarock100
Jan 9 2020, 3:26 AM
Referenced Files

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

Screenshot_20200109-093236.png (480×960 px, 145 KB)

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.

qa steps

Compare visual diff button shows on desktop (https://en.wikipedia.beta.wmflabs.org/w/index.php?title=Diff_test&diff=412205&diffmode=source)

Confirm the button is clickable and shows a visual diff.

3. Mobile diff width is broken

Steps to reproduce:

Screenshot_20200109-095241.png (960×480 px, 98 KB)

QA steps

Verify that the diff on https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:MobileDiff/412205 does not cause any width problems on mobile.

QA Results

ACStatusDetails
1T242310#5841370
2T242310#5841370
3T242310#5841370

Related Objects

StatusSubtypeAssignedTask
Declineddchen
OpenNone
OpenNone
DuplicateNone
OpenFeatureNone
OpenBUG REPORTNone
OpenNone
StalledNone
OpenFeatureNone
DuplicateNone
ResolvedNone
OpenNone
OpenNone
OpenFeatureNone
OpenNone
ResolvedNone
ResolvedNone
OpenFeatureNone
OpenNone
ResolvedHalfak
OpenNone
OpenNone
OpenFeatureNone
StalledNone
OpenNone
OpenNone
OpenNone
ResolvedPetrb
OpenNone
OpenNone
Resolvedtstarling
OpenNone
DeclinedNone
ResolvedBUG REPORTJdlrobson
ResolvedNone
Resolvedovasileva
Resolved marcella
Resolvedmatmarex
ResolvedBUG REPORTEsanders
Resolvedovasileva

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Masumrezarock100 added a subscriber: Jdlrobson.

Adding the parent task's assignee as a subscriber.

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.

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.

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.

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.Jan 9 2020, 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: rMW39bc331011e7: 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?

Screenshot_20200110-011838.png (480×960 px, 135 KB)

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.

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

Screenshot_20200110-020952.png (480×960 px, 108 KB)

(diff in https://commons.m.wikimedia.beta.wmflabs.org/wiki/Special:MobileDiff/216275)
with
Screenshot_20200110-013406.png (480×960 px, 135 KB)

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

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.

Screenshot_20200110-194459.png (480×960 px, 42 KB)

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.

Screenshot_20200110-194459.png (480×960 px, 42 KB)

Bug 3. Mobile diff width is broken

BeforeAfter
Screenshot_20200109-095241.png (960×480 px, 98 KB)
Screenshot_20200110-195100.png (960×480 px, 148 KB)

@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?

image.png (1×752 px, 232 KB)

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?

Screenshot_20200110-011838.png (480×960 px, 135 KB)

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.

In T242310#5818081, @alexhollender wrote:

@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.

In T242310#5818081, @alexhollender wrote:

@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.

@nray @phuedx to test this you need to enable wikidiff2. Right now when you visit Special:MobileDiff you are seeing the 3rd party version of the page rather than the one we use in production.

The 3rd party version is removed in https://gerrit.wikimedia.org/r/556811 (T240622) - and one of the motivations for doing this was that we should be testing the production version locally and not maintaining unnecessary code (note you can confirm the absence of wikidiff2 by checking out that patch as well - you will see the unified view). I scheduled this patch for prioritisation/discussion as we can remove this now at our leisure.

For testing this patch and further diff related patches please install wikidiff2 (https://www.mediawiki.org/wiki/Wikidiff2) or alternatively mock it using the following code in LocalSettings.php

function wikidiff2_do_diff( $a, $b, $c ) {
	return '<tr>
	<td colspan="2" class="diff-lineno"><!--LINE 1--></td>
	<td colspan="2" class="diff-lineno"><!--LINE 1--></td>
</tr>
<tr>
	<td class="diff-marker">&#160;</td>
	<td class="diff-context"><div>Testaasasasa</div></td>
	<td class="diff-marker">&#160;</td>
	<td class="diff-context"><div>Testaasasasa</div></td>
</tr>
<tr>
	<td class="diff-marker">&#160;</td>
	<td class="diff-context"><div>as</div></td>
	<td class="diff-marker">&#160;</td>
	<td class="diff-context"><div>as</div></td>
</tr>';

}
function  wikidiff2_inline_diff( $a, $b, $c ) {
	return '<tr><td colspan="4"><div class="mw-diff-inline-header"><!-- LINES 369,369 --></div>
	<div class="mw-diff-inline-context">*{{Vandal|Tibor1964}}</div>
	<div class="mw-diff-inline-context">Might not be spam bot, but uploads needs wiping and edits reverting.--[[User:BevinKacon|BevinKacon]] ([[User talk:BevinKacon|&lt;span class="signature-talk"&gt;{{int:Talkpagelinktext}}&lt;/span&gt;]]) 21:13, 8 January 2020 (UTC)</div>
	<div class="mw-diff-inline-added mw-diff-empty-line"><ins>&nbsp;</ins></div>
	<div class="mw-diff-inline-added"><ins>== [[User:Rodrigo.Argenton|Rodrigo.Argenton]] ==</ins></div>
	<div class="mw-diff-inline-added"><ins>* User: {{User3|1=Rodrigo.Argenton}}</ins></div>
	<div class="mw-diff-inline-added"><ins>* Reasons for reporting: Some of Rodrigo\'s uploads are using {{tl|Artwork}} template in a way it was never designed to be used, resulting files like [[:File:Centro Português de Fotografía por Rodrigo Tetsuo Argenton (6).jpg]] displaying nonsense info (see "collection" field for example), and ending up in the maintenance categories I am trying to empty. I spend time to [https://commons.wikimedia.org/w/index.php?title=File%3ACentro_Portugu%C3%AAs_de_Fotograf%C3%ADa_por_Rodrigo_Tetsuo_Argenton_%286%29.jpg&amp;type=revision&amp;diff=386185780&amp;oldid=386155287 correct the issues], but were repeatedly reverted with some insulting remarks. My attempt to, explain the issues I am trying to fix at his talk page, got rather hostile response, and eventually failed when [https://commons.wikimedia.org/w/index.php?title=User_talk%3ARodrigo.Argenton&amp;type=revision&amp;diff=386235990&amp;oldid=386235668 he deleted the whole discussion].
	Rodrigo is a great photographer with 67k uploads to his name, but he seem to have a problem with [[Commons:Ownership of pages and files|ownership of his files]] and with hostility towards others. He seem the be frequent visitor on this forum over the years: [[Commons:Administrators\'_noticeboard/Vandalism/Archive_7#Rodrigo.Argenton|2014]], [[Commons:Administrators\'_noticeboard/Blocks_and_protections/Archive_13#Rodrigo.Argenton|2014]], [[Commons:Administrators\'_noticeboard/User_problems/Archive_63#Personal_attacks_User%3ARodrigo.Argenton_(RTA)|2017]], [[Commons:Administrators\'_noticeboard/User_problems/Archive_67#Rodrigo.Argenton|2018]], [[Commons:Administrators\'_noticeboard/User_problems/Archive_78#Rodrigo.Argenton|2019]]. Can someone else have a look at this? --[[User:Jarekt|Jarekt]] ([[User talk:Jarekt|&lt;span class="signature-talk"&gt;{{int:Talkpagelinktext}}&lt;/span&gt;]]) 04:02, 9 January 2020 (UTC)</ins></div>
	</td></tr>';
}

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

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

@Jdlrobson: Can this be tested on the Beta Cluster?

@Jdlrobson I am afraid the 2nd problem's OA steps are outdated. The patches in T242351 got merged, so I am now seeing the buttons (it wasn't used to, as I confirmed above) if I have enabled the beta feature.

Edtadros subscribed.

@Jdlrobson if I'm reading this correctly I need IP permissions. I'm using Edtadros-beta1 as my test user.

@Jdlrobson if I'm reading this correctly I need IP permissions. I'm using Edtadros-beta1 as my test user.

I am not sure what you meant by IP permissions. By making edits as a IP, I meant that you needed to make edits while you are logged out/using incognito mode. The only permission you need at the moment is "patrol" rights which let's one mark edits as patrolled. You won't be able to see the mark as patrolled link without this permission. And done. You should now be able to see the patrol links on beta Commons.

@Masumrezarock100 Thanks for clarifying what IP meant, and the patrol rights.

Test Result

Status: ✅ PASS
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: iPhoneX, iPad, iPad Pro

Test Artifact(s):

QA Steps

✅ AC1. Mark as patrolled link isn't being shown in this new Special:MobileDiff
Go to https://commons.m.wikimedia.beta.wmflabs.org/wiki/Test on Commons beta cluster since patrolling is enabled there
Make an edit as an IP.
Login as a user with Patrol rights and compare the both mobile and desktop diff. eg. https://commons.m.wikimedia.beta.wmflabs.org/wiki/Special:MobileDiff/216260 with https://commons.wikimedia.beta.wmflabs.org/w/index.php?title=Test&diff=216260&diffmode=source

commons.wikimedia.beta.wmflabs.org_w_index.php_title=Test&diff=216685(iPad Pro).png (2×2 px, 884 KB)
commons.m.wikimedia.beta.wmflabs.org_wiki_Special_MobileDiff_216685(iPhone X).png (2×1 px, 175 KB)

✅ AC2. Buttons to switch to a VisualDiff doesn't work
Compare visual diff button shows on desktop (https://en.wikipedia.beta.wmflabs.org/w/index.php?title=Diff_test&diff=412205&diffmode=source)
Confirm the button is clickable and shows a visual diff.

Difference between revisions of _Diff test_ - Wikipedia, the free encyclopedia.gif (480×640 px, 1 MB)

✅ AC3. Mobile diff width is broken
QA steps
Verify that the diff on https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:MobileDiff/412205 does not cause any width problems on mobile.

en.m.wikipedia.beta.wmflabs.org_wiki_Special_MobileDiff_412205(iPhone X).png (2×1 px, 355 KB)
en.m.wikipedia.beta.wmflabs.org_wiki_Special_MobileDiff_412205(iPhone X) (2).png (2×1 px, 354 KB)