Page MenuHomePhabricator

MFA: [Spike, 2hrs] Toast busters: I'm afraid of no toast - should toast be upstream to core?
Closed, ResolvedPublic0 Story Points

Description

The toast code wraps mw.notify and has 0% code coverage.

It provides what looks like useful functionality to show a toast on a page reload:
Toast.prototype.showOnPageReload

The question posed is should this functionality be moved into mw.notify in core and the Toast class deprecated?

This decision impacts how we go about code coverage for this file.

Outcomes

  • The code and how it is used is scrutinised
  • A decision by the team on the future of this code
  • A task is created documenting those next steps (e.g. keeping+improving code coverage / removing / upstreaming to core)

Out of scope

Actually upstreaming this to core (if decided) is out of scope for this task. As part of sign off, setup tasks to pursue the next steps.

Event Timeline

Jdlrobson created this task.Nov 9 2018, 9:23 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 9 2018, 9:23 PM
Jdlrobson renamed this task from [Spike, 2hrs] Toast busters: I'm afraid of no toast - should toast be upstream to core? to MFA: [Spike, 2hrs] Toast busters: I'm afraid of no toast - should toast be upstream to core?.Nov 15 2018, 6:47 PM
Jdlrobson triaged this task as Normal priority.Nov 16 2018, 12:41 AM
Jdlrobson updated the task description. (Show Details)Nov 21 2018, 5:15 PM
Jdlrobson set the point value for this task to 0.

I just discovered what $wgResourceModuleSkinStyles does today. If we upstream the Toast functionality to core, could we use that resourceLoader feature to "skin" mw.notify into the "toast" style for Minerva?

I just discovered what $wgResourceModuleSkinStyles does today. If we upstream the Toast functionality to core, could we use that resourceLoader feature to "skin" mw.notify into the "toast" style for Minerva?

Yup! That's what it's for. We actually do this already. If you use mw.notify right now it will style this way!

The only feature we add to Toast is the ability to delay a toast until after a page reload.

I suspect VE must do something similar?

Per @Jdlrobson's suggestion, I’ve looked into how VE and mw-core load the “post-edit” toast.

On mobile, VE uses the same base class as the source editor, EditorOverlayBase, and so they both use the toast’s showOnPageReload function to render the toast after the page has reloaded.

Desktop gets interesting though. Both the wikitext editor and VE provide a mw.notify notice after a page has been edited. The wikitext editor uses a server-side cookie to facilitate this, whereas VE provides the notice because it doesn’t actually refresh the page, instead it just repopulates it with the new content.

In the wikitext editor scenario, the PHP function setPostEditCookie() sets a temporary cookie wikiPostEditRevision{id}=[saved|created|restored] after a page reloads after an edit. The cookie gets deleted after the page redirects, but its value is stored in a JS var called wgPostEdit in Article.php#859 . This can be inspected in the dev console after making a source edit in Vector and then mw.config.get('wgPostEdit’).

The var is then picked up by mediawiki.action.view.postEdit.js which renders the mw.notification.

If we were to adopt this workflow on mobile, we would have to make substantial changes to our editor code. We would essentially have to save edits via a form instead of via the API, and then check if that JS value exists after page reload.

The VE approach of populating the page content without a refresh seems like a cleaner experience. However, many of the functions in MF are initiated on pageload, like collapsible sections, TOC, lazy-loaded images, page-issues etc. Having these features reinitialize after the page content is updated, without a page reload, also seems like a substantial effort (although I honestly haven’t looked at this too deeply).

Given all this, I think moving the toast function into core might be the simplest solution. Although, from a UX point of view, refreshing the page without a page reload does seem very nice.

Jdlrobson added subscribers: Esanders, matmarex.

@matmarex @Esanders do you have any thoughts/concerns about formalising a way to send notifications after a page reload?

many of the functions in MF are initiated on pageload, like collapsible sections, TOC

This exact problem is why mw.hook() was created. It enables developers to decouple the creation and updating of user interface elements from features that enhance those elements. It does this through its "memory" behaviour (that is, attaching new handlers after an element is last created or updated, will fire retroactively as needed).

Core JS, most extensions, and many gadgets have already adopted mw.hook() for this purpose; in favour of jQuery-style dom-ready handlers. Migration is pretty straight forward:

// Before
$( function () {
  var $my = mw.util.$content.find( '.selector');
  handleThings( $my );
} );
// Before (alt. using hardcoded ID)
$( function () {
  var $my = $( '#mw-content-text .selector');
  handleThings( $my );
} );

// After
mw.hook( 'wikipage.content', function ( $content ) {
  handleThings( $my.find( '.selector' ) );
} );

Using mw.hook ensures enhanced features work as expected during live preview (wikitext editor), post-edit (VisualEditor), as well as in various other ajax-y scenarios where an extension or gadget refreshes, changes, or adds a piece of content.

These hooks currently exist for page content, category list, diff content, and a few others.

The list of hooks also includes postEdit, which might be applicable for the mobile editor to avoid needing a dependency between the code that saves the edit, and the code displaying the notification. In theory it could also mean that core's existing postEdit notif code could be re-used as-is and work automatically (if desired).

Jdlrobson added a comment.EditedDec 20 2018, 4:56 PM

I agree with @Jdrewniak that rewriting the entire HTML is not going to happen (in fact we used to do that but changed after countless bugs relating to the many different settings we have to consider e.g. beta, logged in preferences)

Given we only use this Toast on the editor and Special:MobileOptions maybe the next step is to rename src/mobile.startup/toast.js to showOnPageReload.js and deprecate and remove the Toast class to use mw.notify from now on.

That said, it would be nice to generalise the wgPostEdit and showOnPageReload mechanisms to be something we can reuse from core.

Will leave open a little long in case @Esanders @matmarex or anyone else have any opinions/ideas.

Jdlrobson closed this task as Resolved.Dec 21 2018, 11:17 PM

Let's continue the conversation in T212542.