Page MenuHomePhabricator

toast.js renders a (non-visible) toast on every page load
Closed, ResolvedPublic

Description

The toast module creates a new object of itself while initializing. It takes only some ms (2,7ms), but isn't really needed most of the time. Maybe we should revisit that (I think it's mostyl done to render a pending toast, e.g. after an edit was saved) and separate the pending toast rendering from the toast itself, so it's only rendered, if really neded.

It also creates an empty toast element in the DOM (which is normal while rendering):


Event Timeline

Florian raised the priority of this task from to Needs Triage.
Florian updated the task description. (Show Details)
Florian added a project: MobileFrontend.
Florian added subscribers: Florian, Jdlrobson, Jhernandez.
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Florian set Security to None.

This sounds like a bug we've had before...

I remember we talked about a similar problem in Lyon, maybe you mean that? I'm not sure, if that was the toast, too :/

I think this has always been the case for Drawers
Drawer.js has a line:
$( function () {

				self.appendTo( self.appendToElement );
			} );

Really they shouldn't be doing this as they should not know about jQuery global.
It would be great to make these more explicitly added to the DOM.

But drawers get first initialized and rendered, when they are used, not on page load (like toasts) :)

See:
toast.js:

M.define( 'toast', new Toast() );

Drawer.js:

M.define( 'Drawer', Drawer );

I think the easiest move would be to move showOnPageReload and _showPending out of the toast module to not need to create a new object of toast on pageload. That would need, that we rewrite all usages of toast to create a new object by themself.

Another way would be to wrap the toast itself into a new object, which creates a toast object itself, but only, if needed. Something like:

toast = {
   _getToast: function () {
      if ( this.toast === undefined ) {
         this.toast = new toastOverlay();
      }
      return this.toast;
   }
   show: function ( msg, className ) {
      this._getToast().show( msg, className );
   }
   // ... and the save and show pending toast functions
}

Where ToastOverlay is the actual class, which we call "toast", which extends Drawer.js. As the toast module, we could set M.define( 'toast', new Toast() ); without any problems, because it's a simple object now with some wrapper functions and two functions to save and show a pending toast.

Quick note: showOnPageReload and friends are used in MF and also in Gather.
Were we to change them be sure we're not breaking other extensions.

Change 230112 had a related patch set uploaded (by Florianschmidtwelzow):
Don't render a toast on page load

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

I actually think Drawers adding themselves to the Dom is broken. It's inconsistent with all our other views which have to manually be added.

At very least appending to Dom should happen on the first call to show. I should be able to initialise a drawer without putting it in the Dom.

Jhernandez removed a project: Readers-Web-Backlog.
Jhernandez moved this task from Backlog to team:other on the MobileFrontend board.

@Jhernandez @phuedx I would appreciate your thoughts on this subject. To me this seems an unnecessary micro optimisation.

Right now, I'm less interested in shaving off 2.7 ms than I am in having a couple of views that don't behave like all of the others. I also don't like constructors with side effects, so this is doubly irksome.

Jdlrobson changed the task status from Open to Stalled.Sep 23 2015, 7:33 PM

I agree that the synchronous and unconditional creation of DOM structures and various other DOM methods used in the process does seem something worth optimising away. I think in practice, it'll be more than 2.7ms. It's probably not including all the side-effects from this code path, and presumably it was measured on a desktop machine as well.

Change 230112 merged by jenkins-bot:
Don't render a toast on page load

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

Thanks for the input @Krinkle and @phuedx, and thanks for your patience @Florian and ultimately good solution.