Page MenuHomePhabricator

Thumbnails inside tables, galleries and wrapping containers are not moved triggering log warnings
Closed, ResolvedPublic

Description

In T258201 we enabled the lead paragraph transform to apply to non-infobox elements. However this broken the debug logging that was in place for https://logstash.wikimedia.org/goto/04520c8fceeb291e778f6cfcf4daf6dc

Usually we see < 50 errors an hour, but since this change we've been seeing > 5000.

The debug logging/transformation logic should be adapted to handle such elements.

The error occurs when the lead thumbnail is inside an unexpected container, eg. a div,table, or gallery.

Expected

Logic in https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/master/includes/Transforms/MoveLeadParagraphTransform.php#L190 should only apply to elements with the infobox class.

QA Results - Prod

ACStatusDetails
1T261993#6455677

Event Timeline

Jdlrobson renamed this task from Found infobox wrapped with container to "Found infobox wrapped with container" is applying to non-infobox elements adding .Sep 3 2020, 9:22 PM
Jdlrobson renamed this task from "Found infobox wrapped with container" is applying to non-infobox elements adding to "Found infobox wrapped with container" is applying to non-infobox elements making debugging useless.
Jdlrobson added a project: MobileFrontend.
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a subscriber: Esanders.

The debugging chore is pretty useless while this is true. There is no user impact but this does interfere with our ability to catch major changes in infobox templates which was the original intention of the debugging message.

The logging was implemented here: T149884. Do we still need this?

Change 624272 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/extensions/MobileFrontend@master] Check again for infobox class before logging

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

On second look, maybe this logging is helpful?
It appears to be telling us where the lead transform is not applying.

In https://en.m.wikipedia.org/wiki/Glycolysis for example the image is being wrapped in a table. Adding infobox to that table will allow it to be moved.

On https://en.m.wikipedia.org/wiki/List_of_countries_by_received_FDI the image is inside a div.center

On https://en.m.wikipedia.org/wiki/Timeline_of_punk_rock it's inside a gallery.

On https://en.wikipedia.org/wiki/Andromeda%E2%80%93Milky_Way_collision it is inside an element with noresize

@Esanders Do we want to fix the code so that if an image is contained inside any element that itself is not an infobox the parent is moved?

Jdlrobson renamed this task from "Found infobox wrapped with container" is applying to non-infobox elements making debugging useless to Thumbnails inside tables, galleries and wrapping containers are not moved triggering log warnings.Sep 3 2020, 9:59 PM
Jdlrobson updated the task description. (Show Details)

Change 624829 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/extensions/MobileFrontend@master] MoveLeadParagraphTransform match any image container

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

As I said on T258201#6330080, I think we should switch the MCS approach, which just takes the first paragraph and hoists it to the top, regardless of what comes after it (which a small exception for hatnotes).

Change 624272 abandoned by Jdlrobson:
[mediawiki/extensions/MobileFrontend@master] Check again for infobox class before logging

Reason:
no longer needed

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

Change 624829 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] MoveLeadParagraphTransform match any image container

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

@Jdlrobson
Should I re-test the same articles from the previous hoisting ticket? (in addition to those mentioned above in T261993#6436987)

@Mhurd that would be great
In respect to QAing in production you can skip that and assign it to me - and I'll use the debug logs to determine if this change was successful.

@Jdlrobson
Would cloning the pages you mentioned in T261993#6434739 to my user namespace on beta be a reasonable way to test those pages?
i.e. https://en.m.wikipedia.org/wiki/Glycolysis > https://en.m.wikipedia.beta.wmflabs.org/wiki/User:Montehurd/test/T261993/Glycolysis
( looks like beta *should* have the change, but maybe something about the cloned page being in my user namespace might be interfering? 🤔 )

@Jdlrobson
Would cloning the pages you mentioned in T261993#6434739 to my user namespace on beta be a reasonable way to test those pages?
i.e. https://en.m.wikipedia.org/wiki/Glycolysis > https://en.m.wikipedia.beta.wmflabs.org/wiki/User:Montehurd/test/T261993/Glycolysis
( looks like beta *should* have the change, but maybe something about the cloned page being in my user namespace might be interfering? 🤔 )

@Mhurd - I think it's working as expected:

@ovasileva I may be confused 🤔 - wouldn't it need to pull some text above the image as seen in T261993#6436987?

@Mhurd - my bad! I think my brain was seeing what was not there :/

The issue with that article is it has a table and an image. This edge case hasn't been fixed, but we are back to a manageable 25 log warnings a day thankfully so we can call this done provided there have been no regressions to the original test cases.

Test Result - Prod

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

QA steps
  • Recheck articles from the description hoisting ticket T258201.
  • Confirm text still hoisty

Looks good now! Resolving.