Page MenuHomePhabricator

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


Steps to Reproduce

Note that the appearance of the table of contents pushes down the remaining content.
In comparison, 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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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:

Screenshot from 2018-06-06 09-36-44.png (621×442 px, 317 KB)

Screenshot from 2018-06-06 09-37-15.png (613×435 px, 319 KB)

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

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

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?

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

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?

Screen Shot 2018-06-19 at 11.00.47 AM.png (1×1 px, 297 KB)

Screen Shot 2018-06-19 at 11.00.27 AM.png (1×1 px, 640 KB)

This still needs code review. 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

Checked 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.

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...

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

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 Medium to High.
Vvjjkkii removed the point value for this task.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed 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

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

Looks good on Safari desktop. is not loading in Chrome. I do not have an iPad to test with. Over to QA.

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