Page MenuHomePhabricator

MoveLeadParagraphTransformInfobox re-order should apply other floated elements, e.g. images
Closed, ResolvedPublic3 Estimated Story Points

Description

Pages which start with floated elements (i.e. elements which aren't supposed to be fully above the first paragraph) should show the paragraph first on mobile.

e.g. https://en.wikipedia.org/wiki/Enriched_uranium

Currently we only look for table.infobox & div.infobox (then if they have a parent of .mw-stack or .collapsible)

Proposal

Currently we treat "infoboxes" as any element with the class infobox. With this change, infobox would be expanded to anything which contains a thumb class.

// Thumbnail images: .thumb, figure (Parsoid)
'.//*[contains(concat(" ",normalize-space(@class)," ")," thumb ")]',
'.//figure',

QA steps

Using mobile site:

QA Results - Beta

ACStatusDetails
1T258201#6411025
2T258201#6411028

QA Results - Prod

ACStatusDetails
1T258201#6420493
2T258201#6420492

Event Timeline

Esanders created this task.Jul 16 2020, 4:36 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 16 2020, 4:36 PM
Esanders updated the task description. (Show Details)Jul 16 2020, 4:51 PM

T258011 should be merged into this task. The wiki is using the wikibase-based generic databox template which should also be classified as needing the move.

Esanders added a comment.EditedJul 17 2020, 11:02 AM

I think that's a different issue. The template on that wiki has no identifying classes. Although the title here is "floated elements", the transformation takes places in HTML without the CSS, so it won't be based on computed CSS.

Change 613700 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/extensions/MobileFrontend@master] MoveLeadParagraphTransform: Hoist thumbnails

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

ovasileva triaged this task as Medium priority.Jul 21 2020, 3:22 PM

This change looks good but does seem a bit risky and could possibly have implications, so I've moved to upcoming to make sure the team chats through the implications of such a change.

cc @alexhollender @ovasileva

Jdlrobson updated the task description. (Show Details)Jul 23 2020, 3:43 PM
Esanders added a comment.EditedJul 23 2020, 3:57 PM

FYI both the mobile apps, which also implement lede paragraph reordering, apply the transform to images:

iOSAndroid

Edit: It looks like MCS doesn't look for infobox/thumb, but moves the paragraph up past anything that isn't a hatnote: https://github.com/wikimedia/mobileapps/blob/e858d09962aa9c0e9a0b04bdbed36c18f973aeb5/lib/mobile/MobileHTML.js

We can move towards that later, but for now moving past thumbnails seems sensible.

Jdlrobson set the point value for this task to 3.

Change 613700 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] MoveLeadParagraphTransform: Hoist thumbnails

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

Jdlrobson updated the task description. (Show Details)
Krinkle removed a subscriber: Krinkle.Aug 19 2020, 9:45 PM
Mhurd added a subscriber: Mhurd.EditedAug 26 2020, 5:51 AM

Test Result - Beta

Status: ✅ PASS
Environment: beta/enwiki
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: NA
Test Artifact(s):

QA steps

✅ AC1: Verify that the image on https://en.m.wikipedia.beta.wmflabs.org/wiki/Enriched_uranium appears below the first paragraph.

Left side of image below shows beta with the fix, right side is production without the fix (note the beta article isn't identical to the prod article, but seems close enough for verifying the fix):

Mhurd added a comment.EditedAug 26 2020, 6:00 AM

Test Result - Beta

Status: ✅ PASS
Environment: beta/enwiki
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: NA
Test Artifact(s):

QA steps

✅ AC2: Exploratory testing. Try to find at least ten pages with infoboxes and images at the top of the page where text does not appear first.

Left sides of images below show beta with the fix, right side is production without the fix (note the beta articles aren't strictly identical to the prod articles, but seem close enough for verifying the fix):

^ I found 8 articles on beta which had images above text to be hoisted. I'm sure there are others but I got tired of hunting for them 😂

Esanders updated the task description. (Show Details)Aug 26 2020, 2:12 PM
Mhurd claimed this task.Aug 26 2020, 10:59 PM
Mhurd added a subscriber: Edtadros.
Mhurd added a comment.EditedAug 29 2020, 4:47 AM

Test Result - Prod

Status: ❓ QUESTION
Environment: enwiki
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: NA
Test Artifact(s):

QA steps

❓ AC2: Exploratory testing. 7 of the 8 articles I tried from T258201#6411028 are now correctly hoisting text on prod. Yay! The remaining article hoists text with ?safemode=1 ( see notes by the "Domestication" screenshots below ), so I think it's ok? @Jdlrobson does that sound ok?

The "Domestication" article failed to hoist text above the image:

However, the "Domestication" article hoisted text nicely with ?safemode=1:

Mhurd added a comment.Aug 29 2020, 4:57 AM

Test Result - Prod

Status: ❓ QUESTION
Environment: enwiki
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: NA
Test Artifact(s):

QA steps

❓ AC1: Verify that the image on https://en.m.wikipedia.org/wiki/Enriched_uranium appears below the first paragraph

It's not working:

However, with ?safemode=1 it is working nicely:

@Jdlrobson Same question as with T258201#6420492 - since it works with safemode is it ok to mark it as PASS?

Mhurd updated the task description. (Show Details)Aug 29 2020, 4:58 AM

Both of the pages display correctly after purging the cached version (using ?action=purge), so that the page is rendered again by the new code.

https://en.m.wikipedia.org/wiki/Enriched_uranium
https://en.m.wikipedia.org/wiki/Domestication

BeforeAfter
ovasileva updated the task description. (Show Details)Aug 31 2020, 8:41 AM
ovasileva closed this task as Resolved.Aug 31 2020, 11:53 AM