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

[mediawiki/skins/MinervaNeue@master] Fix deprecation warnings in Minerva

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

User facing issue that was caused by the parser migration.

Change #1100876 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Fix deprecation warnings in Minerva

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

Change #1125523 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/extensions/MobileFrontend@master] Fix findSectionHeadingByIndex() to account for new heading structure

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

I updated this task into a Spike because it turned out to be more complicated and annoying to fix, I have a POC up here: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/1125523

What I found out in my spike:

• Page issues isnt opening because findChildInSectionLead() is not working with the new heading markup. This function is used to query for ambox and other banner elements for a given section (i.e. in between 2 heading elements)
  • This fix could be entirely in code, or it could potentially involve a config change (i.e. T388083).
• This code parses thru the HTML to make this query, and there are multiple versions of HTML that we need to account for: Legacy mobile (i.e. section collapsing with section-heading), parsoid mobile, and "desktop" mobile (i.e. viewing a page on desktop with the useskin=minerva url param
• The tests for PageHTMLParser need to be updated to reflect the new heading structure.
• The tests and code should be better documented on these cases

QA

• qa is a bit difficult, you can use content provider to see the page issue, but the useparsoid query param doesnt seem to work well with content provider pages

Page issues isnt opening because findChildInSectionLead() is not working with the new heading markup. T

When you refer to "new heading markup" could you be explicit about what you mean? I think we only need to support Parsoid and legacy parser. In Parsoid that should be easy (due to section tags for all elements). For legacy parser I think it's fine to restrict to elements with and ID e.g. mf-section-2

This function is used to query for ambox and other banner elements for a given section (i.e. in between 2 heading elements)

Why between 2 heading elements? I thought we only need to identify amboxes in a given section? Why do we need that level of granularity? Could you explain why the feature needs to be able to do that?

(i.e. viewing a page on desktop with the useskin=minerva url param

I think we should stop supporting this given it's broken, not widely used and complicated to support.

The tests and code should be better documented on these cases

Agreed.

When you refer to "new heading markup" could you be explicit about what you mean?

heading elements are wrapped in a mw-heading div now, this is for both parsoid and non parsoid

<div class="mw-heading mw-heading3">
  <h3> h3</h3>
</div>

Why between 2 heading elements?

thats just how findChildInSectionLead() functions. From what i understand its because desktop html doesnt have sections. also, you can have amboxes in nested sections. We could refactor this, but that would be rewriting not just these 2 functions, but the entire logic for progressively enhancing page issues, which i dont have context to.

I think we should stop supporting this given it's broken, not widely used and complicated to support.

i think thatd be great, @ovasileva is it acceptable to stop supporting page issues overlaps for "desktop" mobile users (i.e. viewing a page on desktop with the useskin=minerva url param

Change #1125523 abandoned by Bernard Wang:

[mediawiki/extensions/MobileFrontend@master] POC: Fix findSectionHeadingByIndex() and findChildInSectionLead to account for new heading structure

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

I can confirm what Bernard is saying here. The switch to Parsoid has made the code difficult to read, and means we need to consider 3-4 different types of HTML.

This is a small bug but this is no small fix. This feature likely needs a partial rewrite (proposed in T388696 ) if we need to keep it as we've neglected it for long enough that it's become unmanageable.

Given the bug, the other option is to remove the code enhancements for non-lead section so at least these page issues are usable in the short term.

Test Steps

Status: ❓
Environment: Beta
OS: macOS
Browser: Chrome
Device: MS

Test Case 1: Verify page issues banner opens on mobile legacy parser

1. Visit PayScale article with legacy parser.
2. Scroll down to the Overview section.
3. Click the page issues banner.
4. ❌ AC1: Confirm the banner opens and displays the issue details.

5. Open the JavaScript console.
6. ✅ AC2: Confirm there are no warnings or errors in the console.

@Jdlrobson-WMF I think this made it into the deployment testing task inadvertently. I documented and tested it. It doesn't work, but there are no console errors. But reading the comments I think that is what was expected.

@Edtadros this is a pass as AC1 was moved to T388696: [Regression] Page issues banner non-function on mobile legacy parser