Page MenuHomePhabricator

FOUC when loading talk pages on mobile using Parsoid read views
Open, In Progress, MediumPublic3 Estimated Story Points

Description

MF uses 22px left padding to hold space for the expand/collapse icon, however this doesn't behave like the real icon when more content is added to the heading.

Before JSAfter JS
image.png (951×451 px, 95 KB)
image.png (951×451 px, 97 KB)

QA steps

Event Timeline

Change #1101034 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/extensions/MobileFrontend@master] Reserve space for collapsible heading icon more accurately

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

This will become a priority once Parsoid read views for talk pages start happening but it currently doesn't affect any production sites.

Jdlrobson triaged this task as Medium priority.Jan 6 2025, 6:42 PM
Jdlrobson changed the task status from Open to In Progress.Jan 7 2025, 11:41 PM
Jdlrobson subscribed.

There is an open patch from Ed here that touches article page views. Given issues we've had with parsoid and non-Parsoid views, web team should be involved with code review for this change.

We discussed this and want to consider a few other approaches to solve this issue.

SToyofuku-WMF set the point value for this task to 3.Jan 14 2025, 5:09 PM

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

[mediawiki/extensions/DiscussionTools@master] Fix Parsoid flash of unstyled content on talk page load

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

I've proposed an alternative patchset which seems to resolve the issue ^

I don't think it's a good idea to use downstream hacks. The upstream fix I proposed creates a more accurate placeholder for the expand/collapse button in the same place that the expand collapse button is defined.

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/1101034/comments/e5cf1cb8_a9c421aa
I think generally we'd like to avoid changing these styles wherever possible given Wikivoyage relies on these for page views

Is there any indication that Wikivoyage have customised these styles in any way that is likely to cause a regression? The possibility of a minor visual regression on mobile Wikivoyage does not seem like the best reason to avoid doing the right thing and making our code more robust and maintainable.

resources/mobile.init.styles/main.less runs on all wikis not just Wikivoyage so a change here is risky while we are focused on other parts of the code. Both solutions seem hacky TBH since they are changing the default styles of a skin which is neither the responsibility of MobileFrontend or DiscussionTools. I recommended the latter given it removes the web team from blocking you from progress.

The current situation means any time the web team or editing team modify the headings we risk breaking each other's experiences and set ourselves up for a game of whack a mole. If we want our code more robust and maintainable then web and editing team should dedicate some time creating stable APIs in PHP and JS for appending items to section headings with well understood contracts. The community has already requested one for gadgets [1] so I think that would a be a good use of our time. What do you think?

[1] T337286: Gadget API for adding buttons to section titles

Both solutions seem hacky TBH since they are changing the default styles of a skin which is neither the responsibility of MobileFrontend or DiscussionTools. I recommended the latter given it removes the web team from blocking you from progress.

MobileFrontend implements collapsible headings, so it is responsible for the styling of them, not the skin (yet), both before and after JS loads. This is the status quo. All my patch does is fix the before-JS implementation from a naive padding-left: 22px to one which more closely matches the behaviour of after-JS experience. Implementing it downstream in DT would indeed be a hack, which is why I want to do this in MF.

A quick code search and on-wiki search for mf-collapsible-heading gave no results and I'm unaware of anyone relying on this styling specificially (although you mentioned Wikivoyage which is I asked about it).

Better APIs would be nice, but for now I'm just trying to fix the current behaviour.

Better APIs would be nice, but for now I'm just trying to fix the current behaviour.

I understand that you’re eager to resolve this quickly, but the proposed patch affects all pages in order to address an issue specifically related to talk pages.

The web team is currently managing multiple versions of mobile markup (Parsoid mobile, legacy parser mobile, Parsoid with $wgParserEnableLegacyHeadingDOM enabled, and legacy parser with $wgParserEnableLegacyHeadingDOM disabled). Given current priorities, this is not our primary focus. Any change in this area, no matter how small, requires a context switch and carries a risk of additional workload for the team.

To help move this forward, I’m suggesting an alternative approach that limits the fix to talk pages without involving the web team. Additionally, I’m open to collaborating on a long-term solution, which seems like a more efficient use of our time than repeatedly addressing isolated breakages.

The team (with me as the one who moved it: T381655#10459386) has already indicated that we won’t be able to take this on during the current quarter as proposed. However, if you’d like to revisit the prioritization, I encourage bringing it up in the next web/editing sync, as my understanding is that these meetings are intended for discussions around shifting priorities.

Not quickly, but correctly. Patching DiscussionTools is just building up technical debt, whereas our proposed patch provides a better placeholder that should work for both talk pages and any future use cases.

Perhaps we can take responsibility for QA / fixing regressions / rolling back if it proves to more complex than it appears?

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

[mediawiki/extensions/MobileFrontend@master] WIP: Server side render the collapse arrow in MobileFrontend

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

Perhaps we can take responsibility for QA / fixing regressions / rolling back if it proves to more complex than it appears?

My concern here is that we probably won't know about issues until a few months later when Parsoid rolls out further to more wikis. The problem we have right now is not many people are reporting Parsoid bugs as they happen and they keep leaking into the HTML here. My understanding is that Parsoid's goal is to be everywhere by end of next quarter and it's likely the web team will have a hypothesis relating to this in Q4 so would want to be closely involved with any heading changes. Having less variations of this HTML will be helpful. I've also recommended we drop support for legacy markup to make this easier in our world (

Not quickly, but correctly. Patching DiscussionTools is just building up technical debt

I get it. Patching MobileFrontend is also building up technical debt for web team, and that's what I'm trying to get across here. We need to pay off this technical debt as part of the Parsoid release.

Since we are both complaining about technical debt.. it seems like either way one of our team's here ends up with code they'd consider technical debt.. I really think it would be worthwhile relying less on hacks and formalizing this more.

I think ideally, the ext-discussiontools-init-section-metadata and h element would be ancestors of the same element so we could rely on HTML for layout. Are there reasons you didn't mark it up that way?
Given DiscussionTools has shown the concept of adding context to headings is useful (also the gadget community is regularly asking for this). Is this something the editing team would be interested in collaborating on formalizing in PHP/JS (so we're not resorting to badly-defined CSS hacks that need to be kept in sync with Codex)?

Currently:

<div class="mw-heading">
   <span class="mf-icon mf-icon-expand mf-icon--small indicator "> </span>
    <h2></h2>
   <span class="mw-editsection" />
   <span class="ext-discussiontools-init-section-overflowMenuButton" />
   <div class="ext-discussiontools-init-section-bar"/>
</div>

Ideal proposed version:

<div class="mw-heading">
   <div class="mw-heading-start">
  </div/>
   <span class="mf-icon mf-icon-expand mf-icon--small indicator "> </span>
   <div class="mw-heading-main">
       <h2></h2>
      <div class="ext-discussiontools-init-section-bar"/>
  </div>
   <div class="mw-heading-end">
      <span class="mw-editsection" />
      <span class="ext-discussiontools-init-section-overflowMenuButton" />
   </div>
</div>

I'm not sure whether it makes sense to do this first in DiscussionTools, MobileFrontend or in the parser, but I'm open to either option.

In lieu of that, really the right thing to do here is the above - to make the parsoid version more consistent with legacy (which is the above patch) rather than fiddling more with this CSS technical debt in MobileFrontend.

Change #1113229 abandoned by Jdlrobson:

[mediawiki/extensions/DiscussionTools@master] Fix Parsoid flash of unstyled content on talk page load

Reason:

Ed considers this tech debt, so let's not do this. Suggested https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/1132030/ instead.

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

Change #1132030 abandoned by Jdlrobson:

[mediawiki/extensions/MobileFrontend@master] WIP: Server side render the collapse arrow in MobileFrontend

Reason:

Abandoning while I'm out. Happy for someone to fork or restore while I'm gone.

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

I get it. Patching MobileFrontend is also building up technical debt for web team, and that's what I'm trying to get across here. We need to pay off this technical debt as part of the Parsoid release.

Since we are both complaining about technical debt.. it seems like either way one of our team's here ends up with code they'd consider technical debt.. I really think it would be worthwhile relying less on hacks and formalizing this more.

This isn't adding any tech debt to MF - it's just improving the existing approach (specifically - using CSS to create a placeholder for the expand/collapse buttons). While this currently only manifests when DT is enabled, it is still a MF bug.

Given DiscussionTools has shown the concept of adding context to headings is useful (also the gadget community is regularly asking for this). Is this something the editing team would be interested in collaborating on formalizing in PHP/JS (so we're not resorting to badly-defined CSS hacks that need to be kept in sync with Codex)?

I'd be happy to work on that in a separate task along with CTT, but I don't want it to block this fix, as this issue is already affecting wikis where Parsoid is deployed.

Change #1101034 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@master] Reserve space for collapsible heading icon more accurately

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

DLynch added a project: Editing QA.
DLynch subscribed.

e.g. on https://en.m.wikipedia.beta.wmcloud.org/wiki/Talk:Main_Page?useparsoid=1 -- the FOUC is only really visible at narrow phone window sizes.

Jdlrobson-WMF removed SToyofuku-WMF as the assignee of this task.EditedTue, Nov 25, 9:34 PM
Jdlrobson-WMF added a subscriber: SToyofuku-WMF.

I replied to the dark mode issue, it relates to something else from what I can see. It looks like a patch was merged in August while I was out on sabbatical. @Esanders @DLynch can this ticket be QAed and resolved?