Page MenuHomePhabricator

[collapsibleTabs] If a tab's width changes after initial page load, endless animation loop can happen
Open, Stalled, LowPublic

Description

Alternative: T254277: Proposal: Stop collapsing Vector menu items under more menu and removal associated code

If the "View history" tab changes size such that becomes too large where it before was not, an endless animation loop of collapse-expand-collapse-etc can be triggered. A resize could be caused by an ordinary zoom (frequently resizing the pixel width of the element by a few pixels), by a change in browser text size preferences (likely to significantly change the width), or by a user script modifying the text content of the element.

This appears to be caused by expandCondition in Vector.js using a cached version of the element's width (collapsibleTabs.data's expandedWidth passed as the eleWidth argument), while collapseCondition is checking .width() each time.

This happens when you enable Editing mode: show both tabs is enabled in preferences in VisualEditor and visit https://www.mediawiki.org/wiki/User:Martin_Urbanec?uselang=fr at certain screen resolutions. Demonstrated in image below:

Solutions

Option 1 - Remove the animation and fix the collapsibletabs code:

Rationale: The animation plays at times when it's not meaningful, eg. after page load. The tabs should be collapsed immediately, without animation.
About half of the code is removed, including callbacks, resulting in a simple, less error-prone logic.
Analysis of the bug, solution: T71729#6183821 Remove animation

Option 2 - Keep the animation, fix the collapsibletabs code:

Rationale: Keeps the old functionality (animation) while safeguarding against the oscillation.
Solution: T71729#6183821 Easy fix

Option 3 - Make the tabs responsive

Rationale: No more collapsing, the tabs wrap into any number of rows.
Mostly CSS solution with a one-liner resize event handler.
Solution detailed: T71729#6183822

Update 2020-05-30

This bug resurfaced as a more prominent regression after the MW-1.35-notes (1.35.0-wmf.34; 2020-05-26) deployment:
T253819: Regression: Dancing Search Bar in MediaWiki when menu is hidden
The difference is: the "More" ("Plus" / "Mehr") menu is not visible:


Version: unspecified
Severity: normal

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Aklapper lowered the priority of this task from Unbreak Now! to High.May 30 2020, 10:59 PM

Again: This is definitely not Unbreak Now ("Something is broken and needs to be fixed immediately, setting anything else aside."). Please stop setting that priority.

Demian added a comment.EditedMay 31 2020, 3:46 AM

Again: This is definitely not Unbreak Now ("Something is broken and needs to be fixed immediately, setting anything else aside."). Please stop setting that priority.

Apparently there's no agreement on that: if the image of Wikimedia matters then it needs to be fixed immediately. That being said it does not matter until the next SWAT window, which is on Monday. If you don't agree, I'm happy to discuss this, but please do so off-ticket, it's off-topic here.

@Aklapper the fix I've submitted is for a new regression introduced in wmf.34, which was reported in a wave of recent tickets that were closed as duplicates. Those look similar, but in fact aren't duplicates of this bug from 2014. This causes quite some confusion as people believe these are the same issue, furthermore the original bug won't be fixed by the patch, thus the ticket cannot be closed, leading to further confusion. What solution do you propose to untangle this?

Demian updated the task description. (Show Details)May 31 2020, 7:33 AM

@Aklapper Can you please answer the above relevant question?

Aklapper added a comment.EditedJun 1 2020, 7:29 AM

@Demian: ??? Leaving tickets open when tickets don't get completely fixed.

(PS: I don't think that pinging after less than 24h on a weekend is constructive.)

This code is clearly problematic and clearly hard to maintain it.

Given Vector is not a responsive skin I honestly feel we should remove this code. Optimising the experience of Wikipedia at that resolution is not solving any problem except adding code complexity for a small gain.

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?

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

The code is definitely not going to be run in the new Vector and is pretty complex for what it does. 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.

Oraslaci removed a subscriber: Oraslaci.Jun 1 2020, 11:44 PM

"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:


At 1200px width!

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).

Demian added a comment.EditedJun 2 2020, 6:23 AM
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.
Demian added a comment.EditedJun 2 2020, 6:23 AM
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:


Therefore the tabs row needs a proper responsive layout that can wrap properly. This means having #left-navigation and #right-navigation only in #mw-head. With the personal menu moved to the header this will be the case. I've submitted numerous patches in the past months to do this refactor and layout with flexbox.

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.

Jdlrobson changed the task status from Open to Stalled.Jun 2 2020, 7:44 PM
Jdlrobson lowered the priority of this task from High to Medium.

I've opened up T254277 to propose removing this code.

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 updated the task description. (Show Details)Jun 4 2020, 10:50 PM

3 different solutions detailed above and in the description. I'll do the patch in a day, if it is decided to accept any of these fixes.

@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.

Jdlrobson lowered the priority of this task from Medium to Low.Jun 4 2020, 11:37 PM

@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)

Is that the release engineering team?

No it's not.

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

No but spikes.

Demian added a comment.EditedJun 5 2020, 9:40 AM

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.

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).

Demian moved this task from Incoming to Plan on the User-Demian board.Jul 4 2020, 1:44 AM
Demian moved this task from Plan to Freezer on the User-Demian board.

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.

Jdlrobson added a comment.EditedOct 23 2020, 4:52 PM

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

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)

Aklapper removed Demian as the assignee of this task.Sun, Nov 8, 10:25 AM

[Resetting assignee due to inactive user account]

dbarratt removed a subscriber: dbarratt.Sun, Nov 8, 5:17 PM

(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.)