Page MenuHomePhabricator

[Spike 4hr]: Explore methods to move first paragraph above infobox
Closed, ResolvedPublic

Description

We would like to explore several options for moving infobox below the first paragraph

  • Can we do this via CSS e.g. flex box?
  • Could we do this via MobileFormatter by rearranging content?
  • Could we do this via MobileFormatter by pulling out infobox html and making it a property of the page that can be positioned by the skin.

Create prototoypes for both scenarios where possible - preferably on live data but static pages are okay (feel free to use loot-ui or weekipedia ).

Event Timeline

MBinder_WMF renamed this task from Spike: Explore methods to move first paragraph above infobox to [Spike 4hrs]: Explore methods to move first paragraph above infobox.Aug 29 2016, 4:28 PM
Jdlrobson renamed this task from [Spike 4hrs]: Explore methods to move first paragraph above infobox to Spike: Explore methods to move first paragraph above infobox.Aug 29 2016, 4:28 PM
Jdlrobson updated the task description. (Show Details)
jhobs updated the task description. (Show Details)Aug 29 2016, 4:29 PM
MBinder_WMF renamed this task from Spike: Explore methods to move first paragraph above infobox to [Spike 4hr]: Explore methods to move first paragraph above infobox.Aug 29 2016, 4:29 PM
Jhernandez removed Jhernandez as the assignee of this task.Sep 1 2016, 10:40 AM
Jhernandez moved this task from Doing to To Do on the Reading-Web-Sprint-80-V-for-Vandalism board.
Jhernandez added a subscriber: Jhernandez.

Not working on it yet, back to todo in case something wants it

jhobs claimed this task.
bmansurov claimed this task.Sep 7 2016, 5:38 PM
bmansurov moved this task from To Do to Doing on the Reading-Web-Sprint-80-V-for-Vandalism board.
bmansurov added a subscriber: jhobs.

Change 309117 had a related patch set uploaded (by Bmansurov):
Show the lead section first paragraph before infoboxes

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

Change 309202 had a related patch set uploaded (by Bmansurov):
WIP: Show the lead section first paragraph before infoboxes

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

It's worth pointing out hatnotes may appear below the first paragraph with this flex box based approach:
Consider https://trending.wmflabs.org/en/wiki/Sunderland%20A.F.C. where the hatnote appears underneath the lead section (ignore the fact it appears twice - I've been playing with a fix for T143739 in the Mobile content service separately)

There are also various other edge cases that the Android app inside mobile content service is dealing with.

I thus explored this from a slightly different angle.
On the long term I'd like to see MobileFormatter and Mobile Content service converge - ie. do the same things and make it possible for us to use MCS in MobileFrontend.

Given we rely on MobileFormatter for lazy loading images and a host of other things how do people feel about exploring using MobileFormatter to make the lead paragraph, hatnotes and infobox first class citizens? (i.e. mimic the mobile content service approach I proposed here - T145034).

There are probably performance gains to be had here - I notice the API and skin use MobileFormatter in two different places which don't share the same caching and in addition to this it would allow us to control the positioning of the infobox which may be highly useful in tablet mode...

I'm okay with the flex box approach if it's a short term solution, but I think this is a good opportunity to make life easier for ourselves going forward.

Here are the spike results using CSS and MobileFormatter. The MobileFormatter approach is more robust as we only move the first paragraph above the first infobox. The CSS way of doing things makes assumptions about ordering of elements, thus may not be the best solution.

I think @bmansurov's suggestion of using the MobileFormatter makes the most sense. I'm a little concerned about edge cases given the issues I've seen with the implementation in the mobile content service. I think on the long term though we should aim to make the infobox a first class citizen per https://phabricator.wikimedia.org/T143803#2618044

People should chip in with thoughts and a task should be created before this is resolved.

Jdlrobson reassigned this task from bmansurov to phuedx.Sep 8 2016, 5:55 PM
Jdlrobson added subscribers: phuedx, bmansurov.

thoughts @phuedx ?

@Dbrant @Mholloway @Niedzielski @bearND @JMinor @Fjalapeno @Mhurd @JoeWalsh adding you here for visibility. If action=mobileview output format changes, we'd need to test. This is just a spike. Do people have the server/client transforms handy from the apps that do this sort of thing today in the apps?

(n.b., I see Jon's link to the JS file for MCS.)

Adam: We didn't impact apps with the lazy loading images work so I don't expect any issues here. Api requests are currently handled separately.

I've probably missed something crucial here. If I understand correctly, we're talking about mobileview API testing in general:

  • Response properties: e.g., query in a MW query response
  • HTML: e.g., the first lead section

In my opinion, property testing is best performed in the Content Service or MobileFrontend itself. It's easy to examine expected fields in a response.

How the HTML in a response will look is tricky. You can run the Android screenshot tests but these haven't worked as well as hoped as they're integration level (slow and flaky) and don't currently have support for diffing. I think we've focused more lately on moving transforms out of the WebView instead and trusted that it will behave similar to the stock browser. Our loading logic will need a lot more refactoring before we can make these tests effective.

The new Android screenshot tests won't be useful to this end as they are very unit-y, would never make a network request, and only consume completely formed models. These tests are useful for detecting regressions against all supported configurations (locales, strings, images, and other variable inputs) but the data is mocked for each.

The new data client tests use MockWebServer with a mock response but maybe we can make a script to update these periodically. However, most of these data clients only make a dumb network request and provide a fully hydrated model with little manual parsing. They're unlikely to catch bugs affecting presentation.

BTW, AFAIK the Android app still has some native and JS transforms for both Content Service and MediaWiki API responses.

@Niedzielski we will sync with you when we actually plan to make any change. Right now we'll still focused on deciding whether to do this or not so any discussion may be premature. If we do make the change we can easily avoid changes to the mobileview api. We may also want to prioritise work on T139003. Apologies for the confusion.

(@phuedx in case you got lost by the comments my original comment/question is here - https://phabricator.wikimedia.org/T143803#2619937 - please move to sign off if you are happy with the suggested idea for an approach)

phuedx added a comment.Sep 9 2016, 8:22 AM

The new data client tests use MockWebServer with a mock response but maybe we can make a script to update these periodically. However, most of these data clients only make a dumb network request and provide a fully hydrated model with little manual parsing. They're unlikely to catch bugs affecting presentation.

It's our (Reading Web's) job to ensure that any changes that we make to the API are tested, which includes your data client tests.

phuedx added a comment.Sep 9 2016, 8:33 AM

I think @bmansurov's suggestion of using the MobileFormatter makes the most sense. I'm a little concerned about edge cases given the issues I've seen with the implementation in the mobile content service. I think on the long term though we should aim to make the infobox a first class citizen per https://phabricator.wikimedia.org/T143803#2618044

People should chip in with thoughts and a task should be created before this is resolved.

I'm happy that we even explored a CSS-based solution as part of this task.

Generally speaking, any work that we can do server-side, and cache (and edge-cache) should be done server-side. PHP, despite it's failings, is far more flexible than CSS.


There are also various other edge cases that the Android app inside mobile content service is dealing with.

Our first task is to create a diverse corpus of pages to test our pages against.

@bearND, @Niedzielski, @Mholloway: Do you have one of these? Would you like one of these?

phuedx added a subscriber: ovasileva.EditedSep 9 2016, 8:41 AM

Per T143803#2622476, I'm happy with the proposed solution – the most flexible, easy to (unit) test one,

@dr0ptp4kt: There's coordination required around when and how to signal making changes to MobileFormatter and mobileview API. AFAICT there's also an opportunity to create a shared test suite for the API, the value of which will have to be demonstrated to our POs /cc @ovasileva


@Jdlrobson: Let's create a separate task about your concerns about the lack of a shared cache (?) between ExtMobileFrontend::DOMParse and the mobileview API.

Change 309202 abandoned by Bmansurov:
Beta: Show the lead section first paragraph before infoboxes

Reason:
Resurrect if needed. Done for now.

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

Change 309117 abandoned by Bmansurov:
Show the lead section first paragraph before infoboxes

Reason:
Resurrect if needed. Done for now.

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

@dr0ptp4kt: There's coordination required around when and how to signal making changes to MobileFormatter and mobileview API. AFAICT there's also an opportunity to create a shared test suite for the API, the value of which will have to be demonstrated to our POs /cc @ovasileva

Definitely! As a start, do you think there would be a quick and handy way to watch certain paths in https://www.mediawiki.org/wiki/Git/Reviewers#mediawiki.2Fextensions.2FMobileFrontend and friends? I guess there's PageImages, RelatedArticles, TextExtracts, as well - I don't know if the simplest solution for those is for people to auto add for any change in those repos, given that they're very much API.php oriented (contrast this with MobileFrontend, where all kinds of other changes have little impact on api.php).

Jdlrobson closed this task as Resolved.Sep 9 2016, 5:40 PM

The best most future proof solution to this is to [[ T139003 | write tests ]] and run them as part of MobileFrontend which we now do as of this week.

I've created T145216 to capture the outcome of this spike and T145219 to capture my MobileFormatter concerns and a more long term way of using it.

I consider this spike closed.