Page MenuHomePhabricator

MobileFormatter should relocate first paragraph ahead of infobox
Closed, ResolvedPublic8 Estimated Story Points

Description

Story
As a reader, I want the ability to get an overview of an article as soon as I land on the page, so I do not have to scroll very far to know what the article is about

Background
Per outcome of T143803:

MobileFormatter should be updated so that we move the first paragraph above the infobox.

As discussed, be careful not to relocate the paragraph above the hatnote.

  • If a paragraph already appears before the infobox do not do anything.
  • If there is no paragraph before the infobox move the first paragraph that appears after the infobox ahead of the infobox, but after any other elements (e.g. hatnote div) that may be before the infobox.
  • If a lead section has an infobox and no paragraphs do nothing.
  • Controlled via feature flag for beta or stable
  • Enabled in beta.
  • When the config flag is enabled it should have no impact on the API. It should only apply to the mobile skin to avoid any breakages to extensions using MobileView.

QA plan

  1. Opt into the beta mode of the mobile site
  2. Visit https://en.m.wikipedia.beta.wmflabs.org/wiki/Barack_Obama on a mobile browser.
  3. The infobox should appear under a single paragraph of black text
  4. Gray text (hatnote) should appear above the first paragraph of text.
  5. Opt into the stable mode of the mobile site. The infobox should appear underneath the hatnote (gray text) but above the black text.
  6. Repeat the above steps for 3 random pages in https://en.m.wikipedia.beta.wmflabs.org/wiki/Category:Living_people checking that where there is an infobox in the page the positioning is the same.
  7. Also repeat the above steps for the following pages: https://en.m.wikipedia.beta.wmflabs.org/wiki/Suwon, https://en.m.wikipedia.beta.wmflabs.org/wiki/Coccinellidae, https://en.m.wikipedia.beta.wmflabs.org/wiki/Capitol_View,_Atlanta

[1] Infobox looks like this:

Screen Shot 2016-10-20 at 2.52.55 PM.png (404×390 px, 130 KB)

Event Timeline

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

Is it possible to output empty paragraphs?
If we do we should probably remove those regardless from the output and then move the first paragraph.

@bmansurov I meant can there be an empty paragraph before the first paragraph (this seems to be after the second paragraph. Nevertheless an empty paragraph seems like a bug to me and should be removed before we run the formatter.

In answer to your question though we should add test cases for anything that's possible.

Yes, I've seen cases where the first paragraph was empty too.

(it looks a lot better but there's a few unfinished discussions on PS5)

Change 316982 had a related patch set uploaded (by Bmansurov):
Hygiene: Make it easy to retrieve config variables

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

Change 316982 merged by jenkins-bot:
Hygiene: Make it easy to retrieve config variables

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

Change 315577 merged by jenkins-bot:
Beta: move first paragraph in lead section before infobox(es)

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

How do we want to go about QAing this? I've proposed one test but I'd like to be more thorough. Can we lean on Special:RandomInCategory in some way?

Jdlrobson added a subscriber: Nirzar.

@Nirzar - this article begins with an image not an infobox - https://en.m.wikipedia.beta.wmflabs.org/wiki/Maddie_Ziegler - I assume this is okay?
Moving this to -1 until we can work this out and this is deployed on the beta cluster.

How do we want to go about QAing this? I've proposed one test but I'd like to be more thorough. Can we lean on Special:RandomInCategory in some way?

@Jdlrobson either that or we manually pick a handful of articles from different categories.

@Jdlrobson could you please import the following dump to wmflabs?

Once you're done, can you move this task to the Q/A column? In the meantime, I'll update the testing steps.

@ovasileva, yes, I did. Have you opted in to the beta mode?

Tested on Nexus 9 Android 6.0.1 (Chrome) tablet, Galaxy SIII Android 4.4.2 (Browser) mobile phone, and Galaxy SIII Android 4.4.2 (Chrome) mobile phone.

Observing the beta mode of the mobile site, the (Barack Obama), (Living people), Suwon, and Coccinellidae beta pages appear as expected, but Capitol View Atlanta is missing an infobox, so this is not fixed. Screencap below (for the Nexus 9 only, but results were the same across all devices and browsers):

T145216 Nexus 9 Android 6.0.1 (Chrome).png (1×2 px, 395 KB)

@bmansurov - ok, this is very strange. I've tried this on a few browsers and I don't see the infobox after the paragraph anywhere. I must be doing something wrong, but I have no idea what it might be. I cleared my cache and tried it on a different machine just in case as well and nothing. I'm very confused...

@Nicholas.tsg thanks. I think Capitol View Atlanta doesn't have an infobox, so it's working as expected. Sorry for the confusing instructions.

@ovasileva try https://en.m.wikipedia.beta.wmflabs.org/wiki/Coccinellidae?mobileaction=beta on a small screen?

The issue was with @ovasileva's network as we couldn't switch to the beta mode on wmflabs. Using VPN we were able to switch to beta and see the change.

@bmansurov can you please document what you saw on T148975 since it is likely to be helpful for us to debug that bug and replicate the issue. Thank you.

This is causing errors in production which should be fixed. I've thus reopened (see sub task). We should revert or swat a fix for this as soon as we can.

I suggest we don't revert as the change is in beta only. We could disable the feature in beta too until we fix the issues.

I suggest we don't revert as the change is in beta only. We could disable the feature in beta too until we fix the issues.

The important thing is to clean up the error logs for other teams. We have a responsibility to swat something whether a config change, revert or a fix. What do we want to do?

[on basis we don't resolve until subtasks are resolved, please move back to -1 if you think that makes more sense]

is it causing unrelated errors? At any rate, we should resolve the subtasks and reenable (if we do end up disabling)

Change 318978 had a related patch set uploaded (by Bmansurov):
MF Beta: Don't move first paragraph before infobox

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

Change 318978 merged by jenkins-bot:
MF Beta: Don't move first paragraph before infobox

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

Mentioned in SAL (#wikimedia-operations) [2016-10-31T23:46:51Z] <thcipriani@tin> Synchronized wmf-config/InitialiseSettings.php: SWAT: [[gerrit:318978|MF Beta: Do not move first paragraph before infobox (T145216) (T149389)]] (duration: 00m 46s)

Change 319319 had a related patch set uploaded (by Bmansurov):
MF Beta: Enable moving first paragraph before infobox

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

Change 319319 merged by jenkins-bot:
MF Beta: Enable moving first paragraph before infobox

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

Mentioned in SAL (#wikimedia-operations) [2016-11-02T14:13:04Z] <hashar@tin> Synchronized wmf-config/InitialiseSettings.php: MF Beta: Enable moving first paragraph before infobox - T145216 (duration: 00m 47s)

The subtasks have been fixed and the feature has been re-enabled. It was a mistake, I'll disable the feature until the subtasks are in production servers next week.

Change 319326 had a related patch set uploaded (by Bmansurov):
MF Beta: Don't move first paragraph before infobox

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

@bmansurov: Would you prefer to extract a "re-re-enable the feature" task and resolve this one?

Change 319326 merged by jenkins-bot:
MF Beta: Don't move first paragraph before infobox

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

Mentioned in SAL (#wikimedia-operations) [2016-11-02T18:56:30Z] <thcipriani@tin> Synchronized wmf-config/InitialiseSettings.php: SWAT: [[gerrit:319326|MF Beta: Do not move first paragraph before infobox (T145216)]] (duration: 00m 49s)

Is the MobileFormatter used by action=mobileview? If so, the apps may need to remove their move first paragraph logic from the app when this becomes live.

Yes, the MobileFormatter is used by action=mobileview, but this change does not affect apps, nor the API. It's feature flagged and is enabled in mobile web beta only.

I agree it doesn't affect the apps right now. But once this feature becomes enabled by default then it could affect apps. Or will the action=mobileview output stay the same then?

Sorry, I should have been clearer. Yes, it will stay the same even when we enable it in mobile web stable.

Great. Then that wouldn't affect apps then. :)

Is the MobileFormatter used by action=mobileview? If so, the apps may need to remove their move first paragraph logic from the app when this becomes live.

Great. Then that wouldn't affect apps then. :)

We should be aiming for web and apps to use the same API. I think that we all agree that it should be MCS (right?).

Your first comment implies that the apps have their own first paragraph logic. Is this the case? If so, then we should have an actionable plan to reduce the number of implementations from 2+ to 1 IMO.

@bearND @phuedx ideally the MobileFormatter would give an API response identical to MCS and we wouldn't have any need for additional logic in apps to mangle the API output. Epic: T145219

There's a task. Awesome! I believe we're scheduled to talk about that epic during the All Hands week too, right?

Do you have some feedback on this? I don't think this was the brightest idea. The introduction of the article is usually made of compact text (consisting of 2 to 5 paragraphs). You are breaking this text with a long "table". As a reader, I would not be overly impressed about such a great interruption. You should either move full introduction before infobox or move nothing at all.

@Vachovec1 can you provide some example URLs to demonstrate where you feel this is suboptimal? Thank you!