Page MenuHomePhabricator

Make the mobile Add Topic button easier for people to access
Closed, ResolvedPublic

Description

This ticket involves the work of revising where and how the Add Topic button appears on mobile talk pages so that: A) people can more easily discover and "reach" the affordance regardless of where they are on the talk page and B) we can create room for the Learn more about this page link T312309 introduces.

Note: we had previously planned to implement the change described above in T312309.

Requirements

Meta

  • Skin(s): Minerva Neue
  • Mobile DiscussionTools State: Enabled

User experience

  • Move the Add topic button that currently appears atop mobile talk pages to be anchored to the bottom of the page with a reveal on scroll interaction. Upon loading, the Add topic button is visible on the screen.
  • The Add topic button should disappear when:
    • People doing anything that causes the keyboard to appear on-screen
    • When the user scrolls down
  • The Add topic button should reappear when:
    • When the user scrolls up – unless they're at the bottom of the screen, in which case the button is already anchored there after the last topic.

Mockup(s)

On page loadWhen scrolling down the pageWhen scrolling up the pageAt the bottom of the page
mobileAddTopic_Load.png (1×1 px, 171 KB)
mobileAddTopic_scrollDown.png (1×1 px, 176 KB)
mobileAddTopic_scrollUp.png (1×1 px, 174 KB)
mobileAddTopic_footer.png (1×1 px, 170 KB)

Done

  • Requirements are drafted and implemented
  • Mockups are posted

Related Objects

Event Timeline

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

is there an additional case when the Add topic button ought to disappear?

I think it should disappear/reappear on scroll, similarly to how many mobile browsers’ chrome (address bar, toolbar etc.) work: when one scrolls down, they disappear; when one scrolls up, they appear again. (Before any scrolling, it should be visible, otherwise it’d be hard to discover.)

@Tacsipacsi Yes that's exactly what converged on :)

nayoub updated the task description. (Show Details)

Change 851649 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/extensions/DiscussionTools@master] Make "Add topic" button sticky

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

Per the conversation the Editing Team had offline today, we decided to make it so the Add topic button appears above the Read as wiki page link once someone scrolls to the bottom of a talk page.

See quick mock up from @Esanders:

image.png (820×414 px, 67 KB)

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

https://patchdemo.wmflabs.org/wikis/d895116719/w/

I think losing no-JS support is an unacceptable regression. It should be possible to implement this by appending HTML to $text in the OutputPageBeforeHTML hook.

No-JS support will be added in T323221.

Meta: the comment quoted above comes from gerrit.

ppelberg added a subscriber: Esanders.

Deployment timing
As discussing during today's team planning meeting, we are planning for this change to be deployed via a backport window tomorrow, 17 Nov.

Change 851649 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@master] Make "Add topic" button sticky

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

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

[mediawiki/extensions/DiscussionTools@wmf/1.40.0-wmf.10] Make "Add topic" button sticky

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

Change 858308 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@wmf/1.40.0-wmf.10] Make "Add topic" button sticky

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

Mentioned in SAL (#wikimedia-operations) [2022-11-17T14:13:49Z] <urbanecm@deploy1002> Started scap: Backport for [[gerrit:858308|Make "Add topic" button sticky (T316175)]], [[gerrit:858309|CommentFormatter: Fix condition for lede button to consider new wrappers (T323171)]], [[gerrit:858310|Remove override for Minerva hiding .tmbox, no longer needed (T257394)]], [[gerrit:858311|CommentFormatter: Fix condition for lede button to consider table of contents (T323241)]], [[gerrit:858312

Mentioned in SAL (#wikimedia-operations) [2022-11-17T14:14:13Z] <urbanecm@deploy1002> urbanecm and matmarex: Backport for [[gerrit:858308|Make "Add topic" button sticky (T316175)]], [[gerrit:858309|CommentFormatter: Fix condition for lede button to consider new wrappers (T323171)]], [[gerrit:858310|Remove override for Minerva hiding .tmbox, no longer needed (T257394)]], [[gerrit:858311|CommentFormatter: Fix condition for lede button to consider table of contents (T323241)]], [[gerr

Mentioned in SAL (#wikimedia-operations) [2022-11-17T14:50:35Z] <urbanecm@deploy1002> Started scap: rSMIN4e4192126b55: f659d88b: 65cd6881: 96e86cf: 5b94aca: 7a06c4b98: DiscussionTools, GlobalUsage, MinervaNeue backports (T316175, T323171, T257394, T323241)

Mentioned in SAL (#wikimedia-operations) [2022-11-17T14:55:04Z] <urbanecm@deploy1002> Finished scap: rSMIN4e4192126b55: f659d88b: 65cd6881: 96e86cf: 5b94aca: 7a06c4b98: DiscussionTools, GlobalUsage, MinervaNeue backports (T316175, T323171, T257394, T323241) (duration: 04m 29s)

Issues found so far:

  1. On fr.wiki the Reply links are not working on a talk page.
  2. The "Add topic" button is not doing anything on fr.wiki. Also, there are two "Add topic" buttons on a talk page.
  3. The following error appears in the console when you load any talk page on fr.wiki.
Uncaught Error: Collection contains more than one element
Ryasmeen changed the task status from Open to In Progress.Nov 18 2022, 2:34 AM

Just to note, the issues are all mobile-only. Everything's fine on desktop.

It's all the same root-cause, which is that error which is triggered on wikis with the talkpageheader message not disabled -- the onOutputPageBeforeHTML hook runs when that message is being output as well as when the main content is rendered. The hook appends the button to the output of each call, and that results in multiple new topic buttons being on the page. Then OO.ui.infuse fails because it's been passed multiple elements. (And the error interrupts the initialization of all the elements.)

Unfortunately, I don't think whether we're parsing an interface message is actually accessible to the hook. (OutputPage sets the option when creating the ParserOutput but then immediately unsets it, and the ParserOutput is never actually sent to the hook, just the text. We could maybe check for mw-talkpageheader in the text before modifying anything?

Locally, adding this to the top of the hook does work:

if ( strpos($text, "class=\"mw-talkpageheader\"") !== false ) {
	return;
}

...but it feels a bit kludgy.

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

[mediawiki/extensions/DiscussionTools@master] Don't run OutputPageBeforeHTML for the talkpageheader

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

Change 858443 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@master] Don't run OutputPageBeforeHTML for the talkpageheader

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

This problem is on me and my suggestion to use the OutputPageBeforeHTML hook to add the button. I thought that it only runs once, when the content of the page is added, but apparently that's not true. This is even documented on Manual:Hooks/OutputPageBeforeHTML. I'm not the only one to make this mistake, but in other cases it is mostly harmless (e.g. MobileFrontend also does this wrong, causing duplicate <meta name="theme-color" content="#eaecf0"/> tags to be added on fr.wp talk pages).

I'll look for a better way to do this, but in the meantime, @DLynch's patch works and I'll try to get it backported.

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

[mediawiki/extensions/DiscussionTools@wmf/1.40.0-wmf.10] Don't run OutputPageBeforeHTML for the talkpageheader

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

Change 858319 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@wmf/1.40.0-wmf.10] Don't run OutputPageBeforeHTML for the talkpageheader

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

Mentioned in SAL (#wikimedia-operations) [2022-11-18T09:52:28Z] <oblivian@deploy1002> Started scap: Backport for [[gerrit:858319|Don't run OutputPageBeforeHTML for the talkpageheader (T316175)]]

Mentioned in SAL (#wikimedia-operations) [2022-11-18T09:52:50Z] <oblivian@deploy1002> oblivian and matmarex: Backport for [[gerrit:858319|Don't run OutputPageBeforeHTML for the talkpageheader (T316175)]] synced to the testservers: mwdebug2002.codfw.wmnet, mwdebug1002.eqiad.wmnet, mwdebug1001.eqiad.wmnet, mwdebug2001.codfw.wmnet

Mentioned in SAL (#wikimedia-operations) [2022-11-18T09:57:58Z] <oblivian@deploy1002> Finished scap: Backport for [[gerrit:858319|Don't run OutputPageBeforeHTML for the talkpageheader (T316175)]] (duration: 05m 29s)

I'm not the only one to make this mistake, but in other cases it is mostly harmless (e.g. MobileFrontend also does this wrong, causing duplicate <meta name="theme-color" content="#eaecf0"/> tags to be added on fr.wp talk pages).

We should file a task for that.

Tested on fr.wiki, all three issues mentioned above are now fixed. Thanks @DLynch!
There is another issue around this button which I filed separately here: T323400.

Tested on fr.wiki, all three issues mentioned above are now fixed. Thanks @DLynch!

Excellent.

There is another issue around this button which I filed separately here: T323400.

Wonderful – thank you, @Ryasmeen.

Resulting question: does you stating the above mean this ticket can now be closed?

Tested on fr.wiki, all three issues mentioned above are now fixed. Thanks @DLynch!

Excellent.

There is another issue around this button which I filed separately here: T323400.

Wonderful – thank you, @Ryasmeen.

Resulting question: does you stating the above mean this ticket can now be closed?

@ppelberg: I was going to wait until T323400 is resolved. Since, that's directly relevant to the change we intended to do around the behavior of this button. I am fine either way though. What do you think?

Tested on fr.wiki, all three issues mentioned above are now fixed. Thanks @DLynch!

Excellent.

There is another issue around this button which I filed separately here: T323400.

Wonderful – thank you, @Ryasmeen.

Resulting question: does you stating the above mean this ticket can now be closed?

@ppelberg: I was going to wait until T323400 is resolved. Since, that's directly relevant to the change we intended to do around the behavior of this button. I am fine either way though. What do you think?

What you described sounds great to me – I'm going to move this to Blocked / Needs more work in the meantime.

Next step

  • Editing QA to retest this ticket once T323400 is resolved

@ppelberg with T323400 being resolved and verified, I think we can close this one and also move ahead with T321961. I will also resume working on T320997 soon in preparation of that.

@ppelberg with T323400 being resolved and verified, I think we can close this one and also move ahead with T321961. I will also resume working on T320997 soon in preparation of that.

Excellent – all that you described above sounds great to me.

Aside: I value how you laid the sequence above out...thank you, @Ryasmeen
🙂.

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

https://patchdemo.wmflabs.org/wikis/4eef56dbb9/w/