Page MenuHomePhabricator

mw-parser-output now clears after block, which is a noticeable change for Category pages where infoboxes used to render floated alongside the automatic content
Closed, ResolvedPublic

Description

On Commons, we have infoboxes in categories, for example see https://commons.wikimedia.org/wiki/Category:South_Pole_Telescope . Up until recently, these were appearing down the side of the category contents, however I just saw that they no longer do this - instead a lot of whitespace is added, and you have to scroll down to get to the contents.

I don't think this is due to any change in the infobox behaviour. I can also see the same at enwp, e.g., at https://en.wikipedia.org/wiki/Category:Football_Manager - where the code hasn't been updated for some time.

Possibly this is something to do with the default settings of how mw-parser-output and mw-category-generated interact with each other? Has something just changed?

Beforeafter
Screen Shot 2021-05-17 at 1.28.29 PM.png (1×2 px, 1 MB)
Screen Shot 2021-05-17 at 1.28.50 PM.png (1×2 px, 630 KB)

Event Timeline

Mike_Peel triaged this task as Unbreak Now! priority.Mar 31 2021, 7:56 PM

Confirmed that others are also seeing this: https://commons.wikimedia.org/wiki/User_talk:Mike_Peel#%7B%7Btl%7CWikidata_Infobox%7D%7D. Being bold and tagging with 'unbreak now' since this affects 3 million Commons categories.

.mw-parser-output is a semantic style parent, it is not itself a layout element and can and does appear anywhere as needed. Akin to things like date-mw and dir=rtl attributes.

I guess this clearfix was meant to be applied to .mw-body-content or #mw-content-text?

this doesn't have anything to do with desktop improvements.

Jdforrester-WMF renamed this task from Odd style behaviour with mw-parser-output to mw-parser-output now clears after block, which is a noticeable change for Category pages where infoboxes used to render floated alongside the automatic content.Mar 31 2021, 8:10 PM

this doesn't have anything to do with desktop improvements.

Except T277218 seems to have been desktop improvements related?

I do not see this as a train blocker. Definitely something that needs fixing ASAP though.

This is serious. May we just undo this mess asap please?

This was broken in wmf.36, so blocking wmf.38 release makes more sense here. It doesn't impact the current train.

I guess this clearfix was meant to be applied to .mw-body-content or #mw-content-text?

Yeh I think that's the new learning here. It overlooked the fact that categories pages insert a non-parser-output content elemeent, so we should update the internal documentation to reflect that.

That said, am a little confused as on https://en.wikipedia.org/wiki/Category:Football_Manager?useskinversion=1 I'm not seeing the clear fix on the .mw -parser-output element... but I am seeing clear: right's on the infobox and the table so I'm not 100% convinced T277218 is actually the culprit here.

This was broken in wmf.36, so blocking wmf.38 release makes more sense here. It doesn't impact the current train.

If this was broken in .36 wouldn't it have been noticed sooner? It seemed to be noticed almost immediately after rolling .37 to group1.

If this was broken in .36 wouldn't it have been noticed sooner? It seemed to be noticed almost immediately after rolling .37 to group1.
Probably caused by the changes from T277218.

English Wikipedia is still on wmf36 and has the same problem unless I'm misunderstanding?

Jdlrobson lowered the priority of this task from Unbreak Now! to Needs Triage.EditedMar 31 2021, 8:40 PM

I've applied a temporary fix to Commons for now which should be applied everywhere in about 15 mins.

The temporary fix seems to work with "?action=purge".

This was broken in wmf.36, so blocking wmf.38 release makes more sense here. It doesn't impact the current train.

No, it wasn't. wmf.37 (T277218, https://gerrit.wikimedia.org/r/673989, etc.) introduces the misinformed clearfix on the mw-parser-output element. This is live on group1 wikis with wmf.37, and works fine on enwiki and wmf.36.

mw-parser-output is used in lots of places as a codesearch will demonstrate. This may e.g. also be used on inline elements by VE, notices, page indicators etc. Numerous phabricator tickets about related issues and pending fixes to utilize mw-parser-output in this way.

We can roll out to enwiki and ask users to find the numerous ways in which it breaks other things, or we can fix it first.

I'm re-adding the blocker, but if you want to use this ticket for the one side-effect it had on Commons which was fixed by needlessly changing user content, that's fine, but please file a separate blocker in that case for the actual problem (or re-open T277218 as blocker).

That edit temporarily disabled the root cause for some pages (category pages) on one wiki (Commonswiki) for desktop users. We should also rollback the software to fix it and any other side-effects elsewhere, too; while re-approaching the root issue in a different way in time for next week's branch.

No, it wasn't. wmf.37 (T277218, https://gerrit.wikimedia.org/r/673989, etc.) introduces the misinformed clearfix on the mw-parser-output element.

I don't see a clearfix added in https://gerrit.wikimedia.org/r/673989 though?

No, it wasn't. wmf.37

Yes, I understand now. I had a misunderstanding that was clarified by @Mike_Peel on IRC. I misunderstood the task description and the issue does not impact English Wikipedia for category pages.

We could instead rollback the software and fix it and any other side-effects elsewhere, too; while re-approaching the root issue in a different way in time for next week's branch.

I would rather not rollback. This has uncovered invaluable information that I want to document better in our code. I accept the risk that rolling forward may impact other pages in similar ways, but my assessment is that the risk is low and unlikely to fall under "Major stylistic problems affecting all pages" as only certain namespaces put elements under the parser output. This UI regression does not meet the criteria of https://wikitech.wikimedia.org/wiki/Deployments/Holding_the_train#Issues_that_hold_the_train

I am at an offsite this week but promise to fix the root cause (using this ticket) as my top priority Monday. I've put it in our sprint board.

The train should roll forward tomorrow per usual and any further potential problems will be patched by yours truly on wiki on Friday and addressed in the code from a much more informed and less distracted place Monday next week. This is my preferred approach here. If you have an issue with this please let's take that conversation offline.

[…] my assessment is that the risk is low and unlikely to fall under "Major stylistic problems affecting all pages" as only certain namespaces put elements under the parser output. […]

Yeah, only non-article pages have something (besides categorylinks and site footer) following after the user-generated content wrapped by mw-context-text. Specifically, we do this on Category pages, but File description pages as well. Might be worth a look to see what would happen on File pages.

But this isn't about the mw-context-text element. That element has effectively always had a float-clear and we expect one there. The issues we have now is that this clear is missing, and a mw-parser-output clear exists instead. Both of which affect other pages as well.

For example, Special like Special:Contributions and RecentChanges have bodyContent which was cleared before and includes user-generated content via interface messages at the top of the page, and for the bottom of the page via mw-contributions-footer and other such mechanisms. These are now no longer being cleared in wmf.37. And on the other hand, there may be inline things like page status indicators that shouldn't be cleared at all. (Although it seems we still use mw-body-content there instead of mw-parser-content, but that will change with T188443, T230471, etc).

It wouldn't surprise me if this does affect articles and commonly used contributor workflows, and it seems trivial to revert/cherry-pick the Vector patch, right? Just on principle alone, I think it's important this becomes our default mode rather than to talk about it much or try to improvise a quickfix when we know it worked fine before and can let everyone else continue while we fix it properly. We missed the bad selector in CR, and caught it now, that's good, but is there value in learning more from this intermediate state, from knowing what else can break when we clearfix mw-parser-output?

For example, Special like Special:Contributions and RecentChanges have bodyContent which was cleared before and includes user-generated content via interface messages at the top of the page, and for the bottom of the page via mw-contributions-footer and other such mechanisms.

And it’s not even only about user-generated content. If one uses the new watchlist and has hiding seen edits on, the (non-user-generated) help text can flow out of the content area:

Screenshot_2021-04-02 Watchlist - Wikipedia(1).png (565×1 px, 77 KB)

And it’s not even only about user-generated content. If one uses the new watchlist and has hiding seen edits on, the (non-user-generated) help text can flow out of the content area:

I'm aware that we historically have cleared floats from special pages, but have been trying to move us away from this, by being more declarative. Thanks for reporting this one.

Watchlist is fixed in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/676602 - has anyone noticed any other issues with special pages?

Looking into this more today.

What about special pages displaying wikitext pages like Special:BookSources and Special:CiteThisPage? They’re broken at the moment, of course, as the train hasn’t arrived yet, but will they be fixed once it does?

@Tacsipacsi thanks for flagging those. will take a look. For completeness could you update your comment with some screenshots to expand on what's broken there?

Nothing is actually broken on these specific pages, but only because neither of them have anything floating at the end (which is the case on enwiki, but nothing prevents users from adding some floating elements). If I make the last elements float and do as if there were no categories on Special:BookSources (add class="catlinks-allhidden" to #catlinks), those floating elements get out of the content area:

BookSourcesCiteThisPage
Screenshot_2021-04-05 Book sources - Wikipedia.png (397×1 px, 59 KB)
Screenshot_2021-04-05 Cite This Page - Wikipedia.png (485×1 px, 74 KB)

https://gerrit.wikimedia.org/r/c/mediawiki/core/+/676602

Previously it was assumed that all skins cleared any content above
the footer. While this was true for all Wikimedia deployed skins it
was not true for non-wikimedia deployed skins.

Which skins are that? Would those not be broken in various ways due to parser features, extensions, and gadgets that expect mw-context-text or one of its parents (like bodyContent) to clear a float? I'm not aware of any such third-party skin, but I guess a third-party skin could choose not to support some of those features.

I don't see how that justifies adding this responsibility to individual features like RCFilters though. And either way it definitely doesn't belong on mw-parser-output. Last Friday it sounded like you would work today on fixing this bug in Vector, is this not the case anymore? I get that impression because it seems we're still looking for impact and trying to change other features instead of addressing the misunderstanding that led to the bug in the first place.

Last Friday it sounded like you would work today on fixing this bug in Vector, is this not the case anymore?

@Krinkle I am looking into this as I said in T279008#6973047 and will have some updates soon. Please be patient, I am having a busy Monday.

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

[mediawiki/skins/Vector@master] [legacy] Restore old floating style inside Vector

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

Jdlrobson lowered the priority of this task from High to Medium.EditedApr 5 2021, 10:45 PM

Nothing is actually broken on these specific pages, but only because neither of them have anything floating at the end (which is the case on enwiki, but nothing prevents users from adding some floating elements). If I make the last elements float and do as if there were no categories on Special:BookSources (add class="catlinks-allhidden" to #catlinks), those floating elements get out of the content area:

BookSourcesCiteThisPage
Screenshot_2021-04-05 Book sources - Wikipedia.png (397×1 px, 59 KB)
Screenshot_2021-04-05 Cite This Page - Wikipedia.png (485×1 px, 74 KB)

These pages look like they would benefit from better markup. I need to look deeper into how these pages get edited, but I'm presuming there are templates / pages somewhere getting imported here. Despite this there is no mw-parser-output class associated with them, so these special pages appear to be trying to simulate the markup of calling SkinTemplate:wrapHTML but without the consistency.

By the way, looks like BookSources has a problem with floats on the table of contents unrelated to this change ? I assume that's an on-wiki issue?

Screen Shot 2021-04-05 at 3.51.41 PM.png (492×2 px, 120 KB)

I spent a good portion of today investigating this bug more thoroughly to try to understand this issue more. Previously the clearing in Vector was completely undocumented, and a desired outcome is to get this rule well-documented and robust by the people that maintain the codebase. Thank you for your patience as I work through the complexities and I will try to answer the questions that have come up in the thread below.

In the short term, I propose restoring the old clearing method to legacy Vector and improving the definition in core (https://gerrit.wikimedia.org/r/67703), using the new skin to flag and fix the remaining problems at our leisure. Ideally this patch should be merged before tomorrow, but the team does not consider this to meet the criteria of a deployment blocker. Most of my team is out for much of this week, so it's not a big problem if this gets merged later in the week (can backport if necessary).

In the longer term, it's clear the core definition needs iteration which we hope to complete after T89981 and before the 1.36 release. This will be tracked in T279388 (which contains a lot of my analysis today).

Change 676954 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/skins/Vector@master] Revert "Use content-parser-output RL SkinModule feature"

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

I am looking into this as I said in T279008#6973047 and will have some updates soon. Please be patient, I am having a busy Monday.

I spent a good portion of today investigating this bug […]

In the short term, I propose restoring the old clearing method to legacy Vector and improving the definition in core (https://gerrit.wikimedia.org/r/67703), using the new skin to flag and fix the remaining problems at our leisure. Ideally this patch should be merged before tomorrow, but the team does not consider this to meet the criteria of a deployment blocker. […]

Can you please make and own this decision, instead of merely proposing something?

Last Thursday, RelEng was ready to revert the Vector patch and it would have taken a mere comment stating "Yes" at T279008, for them to have done that for you. At the time, you decided to keep the change. A change that, if it had gone well would have made no difference to end-users. The question is not: "How bad is the breakage", but: Did your change work as expected, and if not, why aren't we just reverting it and trying again later, outside the critical path? Especially since this change was not meant to have any benefit to users, it should be trivial to revert. There is no shame in reverting and trying again later – in a context where the "no time, not urgent" filter does not apply about issues with less important features. I think otherwise we embark on a singularity toward poor quality products, where we keep lowering the bar further. We can do better than that.

Now after seemingly spending much time on it, it sounds like you are on board with reverting, but have not finished the job; Drafting a partial undo that leaves fr.wikipedia.org and other New Vector users with a rule that we know is incorrect and that we will end up undoing in favour of something that is effectively the same as what was before. So why not make it what it was before, until then? How frwiki and New Vector behave is a product call, I don't want to argue about that. But the reason I'm recommending against this here, is because you're also saying at the same time that you don't have time/resourcing to see that new partial approach through in short time.

So please own this. Either self-merge this partial revert if you're confident in it and think it's close enough to a clean revert, and ask for deployment (or say so on task, we'll make it happen). Or let's do a clean revert and then you can handle the rest whenever there is time for it. If you do want to defer responsibility on this, I'm fine rolling out a clean revert to master/prod tomorrow.

Change 676954 abandoned by Krinkle:

[mediawiki/skins/Vector@master] Revert "Use content-parser-output RL SkinModule feature"

Reason:

It looks like neither of these reverts addresses New Vector, because New Vector already wasn't clearing it in the right place. (It lost its float-clear some months ago already, the one in the footer here doesn't seem to help much.)

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

Change 677038 merged by jenkins-bot:

[mediawiki/skins/Vector@master] [legacy] Restore old floating style inside Vector

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

Jdlrobson closed this task as Resolved.EditedApr 6 2021, 6:36 PM
Jdlrobson claimed this task.

To be continued on T279388. Classic Vector has now been reverted to its previous form.

Jdlrobson updated the task description. (Show Details)