Page MenuHomePhabricator

Page underneath the overlay flashes for 1 frame when switching between loading overlay and real overlay
Open, NormalPublic

Description

Page underneath the overlay flashes for 1 frame when switching between loading overlay and real overlay. This seems to be a problem with pretty much every overlay in MobileFrontend and Minerva.

Related: T197110 (where this issue was solved for switching between different media overlays).

The video below demonstrates the issue with the following overlays: languages, editor (wikitext), media, talk. (Recorded on https://en.m.wikipedia.org/wiki/The_Fighting_Temeraire using Chrome on desktop, but the same can be seen on real mobile devices.)

It's difficult to consciously notice, so here are frames extracted from the video:

OverlayBefore clickAfter click (loading overlay)FlashReal overlayNotes (feel free to file as separate tasks)
languages
editor (wikitext)
There is an extra loading screen within the overlay with the spinner in a different location:
media
The spinner in this overlay appears to be mispositioned
talk

Developer notes

The issue here is the use of loadModule and OverlayManager.

overlayManager.add( /^\/languages$/, function () {
		var lang = mw.config.get( 'wgUserLanguage' );

		return loader.loadModule( 'mobile.languages.structured', true ).then( function ( loadingOverlay ) {
			var gateway = new PageGateway( api ),
				LanguageOverlay = M.require( 'mobile.languages.structured/LanguageOverlay' );

			return gateway.getPageLanguages( mw.config.get( 'wgPageName' ), lang ).then( function ( data ) {

We hide the loading overlay here...

				loadingOverlay.hide();
				return new LanguageOverlay( {
					currentLanguage: mw.config.get( 'wgContentLanguage' ),
					languages: data.languages,
					variants: data.variants,
					deviceLanguage: getDeviceLanguage()
				} );
			} );
		} );
	} );

_processMatch uses the factoryResult (in this case the return value of loader.loadModule)

					factoryResult.then( function ( overlay ) {
						match.overlay = overlay;
						attachHideEvent( overlay );
						self._show( overlay );
					} );

These things are therefore not 100% connected (ideally loadingOverlay.hide would be managed by the OverlayManager itself)

Given the complexity here, I propose waiting until T212465 has been resolved and then re-evaluating this task. I've left a note on that one.

Event Timeline

matmarex created this task.Jan 24 2019, 9:28 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 24 2019, 9:28 PM

Change 486385 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/MobileFrontend@master] Add #loadModuleAndGetOverlay: a nicer way to use LoadingOverlay

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

Change 486386 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/MobileFrontend@master] Improve loading screens by using #loadModuleAndGetOverlay

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

Change 486387 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/skins/MinervaNeue@master] Improve loading screens by using #loadModuleAndGetOverlay

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

matmarex claimed this task.Jan 24 2019, 9:35 PM
Jdlrobson added a subscriber: Jdlrobson.

Page underneath the overlay flashes for 1 frame when switching between loading overlay and real overlay

Yeh, this is unnecessary and only because of the way we use inheritance in our codebase. The good news is we have a plan for dealing with that that will be commenced post-all hands. We've been looking at the TalkOverlay but it can apply to any overlay:
We're working towards a composition based approach to end up with code like:
https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/471334/

When that's done, we'll be able to move the remaining code into skins.minerva.scripts onto the critical path as it will be using code that's available from page load. I'd love to get you up to speed on those plans and get your help with making that happen!

Neat. So, compared to my proposed approach:

  • Show the loading overlay (handled by OverlayManager)
  • Start loading modules. Once loaded:
    • Replace the loading overlay in-place by the real overlay (handled by OverlayManager)

Your approach instead is:

  • Show a generic overlay (handled by OverlayManager) and put a placeholder spinner inside of it
  • Start loading modules. Once loaded:
    • Replace the placeholder contents of the existing overlay by real contents

I think that's just as elegant. I proposed my solution mostly because it did not require refactoring the existing code for different overlays, except for the one line that loads them. I didn't know you had another approach in the works.

I'll note that it doesn't really have much to do with inheritance. Certainly, you could implement it without the ugly flash with inheritance (as I did), and you could implement it with the ugly flash without inheritance, if you just put a setTimeout(), promise resolution, or something equivalent, between removing the placeolder and inserting real contents. The solution is just to replace the overlay in a single "tick".

Exactly! Also means you can exit the overlay before you finish loading (right now if your connection drops mid overlay load you become stuck!)

The main problem with the inheritance is it means the extended Overlay is not available until its associated code is loaded and if you dont code split you get a larger initial load of JS.

Will probably need some help from you with code review for the refactoring the editor overlay when we tackle that next month.

Change 486385 abandoned by Bartosz Dziewoński:
Add #loadModuleAndGetOverlay: a nicer way to use LoadingOverlay

Reason:
Jon is working on an alternative solution as part of T212465.

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

Change 486386 abandoned by Bartosz Dziewoński:
Improve loading screens by using #loadModuleAndGetOverlay

Reason:
Jon is working on an alternative solution as part of T212465.

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

Change 486387 abandoned by Bartosz Dziewoński:
Improve loading screens by using #loadModuleAndGetOverlay

Reason:
Jon is working on an alternative solution as part of T212465.

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

matmarex removed matmarex as the assignee of this task.Feb 4 2019, 7:44 PM
matmarex removed a project: Patch-For-Review.
Jdlrobson moved this task from Backlog to Bugs on the MinervaNeue board.Feb 5 2019, 6:49 PM
ovasileva triaged this task as Normal priority.Feb 8 2019, 3:01 PM
Jdlrobson claimed this task.Feb 8 2019, 7:34 PM
Jdlrobson updated the task description. (Show Details)Feb 11 2019, 10:58 PM