Page MenuHomePhabricator

MFA: Drawers and Overlays should not auto-append it self to the body element
Closed, ResolvedPublic5 Estimated Story Points

Description

Creating Drawers and Overlay's have the side effect of adding themselves to the DOM. This is not useful for tests and causes compatibility problems when trying to get our library working in skins (which the editing team is currently working towards doing)

Problem

Drawers and Overlays automatically append themselves to the DOM instead Drawer.prototype.postRender:

        postRender: function () {
-               var self = this;
-               // This module might be loaded at the top of the page e.g. Special:Uploads
-               // Thus ensure we wait for the DOM to be loaded
-               util.docReady( function () {
-                       self.appendTo( self.appendToElement );
-                       self.$el.parent().addClass( 'has-drawer' );
-               } );

and Overlay.prototype.show

	/**
	 * Attach overlay to current view and show it.
	 * @memberof Overlay
	 * @instance
	 */
	show: function () {
		var self = this,
			$html = util.getDocument(),
			$window = util.getWindow();

		this.$el.appendTo( this.appendToElement );

In Drawer, inside Drawer.prototype.onShowDrawer a setTimeout hacks around the async nature of Drawer.prototype.postRender:

        onShowDrawer: function () {
-               var self = this;
+               var self = this,
+                       $window = util.getWindow();
 
                this.$el.parent().addClass( 'drawer-visible' );
 
-               setTimeout( function () {
-                       var $window = util.getWindow();
-                       $window.one( 'click.drawer', self.hide.bind( self ) );
-                       if ( self.closeOnScroll ) {
-                               $window.one( 'scroll.drawer', self.hide.bind( self ) );
-                       }
-               }, self.minHideDelay );

Going forward, we'll want to find a better way to do this - where Drawer's/Overlay's are explicitly added to the DOM by OverlayManager and/or DrawerManager (which currently doesn't exist)

Developer notes

To do this safely, for any View, we can check if this.$el.parents().length to tell whether the node has already been inserted in the DOM. Where it hasn't we can continue with the existing behaviour but use mw.log.warn to tell the developer that something needs fixing.

I've written a patch to show what this might look like:
https://gerrit.wikimedia.org/r/473143

Acceptance criteria

  • An auto-added overlay/drawer triggers a JavaScript console warning
  • The OverlayManager has a public method which can be used to append Drawers or Overlays to a page
  • A Drawer and Overlay has been updated to use this mechanism
  • Drawers/Overlays will be added to body rather than mw-mf-viewport going forward to support the editing team's work in T198765
  • Update the OverlayManager tests

Event Timeline

Jdlrobson renamed this task from Drawer should not auto-append it self to the body element to Drawers and Overlays should not auto-append it self to the body element.Nov 9 2018, 12:23 AM
Jdlrobson added a subscriber: matmarex.
Jdlrobson triaged this task as Medium priority.Nov 12 2018, 11:26 PM
Jdlrobson updated the task description. (Show Details)

Change 473143 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] POC: Drawers and Overlay's are no longer in charge of adding themselves to DOM

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

Change 473143 abandoned by Jdlrobson:
POC: Drawers and Overlay's are no longer in charge of adding themselves to DOM

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

Jdlrobson renamed this task from Drawers and Overlays should not auto-append it self to the body element to MFA: Drawers and Overlays should not auto-append it self to the body element.Nov 15 2018, 6:52 PM
Jdlrobson raised the priority of this task from Medium to High.Dec 14 2018, 4:46 PM
Jdlrobson set the point value for this task to 5.Dec 18 2018, 10:17 PM

via async estimation

The AC states that OverlayManager should have a public show method that can be used by Overlays and Drawers.
I find that a little bit confusing because it suggests that Overlays and Drawers should be able to call OverlayManager,
and as the name suggests, OverlayManager should be the one managing the overlays.

I think if Overlays and Drawers want to close themselves (as is their right), they should do so by calling the router instead, and then based on that route, the OverlayManager should close them.

Change 482020 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/MobileFrontend@master] OverlayManager attaches Overlays to DOM

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

^ does that clear things up? Looks like you've done this with "OverlayManager.prototype.attachOverlay"

Jdlrobson raised the priority of this task from High to Needs Triage.Jan 3 2019, 10:38 PM

Change 482783 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/MobileFrontend@master] Drawer.js DOM inserted onShowDrawer instead of on initialization

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

Change 482020 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] OverlayManager attaches Overlays to DOM

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

@Jdrewniak Am noticing that after commit 12f24ee7abe0f18af33f2729b0308193e467cbc0, clicking the languages icon no longer shows the white screen loader. Please let me know if you have trouble reproducing

@nray yup I was able to reproduce, for clarity, the issue is the semi-transparent overlay that appears when you click the language icon doesn't appear.

language-overlay.gif (902×800 px, 1 MB)

Change 483803 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/MobileFrontend@master] Expose OverlayManager show/hide methods publically

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

Change 484261 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Fix regression: LoadingOverlay is not being shown

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

Change 484261 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Fix regression: LoadingOverlay is not being shown

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

This task has proven difficult as I’ve learned more about how Overlays and Drawers work, and how they interact with OverlayManager.

The goal of having Overlays not auto-append themselves to the DOM has been partially achieved by moving this behaviour to the OverlayManager, but this approach also has its downsides.

I’m summarizing my findings here, and further work should be split out into separate tasks.

1. Overlay.show no longer actually shows the overlay.

By moving the DOM appending into the OverlayManager, the Overlay can no longer append itself to the DOM. Overlay.show however, still manipulates DOM outside it $el object, appending a class to HTML element. In essence, Overlays can still mutate the global state.

This wouldn’t be an issue if OverlayManager managed all Overlays, but OverlayManager only knows about Overlays that are attached to routes.

2. Some Overlays are shown/hidden outside the OverlayManager

rlModuleLoader.js actually creates, shows and returns an overlay as a potential side-effect of loading a module. This overlay has to be manually hidden by the requesting module, even if that module itself returns an Overlay that’s passed to the OverlayManager. Category, Languages, and Nearby Overlays are all sneaking loadingOverlays behind the OverlayManagers back.

It should be considered whether Overlays should be show/hiding themselves without the OverlayManager at all, and if they shouldn’t, code relating to this loadingOverlay should be refactored and Overlay.show should be renamed to something like Overlay.onShow.

3. OverlayManager is ill-suited for Drawers

The OverlayManager is primarily a router, since it responds to routes, and executes a show/hide function based on those routes.
It would have to be entirely refactored to accept objects that do not have a route attached. However, an entity that manages both Drawers and Overlays should exist, because neither should be open at the same time, leading to bugs like this one:

IMG_6467.PNG (1×750 px, 339 KB)

On 1934 Phillip Island 100 - Wikipedia click a reference. The drawer opens, but because the document body is still clickable. You can still click the edit button. Hard solution: create a state-machine that manages both Overlays and Drawers, easy solution: prevent the body from being clickable.

4. We loose track of Drawers sometimes

At a more fundamental level that what’s visible on T211691, we still retain references to Drawers that should no longer exist. By adding a console.log to Drawer.onShowDrawer, you can see that that function is being called additively for every time a reference drawer is opened.

Screen Shot 2019-01-09 at 13.37.26.png (988×2 px, 460 KB)

This function responds to an event being emitted, so just to see what happens, I ripped out all the custom events from Drawer:

Screen Shot 2019-01-09 at 15.57.35.png (916×2 px, 408 KB)

(This patch hasn’t been committed or fleshed out yet, but it does manage to also remove the setTimeout in the Drawer code, and should probably be its own ticket).

Change 483803 abandoned by Jdrewniak:
Expose OverlayManager show/hide methods publically

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

Okay! Thanks for all those tasks. I think I've got a handle on what needs to be done here thanks to your excellent research. Please take a look at T214647 at your leisure and we'll hopefully work on its associated tasks soon. Closing this out!

Change 482783 abandoned by Jdlrobson:
Drawer.js DOM inserted onShowDrawer instead of on initialization

Reason:
Larger conversation continuing in https://gerrit.wikimedia.org/r/524314

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