Page MenuHomePhabricator

Make mobilefrontend talk overlay disableable
Closed, ResolvedPublic

Description

MobileFrontend has a special talk page overlay. It reformats the basic talk page into collapsible sections and provides some new-section functionality (but no replying to individual comments).

We need to add a way to disable this functionality, as it's going to interfere with the more-featureful DiscussionTools replying and section-adding. Maybe a hook so that arbitrary other extensions can register a desire to override this behavior?

It (probably?) depends on using a modified version of the talk page markup applied via MobileFormatter, but I'll need to double-check that assumption. Modified markup, in particular, needs to be disabled because it'll conflict heavily with DiscussionTools comment parsing.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Jdlrobson subscribed.

The only reformatting going on is via CSS inside Minerva. I think you can disable it via a line https://github.com/wikimedia/mediawiki-skins-MinervaNeue/blob/592e1151b795faa29eba8e8e13b177d2891306ee/includes/Skins/SkinMinerva.php#L822 but you will also have to disable the JavaScript perhaps with an early exit in
https://github.com/wikimedia/mediawiki-skins-MinervaNeue/blob/master/resources/skins.minerva.scripts/talk.js#L5

The HTML changes for talk page come from the MobileFormatter and apply to all pages so presumably this is not a MobileFrontend issue so retagging.

Note disabling the mobileformatter on talk pages is a solved problem and can be done via config change by the poorly named and soon to be renamed config flag https://github.com/wikimedia/mediawiki-extensions-MobileFrontend#wgmfmobileformatternamespaceblacklist

(Although I wouldn't recommend that as it will make long talk pages impossible to navigate on mobile where the collapsed sections act as a table of contents)

Note disabling the mobileformatter on talk pages is a solved problem and can be done via config change by the poorly named and soon to be renamed config flag https://github.com/wikimedia/mediawiki-extensions-MobileFrontend#wgmfmobileformatternamespaceblacklist

The tricky bit there is that we'd want to disable it based on "is this a page where DiscussionTools is enabled?" which is a page-and-user-specific question that can't be boiled down to just a namespace.

It's why I jumped to thinking of a hook for it -- something abortable that could be checked from MobileFrontend's onOutputPageBeforeHTML handler where it's already making a bunch of checks for whether MobileFormatter should be applied.

(Although I wouldn't recommend that as it will make long talk pages impossible to navigate on mobile where the collapsed sections act as a table of contents)

Challenge here is that DiscussionTools is also going to make changes to section markup soon, so we're probably going to start trampling over each other.

Currently we feel we shouldn't turn off MobileFormatter, because we don't have DiscussionTools' cross-platform section collapsing enabled yet. (But once we do it'll obviously conflict horribly with MobileFormatter. See e.g. T280433 over the weekend, which came from us letting some of the section modifications leak early.)

We talked with Parsing about this, and their position is that they'd prefer to turn off MobileFormatter and rely as much as possible on minimally-modified parsoid output, to make the overall parser switch simpler.

Disabling the talk overlay remains desirable.

Allowing the disabling of the talk overlay CSS and JavaScript seems like a straightforward first step here that will help you make progress.

On the subject of the HTML transforms, I personally would like to offload some of MobileFormatter to the parser. I think the parser (or some additional service that sits on top of it) should have capabilities that allow modifications to the HTML prior to rendering. I believe when we migrate to Parsoid we may be able to remove the section collapsing transforms in favor of CSS/JS only solutions given the section tags that come with it.

I am worried that disabling it might lead to a whole host of other problems and would recommend an all-or-nothing approach. If taking the no MobileFormatter approach we'd need to worry about the page unusable on mobile (think lots of scrolling). Section collapsing aside you'd also lose lazy loading images (not a big deal), and certain ambox templates which are not mobile optimized would appear.

As I understand it, with Parsoid we'll be able to simplify the section collapsing on mobile so that it doesn't require DOM changes. Do you think it's possible we could prioritize getting talk pages using Parsoid for their HTML as part of @ssastry move away from the old wikitext parser?

Perhaps we need to chat?

See T272987 - this won't be resolved right away, but yes, @cscott has successfully advocated for that to be one of the first Parsoid read view use cases.

As for html2html, we've been thinking of html2html transforms more broadly.

Parsoid + HTML2HTML.png (602×1 px, 18 KB)
We need to figure out how much that diagram is mostly a conceptual model for describing how clients get their HTML vs. it being an actual infrastructure layer that enables a number of such html2html to be quickly spun up. This WIP slide deck (sorry WMF internal views only right now) is an early attempt to try to grapple with that.

@ovasileva do you have a sense for when in the future you'd want to work on this? If it's more than a week or so out, we'd probably want to just take it and work on it ourselves...

Change 698904 had a related patch set uploaded (by DLynch; author: DLynch):

[mediawiki/skins/MinervaNeue@master] Add config to skip to the non-overlay version of the talk page

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

Change 699048 had a related patch set uploaded (by DLynch; author: DLynch):

[mediawiki/extensions/MobileFrontend@master] Add a hook that permits disabling MobileFrontend

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

Change 699049 had a related patch set uploaded (by DLynch; author: DLynch):

[mediawiki/extensions/DiscussionTools@master] WIP: Use MobileFrontend hook to disable mobile experience if DT enabled

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

@matmarex to follow up with David re disabling Mobilefrontend

Change 699066 had a related patch set uploaded (by DLynch; author: DLynch):

[mediawiki/extensions/DiscussionTools@master] WIP: Use MinervaNeue hook to disable talk overlay if DT enabled

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

Change 699048 abandoned by DLynch:

[mediawiki/extensions/MobileFrontend@master] Add a hook that permits disabling MobileFrontend

Reason:

Not needed with Ed's patch

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

Change 699049 abandoned by DLynch:

[mediawiki/extensions/DiscussionTools@master] WIP: Use MobileFrontend hook to disable mobile experience if DT enabled

Reason:

Not needed with Ed's patch

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

Ed's patch for mobile compatibility of DT works fine, so I've abandoned the full-disable patches. There remains a patch to MinervaNeue that adds a hook which lets DT disable the overlay as-needed, and a further patch to DT on top of Ed's patch which implements said hook.

Change 698904 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Add hook that allows suppression of the talk page overlay

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

Test wiki created on Patch Demo by Matma Rex using patch(es) linked to this task:

https://patchdemo.wmflabs.org/wikis/0af7013f10/w/

Test wiki created on Patch Demo by Matma Rex using patch(es) linked to this task:

https://patchdemo.wmflabs.org/wikis/0f6285baa3/w/

Context: the remaining patch can't be merged until Peter approves merging the entire enabled-on-mobile patch stack.

Context: the remaining patch can't be merged until Peter approves merging the entire enabled-on-mobile patch stack.

Note: "remaining patch" in this context means the functionality that will enable us to disable MobileFrontend's talk overlay. Although, this patch/functionality is intertwined with the set of patches enable the Reply and New Discussion Tools on mobile. As such, this "remaining patch" cannot be merged until those tools are made available which will happen in (T282638).

Change 699066 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@master] Use MinervaNeue hook to disable talk overlay if DT mobile enabled

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

QA note: This can be tested on https://en.wikipedia.beta.wmflabs.org/, but won't be available on production wikis until we enable the mobile interface there (T270536#7483693).

@ppelberg note the changes here break some of the instrumentation we'll adding to capture bounce rate for talk pages, in preparation for next Monday when the English Wikipedia community enable the talk page tab on mobile.

Adding this note as a placeholder to make sure we don't deploy this to production until we've made sure the instrumentation is ready for that transition.
cc @ovasileva

@ppelberg note the changes here break some of the instrumentation we'll adding to capture bounce rate for talk pages, in preparation for next Monday when the English Wikipedia community enable the talk page tab on mobile.

Adding this note as a placeholder to make sure we don't deploy this to production until we've made sure the instrumentation is ready for that transition.
cc @ovasileva

@Jdlrobson - just to double check, are any of these changes on the next train and if so, is it possible to remove them for now? Also, in terms of getting the instrumentation ready - are we tracking what we would need for the transition under T294738: Define and instrument bounce rate on talk pages or do we need another task?

@ppelberg @ovasileva I have created T295490 and made it a blocker for the deployment task (T270536).

@ppelberg @ovasileva I have created T295490 and made it a blocker for the deployment task (T270536).

Got it, thank you! @ppelberg - does that sound ok from your side?

Adding this note as a placeholder to make sure we don't deploy this to production until we've made sure the instrumentation is ready for that transition.
cc @ovasileva

It won't be deployed to production, the change is controlled by the config option wgDiscussionToolsEnableMobile, which is disabled in production: https://github.com/wikimedia/operations-mediawiki-config/blob/4b7ccc70a08cf2e4cdc8e2c84147e779657ce9a3/wmf-config/InitialiseSettings.php#L19872

Change 738012 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/DiscussionTools@master] Split DiscussionToolsEnableMobile=true into 'behind-overlay' and 'remove-overlay'

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

Change 738012 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@master] Split DiscussionToolsEnableMobile=true into 'behind-overlay' and 'remove-overlay'

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

That change undoes most of the changes for the beta cluster as well, so you can test the enwiki config changes there.

@ppelberg @ovasileva I have created T295490 and made it a blocker for the deployment task (T270536).

Got it, thank you! @ppelberg - does that sound ok from your side?

@ovasileva: blocking the removal of the MobileFrontend talk page overlay (T280417) on instrumentation being added to the "Read as wiki page" view (T295490) sounds good to me.

Note: I've made T295490 a blocker of T280417 considering the current is to offer the Reply and New Discussion Tools on the mobile "Read as wiki page" view while leaving the MobileFrontend overlay "in tact."

matmarex edited projects, added Skipped QA; removed Editing QA.

I think we can close this, and continue on the tasks @ppelberg linked (as well as T295816 about the mess I've made on the beta cluster).

Test wiki on Patch demo by Matma Rex using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/0af7013f10/w/

Test wiki on Patch demo by Matma Rex using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/0f6285baa3/w/