Change 601368 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Drop the collapsibleTabs code

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

@Jdlrobson the collapsibleTabs code existed for years with a valid purpose. I think removing it will raise some eyebrows at best, or cause further breakages at worst.
Shouldn't we focus on fixing what's been broken by T249372 and T253912 and can be fixed easily, without any complications?

In T71729#6183507, @Jdlrobson wrote:

"with a valid purpose."
Just because something has existed for a long time doesn't mean a good decision was made. :)

That's a good summary of my opinion on many decisions in MediaWiki :-) Some of which I've already discussed previously, hoping you'd say the same there ;-)

However, in this case I referred exactly to a functionality, not a decision. Tabs can get pretty long (see for ex. user pages imported from meta with a "View on meta.wikimedia.org" tab), just cutting this code will result in:

I don't think that can be released to the public. There is a gap to be filled after cutting this code that needs to be planned, designed and implemented. I estimate that will take weeks. In the situation where a recently introduced regression needed fixing, this approach was unfeasible, this is why I've downvoted the patch immediately.

Other than that, I think this code was fickle, and the animation might have looked good when Vector was created, but it's unusual today, so I agree it needs to be revisited. TBH I wanted to get rid of the animation for a long time, I dislike it that much. However, whether cutting it entirely is a good decision, depends on what's the alternative and I sense an "rm -rf * removes all bugs" approach that will certainly result in another disappointment for the users (aka. community).

Analysis

I think this requires a bit more thought. A quick look at the code reveals one design mistake that starts the infinite cycle: line 147 calling
rAF( $.collapsibleTabs.handleResize );
after the animation (hiding/showing ONE tab) finishes. I assume to hide another tab, if necessary.

• The immediate issue: does not check if the direction changes (hide -> show -> hide). Easy fix: on resize event save direction, on animation end check the direction is the same for the next tab to be hidden/shown.
  • Remove animation. It's even easier to fix if the animation is removed: a loop can do (hide -> get size -> repeat if needed) in one cycle, the recalculation after the animation becomes unnecessary, which is the source of the design error.
• The design issue: the number of hidden tabs should be decided only once, upon receiving the resize event.
  • That's a more complicated fix, collapseCondition needs to be replaced by a logic that iterates the tabs and marks for hiding as many tabs as necessary to fit the available width.
  • Without animation it's unnecessary to be this elaborate.

Solution

The code is definitely not going to be run in the new Vector and is pretty complex for what it does.

It's mostly essential complexity for the functionality that was intended. The code is actually good for practicing code reading skills: one aspect of mediawiki code that I appreciate is that a big percentage of the code is diligently written and formatted, resulting in very well readable code. Above the average of what I usually encounter.

I see 3 solutions.
1st solution completely removes this code (collapsing tabs). In this case the tabs will wrap in narrow windows. With the SearchBox moved to the header this will affect windows narrower by about 320px than in legacy, but still not avoidable, it can still happen at above 900px width:

Further complication will be to move the content down when the tabs wrap. With the tabs being absolute positioned now, this would require JS. A compromise that I wouldn't do, but it's an acceptable compromise, imho.
Alternatively, the tabs can be positioned above the content with flexbox (problem with some basic browsers that should be auto-updated), or the tabs can be before the content in the HTML.

2nd solution only removes the animation, but does collapse the tabs: I've detailed this as "Easy fix" in the Analysis section, above.

The question becomes whether we want to maintain this as part of the legacy skin. I have various compromises in my head, and I'll be considering them during the course of this week and suggesting some options.

For legacy 2 solutions can be applied: 2nd solution which only removes the animation ("Easy fix"), 3rd solution that keeps the animation ("complicated fix"), both detailed in Analysis section, above.

I'm happy to submit a patch for any of these solutions, if you wish.

Change 601368 abandoned by Jdlrobson:
[legacy] Drop the collapsibleTabs code

Reason:
To be reconsidered at a later date.

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

@Demian please don't work on this task. Any effort expended here right now is likely to be wasted effort and I know you don't like to do that. The maintenance team has no interest in working or review patches for this task right now after our recent spikes.

In T71729#6194759, @Jdlrobson wrote:

@Demian please don't work on this task. Any effort expended here right now is likely to be wasted effort

Thanks for informing me. This is what I expected.

The maintenance team has no interest in working or review patches for this task right now

maintenance team? Never heard of them. Is that the release engineering team?

after our recent spikes.

Spikes? Do you mean the 3 regressions in one week? (T249372)

In T71729#6194862, @Demian wrote:

Is that the release engineering team?

No it's not.

Spikes? Do you mean the 3 regressions in one week?

No but spikes.

In T71729#6195375, @Aklapper wrote:

In T71729#6194862, @Demian wrote:

Is that the release engineering team?

No it's not.

Which one it is? Can I ask?

There is no "maintenance team", Jon was just referring to the Readers Web team (including himself), who maintain the Vector skin.

Thanks for the explanation, @matmarex. I believe the Readers-Web-Team is a developer team, at least in the context of DI, so calling it a maintenance team was very confusing to me.

In T71729#6194759, @Jdlrobson wrote:

The maintenance team has no interest in working or review patches for this task right now

@Jdlrobson I was trying to understand this sentence. As you are the one reviewing the patches mostly (at least mine), referring to yourself wouldn't be confusing.
I understand you prefer to axe the tab collapsing instead of fixing it. I've detailed why that's not feasible currently and for quite some time.
As I said if and when this patch is requested, I'll do it.

... is likely to be wasted effort and I know you don't like to do that.

We should chat sometime so you can get to know me better. I've seen many times in the months we've been collaborating that my values and motivations are misunderstood or assumed to be something it is not.

To clarify the reading web team that I am tech lead for, who are the maintainers of this skin (and legacy version) talked through this task yesterday in our team time meeting. It was agreed that we don't want to make any changes to legacy mode at this point so we will keep the code for now. In general we are not working on legacy bugs as part of the desktop improvements project - the project is considered frozen with the exceptions of changes that aid the new modern experience we are building. We decided to retain the collapsible tabs code and this bug. In future we'd like to rewrite and improve this code but right now we don't believe that fits into our current goals.

I am thus punting this back to the backlog. This task about fixing the existing bug remains stalled on a decision in T254277 which we hope to revisit further into the project. If we decide not to review it, we may decide to rewrite it or simply continue to maintain it as we do now (in which case this task can be unstalled and worked on).

This is an extremely large number of duplicates, for a task that has been sitting here quietly for years with barely anyone ever noticing the problem.

Could something have changed recently in Vector to make this scenario more common?

The current issue is distinct and maybe we should file a separate task. Whenever any tabs should be collapsed into the "More" menu, and the menu isn't already visible, it doesn't actually appear, and the tabs animate back and forth forever.

I did a git-bisect and this bug seems to have been introduced in rSVEC711a41812aac: Use newly available Skin::getPortletData method to get mw-portlet class, although I haven't tried to figure out what about that code causes it.

mw.util.showPortlet( 'p-cactions' ) reveals the portal. If it's not displaying I suspect 6d967ed4a8c is more to blame here.

oh.. whoops.

Change 636068 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Fix logic

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

Change 636068 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Fix logic in collapsibleTabs code

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

Change 636377 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@wmf/1.36.0-wmf.14] Fix logic in collapsibleTabs code

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

Change 636377 merged by jenkins-bot:
[mediawiki/skins/Vector@wmf/1.36.0-wmf.14] Fix logic in collapsibleTabs code

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

Mentioned in SAL (#wikimedia-operations) [2020-10-26T18:43:25Z] <catrope@deploy1001> Synchronized php-1.36.0-wmf.14/skins/Vector/: Fix logic in collapsibleTabs code (T71729) (duration: 00m 58s)

(The previous patch fixes the latest issue where this problem occurred all the time; the older issue where this problem occurs rarely "if a tab's width changes after initial page load" still exists.)