Section collapsing is broken in MobileFrontend
Closed, ResolvedPublic5 Estimated Story Points
Actions

Description

Section collapsing (and thus section editing) is broken in the mobile view.
This is also blocking all merges to the MobileFrontend master branch.
This is related to core changes relating to T37247

The problem can be clearly seen on:
https://en.m.wikipedia.beta.wmflabs.org/wiki/Los_Angeles_Lakers

Notice how:

all sections appear uncollapsed
you cannot collapse the headings
it is not possible to edit sections
the edit icon/collapse chevron are not visible alongside the heading

Due to the deployment schedule this will not hit production till 1.30.0-wmf.2 (group0 is 23rd May)

Acceptance criteria

It is possible to collapse and edit sections with cached HTML
It is possible to collapse and edit sections with new HTML

Details

Related Changes in Gerrit:

Subject	Repo	Branch	Lines +/-
Correctly handle the mw-parser-output wrapper	mediawiki/extensions/MobileFrontend	wmf/1.30.0-wmf.1	+25 -10
Correctly handle the mw-parser-output wrapper	mediawiki/extensions/MobileFrontend	master	+25 -10
Revert "Wrap parser output in <div class="mw-parser-output">"	mediawiki/core	master	+13 -83

Customize query in gerrit

Related Objects

Mentioned In: T165161: Text extracts empty for some articles
T165117: First paragraph transform broken
T165115: action=mobileview sections delivering a div of class "mw-parser-output" without a matching closing div
T165034: MobileFormatter uses hardcoded "mw-parser-output" class which is configurable via ParserOptions
Mentioned Here: T150105: Clicking back icon in edit view of mobile site should always display the article
T37247: content-holding <div> should only contain the page text

Event Timeline

Jdlrobson created this task.May 8 2017, 12:14 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 8 2017, 12:14 PM

Change 352577 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Revert "Wrap parser output in <div class="mw-parser-output">"

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

gerritbot added a project: Patch-For-Review.May 8 2017, 12:14 PM

Jdlrobson triaged this task as Unbreak Now! priority.May 8 2017, 12:15 PM

Restricted Application added subscribers: Jay8g, TerraCodes. · View Herald TranscriptMay 8 2017, 12:15 PM

This may also impact the mobileview api which is used by Android/iOS apps, but I've not checked.

Jdlrobson moved this task from Incoming to 2016-17 Q4 on the Web-Team-Backlog-Archived board.May 8 2017, 1:18 PM

In T164733#3243638, @Jdlrobson wrote:

This may also impact the mobileview api which is used by Android/iOS apps, but I've not checked.

That should have been fixed by this change. Have you tried doing the same in whatever your section collapsing is doing?

I will have a look soon, but the MobileFormatter doesn't directly use ParserOutput it runs on the output direct from core via the OutputPageBeforeHTML hook so I doubt it.

It looks like this was deployed this morning, so it should be on the train and in production by Wednesday. I am not confident in the team's ability to fix this in time in the space of 48hrs for the current train so the patch should either be reverted or this week's roll out should be stalled. cc @greg

In that case, your fragile DOM-hacking code will have to deal with the changed structure.

Looking at the way the hook is called, you might be able to get away (for now at least) with detecting if the one child of <body> is a div with class mw-parser-output, and if so processing the children of that div instead of the children of <body>. Maybe something like this near this line.

// If the only element is a `<div class="mw-parser-output">`, process that instead of
// the `<body>` directly. (T164733)
$hasNextElement = false;
for ( $node = $sibling->nextSibling; !$hasNextElement && $node; $node = $node->nextSibling ) {
    $hasNextElement = $node instanceof DOMElement;
}
if ( !$hasNextElement && $sibling->nodeName === 'div' && 
    $sibling->getAttribute( 'class' ) === 'mw-parser-output'
) {
    $body = $sibling;
    $sibling = $body->firstChild;
}

That and adding #mw-content-text > .mw-parser-output here seems to work in a quick test with mediawiki-vagrant, and should work as long as nothing adds other elements to the passed HTML string.

In T164733#3244607, @Anomie wrote:

In that case, your fragile DOM-hacking code will have to deal with the changed structure.

Sure, but we need time to do this. As I've said before this is a very old and vital piece of MobileFrontend's infrastructure and we're not going to easily fix this in the short time we have. We need to ensure we don't pollute the parser cache as well as test on old and new HTML.

Please revert and give us time to address this as this is a serious regression that should under no circumstances land in production.

• JMinor moved this task from Needs Triage to Tracking on the Wikipedia-iOS-App-Backlog board.May 8 2017, 6:42 PM

Jdlrobson added a project: VisualEditor.May 8 2017, 6:52 PM

Jdlrobson updated the task description. (Show Details)

• Jhernandez subscribed.May 8 2017, 7:02 PM

@MBinder_WMF can you facilitate an estimation?
@ovasileva this should be the top priority of the sprint now, so I guess we should move something out in it's place post estimation?

@Jdlrobson Happy to. I'm assuming you mean in a face-to-face hangout. The next opportunity may be tomorrow's standup. Does that work, given that this is UBN (and this presumably urgent)? :)

Jdlrobson removed a project: Patch-For-Review.May 8 2017, 7:59 PM

Jdlrobson updated the task description. (Show Details)

dr0ptp4kt added a project: Reading-Admin.May 8 2017, 8:32 PM

dr0ptp4kt moved this task from Backlog to Doing on the Reading-Admin board.

Change 352737 had a related patch set uploaded (by Tim Starling; owner: Tim Starling):
[mediawiki/extensions/MobileFrontend@master] Correctly handle the mw-parser-output wrapper

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

gerritbot added a project: Patch-For-Review.May 9 2017, 1:16 AM

Change 352577 abandoned by Jdlrobson:
Revert "Wrap parser output in <div class="mw-parser-output">"

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

Tim's patch https://gerrit.wikimedia.org/r/#/c/352737/2 seems to fix this problem but needs a good thorough review.

^ Per T164733#3246453.

• Qse24h closed this task as a duplicate of T164723: New git repository: <repo name>.May 9 2017, 10:07 AM

• Qse24h closed this task as a duplicate of T164723: New git repository: <repo name>.

Aklapper reopened this task as Open.May 9 2017, 10:19 AM

Dbrant moved this task from Needs Triage to Tracking on the Wikipedia-Android-App-Backlog board.May 9 2017, 2:36 PM

MBinder_WMF set the point value for this task to 5.May 9 2017, 5:10 PM

Anthony, could you please test out how toggling is working across the beta cluster? There should be no pages where toggling is not possible so if you find anything please let me know.

This regression is pretty big so we should aim to have testing done by May 17th to have increased confidence that the problem has disappeared.

Thank you!

Change 352737 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Correctly handle the mw-parser-output wrapper

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

ReleaseTaggerBot added a project: MW-1.30-release-notes (WMF-deploy-2017-05-23_(1.30.0-wmf.2)).May 10 2017, 2:00 PM

pmiazga removed a project: Patch-For-Review.May 10 2017, 5:04 PM

Jdforrester-WMF moved this task from To Triage to External and Administrivia on the VisualEditor board.May 10 2017, 5:47 PM

Hello, I did some initial testing using crossbrowsertesting.com and so far sections are expanding and collapsing normally. The edit icons are present and working, but I noticed a side effect.

It is possible to get stuck in a loop by doing the following:

Open https://en.m.wikipedia.beta.wmflabs.org/wiki/Los_Angeles_Lakers (logged out)
Scroll to any section and expand it
Tap on the edit icon of a sub-section, like Rivalries>Boston Celtics
Tap Edit without logging in
Tap back (arrow next to Editing), tap back again

At this point, the user is stuck in a loop between the edit page and the login/signup page and cannot back to the article. Sometimes, the bug does not occur on the first attempt, but doing the steps again seems to reproduce it. @Jdlrobson Should this be a separate ticket?

Tested on:
iPad Pro iOS 10.2 - Safari 10
iPad 2 iOS 8.1 - Safari 8.0
iPad Mini iOS 7.0 - Safari 7.0
iPhone 6 iOS 8.1 - Safari 8
Nexus 6P Android 7.0 - Chrome 55
Galaxy 6 Android 5.0 - Android 5.0

Note - I am seeing something similar while logged in. I will continue to research that behavior, but I wanted to get this posted sooner than later.

In T164733#3253549, @ABorbaWMF wrote:

It is possible to get stuck in a loop by doing the following:

Open https://en.m.wikipedia.beta.wmflabs.org/wiki/Los_Angeles_Lakers (logged out)

Did you try the same on en.m.wikipedia.org? If it happens there too, it's a separate issue.

Thanks, @Anomie

I don't usually work on editing much. It is happening on enwiki. I'll log a separate bug.

Actually I think this is the ticket that covers this issue: https://phabricator.wikimedia.org/T150105

Yup! T150105 tracks that. Thanks for testing @ABorbaWMF

pmiazga mentioned this in T165034: MobileFormatter uses hardcoded "mw-parser-output" class which is configurable via ParserOptions.May 11 2017, 12:29 PM

ovasileva moved this task from Needs QA to Ready for Signoff on the Readers-Web-Kanbanana-Board-Old board.May 11 2017, 5:05 PM

looks good, all done

Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptMay 11 2017, 5:53 PM

This problem is now on enwiki. I'm in the releng channel trying to sort this out.
Section collapsing and section editing is broken across all pages in production.

Jdlrobson mentioned this in T165115: action=mobileview sections delivering a div of class "mw-parser-output" without a matching closing div.May 12 2017, 9:22 AM

Change 353514 had a related patch set uploaded (by Hashar; owner: Tim Starling):
[mediawiki/extensions/MobileFrontend@wmf/1.30.0-wmf.1] Correctly handle the mw-parser-output wrapper

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

gerritbot added a project: Patch-For-Review.May 12 2017, 9:33 AM

Change 353514 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@wmf/1.30.0-wmf.1] Correctly handle the mw-parser-output wrapper

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

Mentioned in SAL (#wikimedia-operations) [2017-05-12T09:59:23Z] <hashar@tin> Synchronized php-1.30.0-wmf.1/extensions/MobileFrontend: Correctly handle the mw-parser-output wrapper - T164733 (duration: 00m 43s)