Page MenuHomePhabricator

HTML for VisualDiff mode button appears in mobile diff pages (disabled by hack)
Closed, ResolvedPublic

Description

As the diffs for mobile and desktop site converge one of the side effects was the visual diff control now loads in the mobile diff sans the javascript that makes it work.

The visual diff control should check to see if it is in mobile mode (using MobileContext::shouldDisplayMobileView if available) or should integrate itself into the mobile site.

For the time being it is being hidden via hack in MobileFrontend in resources/mobile.special.mobilediff.styles/mobilediff.less.
https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/master/resources/mobile.special.mobilediff.styles/mobilediff.less#L9

Acceptance criteria

  • The control is either conditionally added or added and works.
  • MobileFrontend hack removed

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

Event Timeline

Jdlrobson created this task.Jan 9 2020, 5:12 PM
Restricted Application added a project: VisualEditor. · View Herald TranscriptJan 9 2020, 5:12 PM
Jdlrobson updated the task description. (Show Details)Jan 9 2020, 5:13 PM
Jdlrobson renamed this task from VisualDiff mode button appears in diff pages (disabled) to HTML for VisualDiff mode button appears in mobile diff pages (disabled).Jan 9 2020, 5:26 PM

VisualDiff feature doesn't work without JS, so why not load the button using JS? It doesn't make any sense.

Jdlrobson updated the task description. (Show Details)Jan 9 2020, 5:42 PM

Question: @Jdlrobson do you plan to work on this? Or the assigned field was just inherited from the parent task?

Jdlrobson renamed this task from HTML for VisualDiff mode button appears in mobile diff pages (disabled) to HTML for VisualDiff mode button appears in mobile diff pages (disabled by hack).Jan 10 2020, 5:11 PM
Jdlrobson removed Jdlrobson as the assignee of this task.
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task from Needs triage to Triaged on the Mobile board.Jan 10 2020, 6:01 PM

VisualDiff feature doesn't work without JS, so why not load the button using JS? It doesn't make any sense.

This way we avoid the contents of the page moving around when JS code loads (it only changes the button from enabled to disabled). The button is supposed to be hidden when JS is disabled, but the code for that doesn't get loaded on mobile.

Change 568556 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] Enable visual diff code on mobile

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

Change 568557 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/MobileFrontend@master] Re-enable visual diff mode

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

Change 568556 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Enable visual diff code on mobile

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

Change 568557 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Re-enable visual diff mode

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

Masumrezarock100 added a comment.EditedJan 29 2020, 8:55 PM

This way we avoid the contents of the page moving around when JS code loads (it only changes the button from enabled to disabled). The button is supposed to be hidden when JS is disabled, but the code for that doesn't get loaded on mobile.

Thanks for explaining that. But the above patches looks like they will load the JS on mobile. Can we be sure that the VisualDiff feature will work properly on tall and small screens?

This is working. But the design looks bad on mobile. Leaving this to @alexhollender for review. Could you take a look at https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:MobileDiff/411223...412204?diffmode=visual (with VisualDiff beta feature enabled) and check if this design is okay?

Thanks for explaining that. But the above patches looks like they will load the JS on mobile. Can we be sure that the VisualDiff feature will work properly on tall and small screens?

It works okay, no worse than visual editor. It's probably pretty bad in some cases, but there's nothing we can do if the content itself is not properly made for mobile.

This is working. But the design looks bad on mobile.

If you mean just the switcher buttons, then it seems Jon and the gang have some further plans to redesign it, see parent task T240608: Standardise a control for switching diff types from side by side to inline/visual. It should be easier to work on that now that we fixed the issue here.

Masumrezarock100 added a comment.EditedJan 29 2020, 9:39 PM

The switched buttons looks fine to me but I am worried about the diff itself. The different colors in diffs don't look good at all. VisualDiff was designed for the desktop side by side diff. The design looks on mobile because mobile only shows inline (unified) diffs which was recently introduced to core, and this extension is not optimized for such diff design.


Screenshot of https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:MobileDiff/411223...412204?diffmode=visual

Given its a beta feature I am not concerned about these issues right now but please open a new ticket. Possibly hiding the button may be preferable in the meantime but there is also value to displaying something in beta in a broken form.

This is working. But the design looks bad on mobile. Leaving this to @alexhollender for review. Could you take a look at https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:MobileDiff/411223...412204?diffmode=visual (with VisualDiff beta feature enabled) and check if this design is okay?

It seems like there might be some other layout issues with the page, but as far as I can see the toggle button looks fine:

JTannerWMF added a subscriber: JTannerWMF.

I believe this is ready for your sign off.

Given its a beta feature I am not concerned about these issues right now but please open a new ticket. Possibly hiding the button may be preferable in the meantime but there is also value to displaying something in beta in a broken form.

Created T244673.

matmarex moved this task from To Triage to Triaged on the VisualEditor board.Feb 12 2020, 5:24 PM
marcella closed this task as Resolved.Mar 4 2020, 5:17 PM
Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptMar 4 2020, 5:17 PM