Page MenuHomePhabricator

Section collapsing is broken in MobileFrontend
Closed, ResolvedPublic5 Story Points

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 Gerrit Patches:
mediawiki/extensions/MobileFrontend : wmf/1.30.0-wmf.1Correctly handle the mw-parser-output wrapper
mediawiki/extensions/MobileFrontend : masterCorrectly handle the mw-parser-output wrapper
mediawiki/core : masterRevert "Wrap parser output in <div class="mw-parser-output">"

Event Timeline

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

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.

Anomie added a subscriber: Anomie.May 8 2017, 1:19 PM

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

Anomie added a comment.EditedMay 8 2017, 4:33 PM

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

Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)May 8 2017, 7:19 PM
Jdlrobson added subscribers: ovasileva, MBinder_WMF.

@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 updated the task description. (Show Details)
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

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.

Qse24h closed this task as a duplicate of T164723: New git repository: <repo name>.
Qse24h closed this task as a duplicate of T164723: New git repository: <repo name>.
Qse24h closed this task as a duplicate of T164723: New git repository: <repo name>.
Qse24h closed this task as a duplicate of T164723: New git repository: <repo name>.
Qse24h closed this task as a duplicate of T164723: New git repository: <repo name>.
Qse24h closed this task as a duplicate of T164723: New git repository: <repo name>.
Qse24h closed this task as a duplicate of T164723: New git repository: <repo name>.
Qse24h closed this task as a duplicate of T164723: New git repository: <repo name>.
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
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

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:

  1. Open https://en.m.wikipedia.beta.wmflabs.org/wiki/Los_Angeles_Lakers (logged out)
  2. Scroll to any section and expand it
  3. Tap on the edit icon of a sub-section, like Rivalries>Boston Celtics
  4. Tap Edit without logging in
  5. 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.

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

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

ovasileva closed this task as Resolved.May 11 2017, 5:53 PM

looks good, all done

Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptMay 11 2017, 5:53 PM
Jdlrobson reopened this task as Open.EditedMay 12 2017, 9:18 AM

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.

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

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)

hashar added a subscriber: hashar.May 12 2017, 10:16 AM

Should be all fixed in production now. Deployed with @Jdlrobson .

phuedx closed this task as Resolved.May 15 2017, 8:23 AM
phuedx claimed this task.
phuedx added a subscriber: ABorbaWMF.

I've tested this on a handful of articles logged in/out and it WFM.