Page MenuHomePhabricator

No longer possible to collapse sections on mobile
Closed, ResolvedPublicPRODUCTION ERROR

Description

Error

Our visual regression tests flagged a serious problem:

Screenshot 2023-11-06 at 12.58.11 PM.png (426×1 px, 49 KB)

Impact
  • It is no longer possible to hide/show sections. This impacts all pages.
Notes
  • This was working fine on Friday, so presumably something got merged between Friday and now.

QA Results - Prod

ACStatusDetails
1T350627#9342923

Event Timeline

Jdlrobson triaged this task as Medium priority.Nov 6 2023, 8:59 PM
Jdlrobson created this task.
Jdlrobson created this object with edit policy "Custom Policy".
Jdlrobson renamed this task from Mobile section toggles have beeb to No longer possible to collapse sections on mobile.Nov 6 2023, 9:00 PM
Jdlrobson raised the priority of this task from Medium to Unbreak Now!.
Jdlrobson removed the point value for this task.

Looks like the MobileFrontend MakeSectionsTransform is no longer working correctly with presumably-changed Parsoid markup -- the linked beta page has the entire article content wrapped in <section class="mf-section-0" id="mf-section-0">.

I thought MFE didn't use Parsoid HTML?

Looks like this query selector in MFE is the problem:

		$containers = $xpath->query( 'body/div[@class="mw-parser-output"][1]' );

After the change, that query selector no longer applies. Patch incoming.

I guess that patch changed the base Parser output as well, so it really could be either.

It looks like all the patch should have done is chang the wrapper from <div class="mw-parser-output"> to <div class="mw-content-ltr mw-parser-output" lang="en" dir="ltr">. So presumably it's some selector somewhere that expects these classes to be on different elements, or something.

EDIT: sorry, didn't see that reply just before I posted.

Quick explanation of the issue:

  1. [@class="mw-parser-output"] is checking for an exact-match of the class attribute.
  2. The parser patch added more classes to the mw-parser-output div.
  3. class="mw-content-ltr mw-parser-output" is no-longer an exact match.

This bug is the XPath-equivalent of the CSS-selector [rel="link"] issue, that we've slowly been fixing by replacing those with [rel~="link"] when we notice them. Sadly, XPath doesn't have an elegant query quite like that, but there's workarounds.

Change 972058 had a related patch set uploaded (by DLynch; author: DLynch):

[mediawiki/extensions/MobileFrontend@master] Fix xpath selector in MakeSectionsTransform for multiple classes

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

Change 972058 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@master] Fix xpath selector in MakeSectionsTransform for multiple classes

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

DLynch claimed this task.

ssastry did a code search and noticed that there's one non-WMF skin that was going to be hit by the same xpath issue, so I made a pull request on it on github.

Thanks everyone for the quick and thoughtful response here!

Edtadros subscribed.

Test Result - Prod

Status: ✅ PASS
Environment: enwiki
OS: macOS Sonoma
Browser: Chrome
Device: MBA
Emulated Device:NA

Test Artifact(s):

QA Steps

✅ AC1: Sections should be collapsible on mobile pages.

screenshot 119.mov.gif (932×432 px, 399 KB)