Page MenuHomePhabricator

Mobile section collapsing doesn't work with Parsoid HTML
Closed, ResolvedPublic3 Estimated Story PointsBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

  • visit mobile site with new parser e.g. office.m.wikimedia.org

What happens?:

  • all sections are expanded and there is no way to expand them

What should have happened instead?:

  • h2s should be collapsible

Software version (skip for WMF-hosted wikis like Wikipedia):

Sign off steps

  • If deemed useful and new information can be applied to T145106 create a follow up tickets for considering alternative markup/implementations
  • Create a ticket for Justin to re-assess the default state of collapsed by default.
  • Create a ticket for any additional follow up

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Change 1010996 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/MobileFrontend@master] [poc] Make MobileFrontend compatible with new Parsoid HTML

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

SToyofuku-WMF set the point value for this task to 3.Apr 4 2024, 5:22 PM
SToyofuku-WMF subscribed.

@Jdlrobson mentioned a desire for us to get an API from the parsoid team to target section headings - we estimated this based on the assumption that we would fix the code in mobilefrontend instead

The API I referred to was T337286. I think this one is actually a 5 pointer. We need to update the existing CSS and JS as well as changing the PHP that transforms the whole article HTML.

MobileFrontend MobileFormatter currently inserts section tags to legacy HTML. However, Parsoid HTML already has section elements. The Parsoid HTML is extremely close to the MobileFrontend output:

What Parsoid provides:

<section>
<h2></h2>
<p></p><p></p>
</section>

What MobileFrontend needs:

<section>
<h2></h2>
<div><p></p><p></p></div>
</section>

The reason it needs the DIV is that the H2 hide/shows the section content.

The best thing to happen here would be to insert the wrapping div in Parsoid and remove the need for the MobileFrontend transform, however I'm not sure if that's possible at this stage (it seems like it should be trivial to do that given we already have section tags ensuring balanced HTML. @ssastry ?

If not, we'll need to detect and find the section element and wrap everything except the the h2 in MobileFrontend but given our strategy of dismantling MobileFrontend and making mobile HTML as consistent with desktop as possible this seems like something we should avoid if possible.

We could also consider doing the section wrapping in JS instead of PHP (not sure if any downsides to doing that)

@Jdlrobson We cannot make this change to the canonical HTML Parsoid is generating since that would be a breaking change to the HTML spec and would impact both internal and external clients. We can only make changes that are b/c (so addition of new types of information is okay). We do intend to make major version bump and breaking changes at a later point, but probably once we have rolled out parsoid read views.

That said, if MFE is consuming content from the OutputTransform pipeline in core, this kind of change is feasible there, but afaik, MFE is getting content from ParserCache which is canonical Parsoid HTML.

So, for now, this change would have to live inside MFE.

I don't think MFE is getting canonical Parsoid HTML -- it is getting the output of ParserOutput:getText() which is postprocessed.
In particular, it is using the OutputPageBeforeHTML hook to fetch the final "read view" HTML and then munging it.

I'm not sure we want to add a <div> wrapper around section contents in read views unconditionally, but we could probably add it to the HandleParsoidSectionLinks pass as an optional transform controlled by a ParserOptions flag.

The following code when run on desktop Parsoid content seems to run without problems. If it's not a performance issue, we could explore doing this instead of rewriting the parser. This would also mean we'd be able to add section collapsing on desktop if that made sense.

document.querySelectorAll( '.mw-parser-output > section' ).forEach( ( node ) => {
  const content = document.createElement( 'div' );
  content.classList.add( 'mf-section-content' );
  node.appendChild( content );
  node.childNodes.forEach((node, i ) => {
    if ( !node.hasAttribute ) {
      console.log( 0, node, i );
      content.appendChild( node );
    } else if ( !node.classList.contains( 'mw-heading' ) && !node.classList.contains( 'mf-section-content' ) ) {
      console.log( 1, node, i );
      content.appendChild( node );
    } else {
      console.log( 2, node, i );
    }
  } );
} );
$('.mw-heading').on( 'click', (ev)=> $( ev.target ).next( '.mf-section-content' ).toggle() )

Change #1010996 abandoned by Jdlrobson:

[mediawiki/extensions/MobileFrontend@master] [poc] Make MobileFrontend compatible with new Parsoid HTML

Reason:

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

What Parsoid provides:

<section>
<h2></h2>
<p></p><p></p>
</section>

What MobileFrontend needs:

<section>
<h2></h2>
<div><p></p><p></p></div>
</section>

The reason it needs the DIV is that the H2 hide/shows the section content.

Just so I'm clear on the desired behavior, section collapsing obeys nesting, right? That is, is the desired output:

<section>
<h2></h2>
<div>
  <p></p><p></p>
  <section>
  <h3>...</h3> 
  <div><p></p><p></p></div>
  </section>
</div>
</section>

or something like this:

<section>
<h2></h2>
<div><p></p><p></p></div>
  <section>
  <h3>...</h3> 
  <div><p></p><p></p></div>
  </section>
</section>

We also discussed previously whether section collapsing could be implemented with a :first-child selector, like:

section.collapsed * { display: none; }
section.collapsed *:first-child { display: block; }

and my vague recollection was that the problem with the CSS-only approach was also related to section nesting, although adding a section.collapsed > section selector might be possible there.

From playing around on pages with my phone, it looks like only <h2> elements are collapsed, and that nested <h3> etc elements get collapsed along with their parent <h2>.

Change #1017334 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/core@master] Add ParserOptions::setCollapsableSections()

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

Jdforrester-WMF renamed this task from Section collapsing doesnt work on mobile to Mobile section collapsing doesn't work with Parsoid HTML.Wed, Apr 10, 8:28 PM

Meeting scheduled to discuss this on Monday

Change #1020393 had a related patch set uploaded (by Stoyofuku-wmf; author: Stoyofuku-wmf):

[mediawiki/extensions/MobileFrontend@master] Set collapsable sections parser option

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

Tested and confirmed that @cscott's patch works with the patch I just uploaded

@SToyofuku-WMF @Jdlrobson I was looking at this and I was wondering, is there any reason why we cant use <details> and <summary>?

Parsoid provides:

<section>
<div><h2></h2></div>
<p></p><p></p>
</section>

and all we'd need to get the section collapsing with default browser behavior is:

<details>
<summary><h2></h2></summary>
<p></p><p></p>
</details>

We'd have to have more DOM transformations which sucks, but I think it could reduce the amount of JS and logic

Hmm this is a good idea - I feel like moving towards a natively supported implementation, even gradually, is probably a good idea

Change #1017334 merged by jenkins-bot:

[mediawiki/core@master] Add ParserOptions::setCollapsibleSections()

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

I think this now unblocked. We have one open patches. First one adds the setCollapsibleSections API to Parsoid itself. It looks like there is movement on this patch, and has been conditionally approved pending the addition of a test. The test has now be added and merged.

We have an open request to avoid splitting the parser cache which we need to follow up on.

We made some progress with the styling.

During standup we briefly touched upon using summary/details. I think we should favor semantic HTML that is consistent with Parsoid over adjusting the HTML markup inside MobileFrontend. For those that like historic artifacts this was discussed in 2016 in T145106. If we feel reassessing summary/details has merit please feel free to create a new ticket to consider this at a later date. For now, the goal should be collapsible sections using the existing HTML with minimal CSS/JS (and I would recommend keeping them open by default for now to avoid the complexities of that - this will also be considered in a follow up task). I've captured this in the sign off steps of this ticket.

Talked about this with @SToyofuku-WMF today and given new emerging requirements we don't consider it possible to wrap up this work by the end of the sprint and decided we are going to wrap this work up in an intermediate state.

Once that's done I'll create follow up tickets to continue this work as part of sign off.

For the frontend follow up ticket: first attempt is here, more up to date version with commentary on the state in the commit message is here

Change #1020393 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@master] Set collapsible sections parser option

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

I'll sign this off tomorrow morning.

Created T363933, T363934 and T363935 for follow up work. @SToyofuku-WMF let me know if I've missed anything! Thanks!