Page MenuHomePhabricator

Regression: issues with MobileDiff
Closed, ResolvedPublic

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.

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:

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
OpenNone
OpenNone
OpenNone
StalledNone
DuplicateNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
Resolvedppelberg
ResolvedKrinkle
OpenNone
OpenNone
OpenNone
ResolvedHalfak
OpenNone
OpenNone
StalledNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
ResolvedPetrb
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
Resolvedovasileva
Resolvedmarcella
OpenNone
ResolvedBUG REPORTEsanders
Resolvedovasileva

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Masumrezarock100 updated the task description. (Show Details)
Restricted Application added a project: VisualEditor. · View Herald TranscriptJan 9 2020, 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.Jan 9 2020, 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.EditedJan 9 2020, 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.EditedJan 9 2020, 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.EditedJan 9 2020, 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.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?


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.EditedJan 9 2020, 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)Jan 9 2020, 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.EditedJan 10 2020, 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.Jan 23 2020, 6:07 PM

@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

phuedx added a comment.EditedJan 28 2020, 6:00 PM

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

Jdlrobson reassigned this task from Jdlrobson to Edtadros.Jan 28 2020, 10:31 PM
Jdlrobson updated the task description. (Show Details)

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

Jdlrobson updated the task description. (Show Details)Jan 29 2020, 11:55 PM
Edtadros reassigned this task from Edtadros to Jdlrobson.Jan 30 2020, 1:47 AM
Edtadros added a subscriber: Edtadros.

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

Masumrezarock100 added a comment.EditedJan 30 2020, 7:51 AM

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

Edtadros reassigned this task from Edtadros to ovasileva.Jan 31 2020, 10:53 AM

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

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

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

Edtadros updated the task description. (Show Details)Jan 31 2020, 10:54 AM
ovasileva closed this task as Resolved.Feb 4 2020, 2:20 PM

Looks good!

Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptFeb 4 2020, 2:20 PM