[Bug] Table of contents causes reflow
Closed, ResolvedPublic2 Story Points

Description

Steps to Reproduce

Visit https://en.m.wikipedia.org/wiki/Astragalus_pomonensis
Note that the appearance of the table of contents pushes down the remaining content.
In comparison, https://en.m.wikipedia.org/wiki/Inflorescence works fine.

MobileFormatter is supposed to add a placeholder for table of contents (MFTOC) via the AddMobileTocTransform - to reserve space for table of contents.

The logic seems to be linked to whether desktop shows a table of content or not. In the first example, no table of contents is needed since the page is so simple. In the latter, it is needed.

Expected Results

  • The table of contents placeholder should be present in HTML for all mobile pages where the table of contents is rendered via JavaScript

Actual Results

  • The placeholder is not present so when the table of contents is inserted it pushes all content down

Environments Observed

Testing Environment for QA

This should be tested on the beta cluster

Browser Version

OS Version

Device Model

Device Language

There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 1 2018, 6:18 PM
Jdlrobson updated the task description. (Show Details)May 29 2018, 4:15 PM
Jdlrobson set the point value for this task to 2.May 29 2018, 4:17 PM

@Jdlrobson, can you provide an example article? Excuse the shifty lower quality screenshots but I'm not seeing a reflow occur on the Dog article:

Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)Jun 11 2018, 9:02 PM

Change 439773 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Only render table of contents on pages with a table of contents

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

Change 439777 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Simplify how table of contents is stripped from mobile view

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

Oh apparently this needs more work. I missed the gerrit review.

I've reviewed and tested Jon's changes and they look good but would feel more comfortable if they got a second pair of eyes. Can you take a look?

pmiazga reassigned this task from pmiazga to ABorbaWMF.
pmiazga removed a project: Patch-For-Review.
pmiazga added a subscriber: pmiazga.

Change 439777 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Simplify how table of contents is stripped from mobile view

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

Jdlrobson removed ABorbaWMF as the assignee of this task.Jun 19 2018, 5:09 PM
Jdlrobson added a subscriber: ABorbaWMF.

Hang on, this needs QA steps for Anthony.

Just wondering if we have screenshots of the original issue. Looking now I am having trouble identifying the issue visually. Is it that the TOC appears at all in the first article?

Jdlrobson updated the task description. (Show Details)Jun 19 2018, 6:17 PM

This still needs code review. https://gerrit.wikimedia.org/r/439773 is still open.
@ABorbaWMF sorry for wasting your time. I've also updated the testing criteria to make testing easier next time round.

Change 439773 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Only render table of contents on pages with a table of contents

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

Checked https://en.m.wikipedia.beta.wmflabs.org/wiki/Ronald_Reagan and it looks like the space is properly being reserved so that when the toc loads there is no content reflow/shift.

Looks good to me on Beta with both articles. Nothing seems pushed down. Tried a handful of devices/browsers. Ready for signoff.

ovasileva closed this task as Resolved.Jun 28 2018, 10:34 AM
Jdlrobson reopened this task as Open.EditedJun 29 2018, 12:29 AM
Jdlrobson removed ABorbaWMF as the assignee of this task.

I'm seeing some PHP warnings looks related to this change...

/srv/mediawiki/php-1.32.0-wmf.10/extensions/MobileFrontend/includes/MobileFormatter.php:520
t  exception.message	       	PHP Warning: Invalid State Error
t  exception.trace	       	#0 /srv/mediawiki/php-1.32.0-wmf.10/extensions/MobileFrontend/includes/MobileFormatter.php(520): MWExceptionHandler::handleError(integer, string, string, integer, array, array)
#1 /srv/mediawiki/php-1.32.0-wmf.10/extensions/MobileFrontend/includes/MobileFormatter.php(199): MobileFormatter->makeSections(DOMDocument, array, array)
#2 /srv/mediawiki/php-1.32.0-wmf.10/extensions/MobileFrontend/includes/MobileFrontend.body.php(65): MobileFormatter->filterContent(boolean, boolean, boolean, boolean)
#3 /srv/mediawiki/php-1.32.0-wmf.10/extensions/MobileFrontend/includes/MobileFrontend.hooks.php(231): ExtMobileFrontend::domParse(OutputPage, string, boolean)
#4 /srv/mediawiki/php-1.32.0-wmf.10/includes/Hooks.php(174): MobileFrontendHooks::onOutputPageBeforeHTML(OutputPage, string)
#5 /srv/mediawiki/php-1.32.0-wmf.10/includes/Hooks.php(234): Hooks::callHook(string, array, array, NULL, string)
#6 /srv/mediawiki/php-1.32.0-wmf.10/includes/OutputPage.php(1899): Hooks::runWithoutAbort(string, array)
#7 /srv/mediawiki/php-1.32.0-wmf.10/includes/OutputPage.php(1911): OutputPage->addParserOutputText(ParserOutput, array)
#8 /srv/mediawiki/php-1.32.0-wmf.10/includes/page/Article.php(546): OutputPage->addParserOutput(ParserOutput, array)
#9 /srv/mediawiki/php-1.32.0-wmf.10/includes/actions/ViewAction.php(68): Article->view()
#10 /srv/mediawiki/php-1.32.0-wmf.10/includes/MediaWiki.php(500): ViewAction->show()
#11 /srv/mediawiki/php-1.32.0-wmf.10/includes/MediaWiki.php(294): MediaWiki->performAction(Article, Title)
#12 /srv/mediawiki/php-1.32.0-wmf.10/includes/MediaWiki.php(867): MediaWiki->performRequest()
#13 /srv/mediawiki/php-1.32.0-wmf.10/includes/MediaWiki.php(524): MediaWiki->main()
#14 /srv/mediawiki/php-1.32.0-wmf.10/index.php(42): MediaWiki->run()
#15 /srv/mediawiki/w/index.php(3): include(string)
#16 {main}

Can replicate on http://localhost:8888/w/index.php?title=1240%D8%A1_%DA%A9%DB%8C_%D8%AF%DB%81%D8%A7%D8%A6%DB%8C

Change 443136 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Headings can now be removed

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

Vvjjkkii renamed this task from [Bug] Table of contents causes reflow to uvdaaaaaaa.Jul 1 2018, 1:12 AM
Vvjjkkii raised the priority of this task from Normal to High.
Vvjjkkii removed the point value for this task.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed subscribers: gerritbot, Aklapper.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot set the point value for this task to 2.
CommunityTechBot lowered the priority of this task from High to Normal.
CommunityTechBot renamed this task from uvdaaaaaaa to [Bug] Table of contents causes reflow.
CommunityTechBot added subscribers: gerritbot, Aklapper.

@Jdlrobson - cold you add QA steps and move it to Needs QA?

Change 443136 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Headings can now be removed

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

Jdlrobson assigned this task to alexhollender.

@alexhollender let's run through the original QA steps, just in case this follow up broke something.

Looks good on Safari desktop. https://en.m.wikipedia.beta.wmflabs.org/wiki/Ronald_Reagan is not loading in Chrome. I do not have an iPad to test with. Over to QA.

Looks good here as well.

ovasileva closed this task as Resolved.Jul 6 2018, 11:52 AM
ovasileva added a subscriber: ovasileva.

Looks good

Restricted Application changed the subtype of this task from "Deadline" to "Task". · View Herald TranscriptSep 13 2018, 6:40 PM