Page MenuHomePhabricator

[EPIC] Re-define the contract for displaying drawers and overlays in MobileFrontend
Closed, ResolvedPublic

Description

We continue to get issues with the Drawers and overlays displaying or not displaying when they are supposed to be. The OverlayManager code is bit rotting and the Drawer code is not centralised. We need to rewrite these 2 essential parts of our infrastructure to make this system easier to work with and less error prone.

Drawers:

We have 2 types of drawers in the mobile site:
ReferenceDrawer and Drawer.x

Currently the creation of a drawer also appends it to the DOM. This has
to stop!

var M = mw.mobileFrontend;
var mobile = M.require( 'mobile.startup' );
var ReferenceDrawer = mobile.ReferencesDrawer;
var CtaDrawer = mobile.CtaDrawer;
var Drawer = mobile.Drawer;
var d = new Drawer()
var d2 = new ReferenceDrawer()

Going forward we will add and remove drawers like so.
This code (which currently does not work)
when run should show all 3 types of drawer, hiding and showing
them with animations 3s apart:

var M = mw.mobileFrontend;
var mobile = M.require( 'mobile.startup' );
var ReferenceDrawer = mobile.ReferencesDrawer;
var CtaDrawer = mobile.CtaDrawer;
var Drawer = mobile.Drawer;
var d = new Drawer()
var d2 = new ReferenceDrawer()
var d3 = new CtaDrawer()
d.appendTo( document.body );
setTimeout( function () {
    d.hide().then( function () {
        d2.appendTo( document.body );
        setTimeout( function () {
            d2.hide().then( function () {
                d3.appendTo( document.body );
            } );
        }, 3000 );
    } );

}, 3000 );
// Make it so that clicking on interface elements hides the drawer
$( 'body.drawer, #ca-edit, #mainmenu' ).one( 'click', function () {
    d3.hide();
});

Specification

  • When a Drawer is created it is not added to the DOM (T214045).
  • Each drawers needs to be explicitly added to the DOM by the creator - A drawer cannot access parent nodes and has no side effects on other elements. To get the existing

effect of making the entire skin opaque while the drawer is open, a drawer will have an associated shield
that covers the entire article (see Drawer pseudo-html and T214049)

  • Each drawer has a hide method which handles the animation of hiding an element. It returns a Deferred object

so that the developer can react to the completion of a hide. (it doesn't return a Deferred but it accepts a onBeforeHide input function)

  • It is possible to open multiple drawers at a time. When multiple drawers are opened,

the last drawer should be on top of the last opened drawer. It is assumed that it is up to the client
to check if multiple drawers are opened. In Minerva we will manage this by emptying and
appending to a specific element in the DOM to assert that only one is open at a time.

  • CSS relating to drawers is in a single file inside mobile.startup/Drawer.less and the CSS file contains

no references to other elements or classes that does not belong to it.
Note: resources/mobile.startup/Drawer.less containers Drawer code.
resources/mobile.startup/references/ReferencesDrawer.less contains ReferenceDrawer specific code.
resources/skins.minerva.base.styles/ui.less contains some MInerva specific code around margins (which is fine)

  • Changes to the Drawers must ensure not to break the existing 3 Drawers. This might be a good opportunity

to create a new generic Drawer and migrating all Drawer's to use this.

  • A drawer should not need to emit any events. It is up to whatever creates the Drawer to wire up its behaviour (T214049)

Drawer pseudo-html

<div class="drawer">
    <div class="drawer__shield"><!-- covers entire screen and is opaque --></div>
    <div class="drawer__drawer">
    <!-- this is the current implementation of the drawer -->
    </div>
</div>

It should be possible to open multiple drawers

You can test this acceptance criteria with the following code:

var Drawer = mw.mobileFrontend.require('mobile.startup').Drawer;
var d = new Drawer({onShow:()=>{}, children: [$('<p>').text('drawer 1')]});
d.appendTo(document.body);

var d2 = new Drawer({children: [$('<p>').text('drawer 2')], onShow:()=>{}});
d2.appendTo(document.body);

d.show(); d2.show();

Event Timeline

Jdlrobson triaged this task as Medium priority.Apr 4 2019, 10:13 PM
Jdlrobson renamed this task from [EPIC] Re-define the contract for drawers in MobileFrontend to [EPIC] Re-define the contract for displaying drawers and overlays in MobileFrontend.Jul 31 2019, 8:20 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson raised the priority of this task from Medium to High.Aug 2 2019, 5:03 PM

Change 528595 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] BREAKING: Cleanup relationship between OverlayManager and Overlay

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

Change 558256 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Drawer's no longer touch the body tag

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

Change 528595 abandoned by Jdlrobson:
BREAKING: Cleanup relationship between OverlayManager and Overlay

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

Change 558256 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Drawer's no longer touch the body tag and have their own mask

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

Change 563266 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Drawers are pure

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

Change 563266 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Drawers are pure

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

Thanks to all the work we've been doing on drawers we can resolve an epic! yipee! Just one acceptance criteria to check before signing off!

Jdlrobson updated the task description. (Show Details)
Jdlrobson added a subscriber: ovasileva.

One regression in T242784 ( cc @ovasileva ) but this epic has met its goals