Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: TabsOverflow not being properly disposed when List<items> is changed in FluentTabs #3268

Closed
xeroiv opened this issue Jan 29, 2025 · 12 comments · Fixed by #3284
Closed
Labels
status:in-progress Work is in progress

Comments

@xeroiv
Copy link

xeroiv commented Jan 29, 2025

🐛 Bug Report

I have a component that builds makes a tab for each item in a list. When the user filters the list down to a subset of items the number of tabs is correctly updated in the UI but it leaves behind an orphaned badge of overflow items that previously existed.

💻 Repro or Code Sample

<FluentSwitch @bind-Value=hideInactive >Hide Inactive Items</FluentSwitch>
<FluentTabs @bind-ActiveTabId="@activeid" OnTabChange="HandleOnTabChange" Data="@Items">
@foreach (var item in Items)
{
     <FluentTab Id="@($"tab-{item.Name}")" @bind-Label="@item.Name" Data="@item" > item.Content </FluentTab>
}
</FluentTabs>

@code 
{
     private bool hideInactive;
     List<Items> => Projects.Where(n => n.Active == hideInactive).ToList();
}

🤔 Expected Behavior

If the number of items in the list changed such that there is no longer an overflow, those items should be properly disposed of from TabsOverflow.

😯 Current Behavior

When I change the slider to hide inactive projects, the number of tabs is reduced to the expected amount. If there was an overflow prior to reducing the list of items then the overflow badge remains along with any tab-ids that were in the overflow prior to the item list change.

🌍 Your Environment

  • OS & Device: Windows 11 on PC
  • Browser: Microsoft Edge
  • .NET SDK Version: 9.0.101
  • Fluent UI Blazor library Version: 4.11.3
  • Project Target: net8.0
@microsoft-github-policy-service microsoft-github-policy-service bot added the triage New issue. Needs to be looked at label Jan 29, 2025
@vnbaaij
Copy link
Collaborator

vnbaaij commented Jan 30, 2025

The overflow is only being updated based on when a resize of the parent element takes place (this is done in JS). I need to see if that can be triggered in a different way as well.

The FluentTabs knows nothing about your Items collection and will not do anything with changes to that collection. Placing Items in the Data parameter is just for creating a storage for that data. Same goes for storing the item in the FluentTab Data. The components do nothing with that information

@xeroiv
Copy link
Author

xeroiv commented Jan 30, 2025

Thanks for the under the hood explanation. I figured it might have to do with window resize events to update it but the badge still remained after. You can trigger the same behavior on the demo site by removing items from the dynamic example until the overflow isn't needed anymore but the list has more than one item left. Trying to remove the final item does get rid of the badge.

@vnbaaij
Copy link
Collaborator

vnbaaij commented Jan 30, 2025

Yes, the clicking the close icon on a tab (which is what you do in the example you mention) calls the unregister method on the FluentTabs which then ultimately calls a JS overflow refresh function.

There are be some pretty big changes needed to make this callable in another way as well.

@xeroiv
Copy link
Author

xeroiv commented Jan 30, 2025

Yeah I was reading the internal methods and saw that is what it was doing. However it appears there is still some unexpected behavior if you don't remove from the end of the list. Even when resizing the page the overflow doesn't go away.

Image

@vnbaaij
Copy link
Collaborator

vnbaaij commented Jan 30, 2025

Was looking at that one as well. I think it is cause by the internal tabs collection getting out of sync with the actual DOM tabs collection. No solution found yet...

@xeroiv
Copy link
Author

xeroiv commented Jan 30, 2025

So I was looking at potential work arounds so I changed my filter function so that instead of changing the collection, I would use the Visible property to hide the filtered items. When I apply the filter to hide all the Inactive Items, if there were any active items in the overflow they are correctly removed from the overflow list and displayed in the correct order in the tabs element. However any Inactive Items that were previously in the overflow remain in the overflow. They remain in the overflow despite the tabs being hidden and unable to be able to be navigated to. That overflow persists past window resizes.

@rpodevns
Copy link
Contributor

In my testing, removing the January tab corrupts _tabs -- see Watch window in pic (haven't tested other cases). The key for the February tab tab-02 remains correct, but its value is replaced with March. This pattern continues, causing a shift in subsequent tab values.

Image

@rpodevns
Copy link
Contributor

I see nothing in the code that would cause this issue with _tabs and wondering if maybe it's a synchronization or race condition.

@rpodevns
Copy link
Contributor

There may be an underlying Blazor issue .. particularly in how dictionary updates are handled.

Blazor’s UI diffing appears to be causing Dictionary<string, FluentTab> _tabs to behave incorrectly when a tab is removed. Removing an item from TabsDynamic.Items does not remove the FluentTabs._tabs item Key from _tabs. Instead, Blazor shifts Values left while preserving Keys, leading to mismatched Key/Value assignments:

When removing the January tab, it's associated tab-01 Key remains but now references February instead of January.

_tabs.Count does not decrease. Instead, it keeps the last tab (tab-12) in its original position.

Converting _tabs to a List<FluentTab> prevents the key-value mismatches.

There also appears to be an issue with passing FluentTab to FluentTabs.UnregisterTabAsync. After the tab is removed from Items in TabsDynamic.CloseTab, the FluentTab parameter shifts reference to the next tab -- February -- instead of retaining its original parameter value -- January in this case..

Added some HashCode logging and verified that Blazor is upadating _tabs in-place and _tabs does not shrink on it's own causing the last tab to persist so explicit removal is required.

If n items are programmatically removed from the Items list then n items will likely need to be removed from the end of the _tabs list.

HashCode Test Results

Before OnTabClose: _tabs instance = 11555583, Count = 12

tab-01 => HashCode: 58806301
tab-02 => HashCode: 545308
...
tab-12 => HashCode: 12562072

After OnTabClose: _tabs instance = 11555583, Count = 12
INFO: tab-01 is removed from _tabs when removed from Items

tab-02 => HashCode: 58806301 (previously tab-01’s position)
tab-03 => HashCode: 545308
...
tab-12 => HashCode: 17734827 (previously tab-11’s position)
tab-12 => HashCode: 12562072 (Last tab is not removed)

This is what works for me:

  1. Convert _tabs from a Dictionary<string, FluentTab> to a List<FluentTab>
  2. Pass string id to UnregisterTabAsync instead of FluentTab to avoid reference issues.
  3. Explicitly remove the last tab becausee _tabs does not shrink automatically

Let me know if the sounds way out in left field.

@vnbaaij vnbaaij added status:in-progress Work is in progress and removed triage New issue. Needs to be looked at labels Jan 31, 2025
@vnbaaij
Copy link
Collaborator

vnbaaij commented Jan 31, 2025

No, this does not sound way out at all! I was looking at this yesterday and sort of came to the same conclusion. Just did not think to change the Dictionary to a List.

  1. Explicitly remove the last tab becausee _tabs does not shrink automatically

Where/when are you doing that? I tried a couple of your proposed changes but it is still not working as it should. Can you list the updated code here of do a quick PR?

@rpodevns
Copy link
Contributor

FluentTabs.razor.cs is attached. Most changes are for the Dictionary to List conversion.

    internal async Task UnregisterTabAsync(string id)
    {
        if (OnTabClose.HasDelegate)
        {
            var tab = _tabs.FirstOrDefault(t => t.Id == id);
            await OnTabClose.InvokeAsync(tab);
        }

        if (_tabs.Count > 0)
        {
            _tabs.RemoveAt(_tabs.Count - 1);
        }

          // Set the first tab active
        FluentTab? firstTab = _tabs.FirstOrDefault();
        if (firstTab is not null)
        {
            await ResizeTabsForOverflowButtonAsync();

            await OnTabChangeHandlerAsync(new TabChangeEventArgs()
            {
                ActiveId = firstTab.Id,
            });
        }
    }

3268 - FluentTabs.razor.txt

@vnbaaij
Copy link
Collaborator

vnbaaij commented Feb 1, 2025

Fixed with PR mentioned

@vnbaaij vnbaaij closed this as completed Feb 1, 2025
vnbaaij added a commit that referenced this issue Feb 7, 2025
* Replace Dictionary with a List and fix overflow keeping closed items. Fix #3268

* Process review comments. For now we will go with the quick fix

* Remove unneccesary null negating
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:in-progress Work is in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants