MudDrawer: Fix initialization behaviour, use ParameterState, remove PreserveOpenState

ScarletKuro commented 2 weeks ago

Description

Replaces PR: https://github.com/MudBlazor/MudBlazor/pull/5012 Fixes: https://github.com/MudBlazor/MudBlazor/issues/4585 Fixes: https://github.com/MudBlazor/MudBlazor/issues/7273 Fixes: https://github.com/MudBlazor/MudBlazor/issues/4535

If you open the docs in small initial size, and start to resize it (large direction), the Drawer will not open. This should fix it. Added our ParameterState framework. I removed PreserveOpenState, it doesn't seems useful anymore, it looks like https://github.com/MudBlazor/MudBlazor/pull/684#issuecomment-769632930 it was just some property to overcome some implementation problems. Now when the _isOpenWhenLarge is gone (it was the main culprit why this whole thing wouldn't work adequately) the PreserveOpenState is the default behavior now because it stays open when you increase the browser size: https://github.com/MudBlazor/MudBlazor/blob/fa7d7e880ff6fc00f388b9aa694747c88408fa42/src/MudBlazor/Components/Drawer/MudDrawer.razor.cs#L317-L321

As I understand the _isOpenWhenLarge existed to restore the state when you lower window from a large window size and then resize it back then it goes to the "original" state before the resize. For example, if you had a large window and drawer closed (manually), then make the window small and back large again the drawer doesn't open, the PreserveOpenState was there to disable this this restore logic, but it doesn't help with initial behavior. This in fact all conflicts with the initial behavior when you start with a small window, the _isOpenWhenLarge is triggering when you resize to small window, but you already started small so it's just doesn't do anything. I removed all this logic, as it hard for me to find the justification for this default behavior, and there were no test coverage for it, all responsive tests were done against PreserveOpenState=true. The logic is simple, if it doesn't meet the breakpoints condition the drawer closes, and it does it opens, no "smart" features like restore state from large window etc.

I tried to chunk the code to small methods so that the main logic would look like this:

if (ShouldCloseDrawer(breakpoint))
{
    await _openState.SetValueAsync(false);
}
else if (ShouldOpenDrawer(breakpoint))
{
    await _openState.SetValueAsync(true);
}

Also it seems like the missing popover provider was the reason why examples wouldn't work locally in Edge as I complained in discord.

How Has This Been Tested?

new bUnit tests Visual

Type of Changes

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[x] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] Documentation (fix or improvement to the website or code docs)

Checklist

[x] The PR is submitted to the correct branch (dev).
[x] My code follows the code style of this project.
[x] I've added relevant tests.

ScarletKuro commented 2 weeks ago

Would be nice, if someone is willing to test it with their drawers.

codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 84.09091% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 90.13%. Comparing base (28bc599) to head (fa7d7e8). Report is 127 commits behind head on dev.

Files	Patch %	Lines
src/MudBlazor/Components/Drawer/MudDrawer.razor.cs	83.33%	4 Missing and 3 partials :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## dev #8833 +/- ## ========================================== + Coverage 89.82% 90.13% +0.30% ========================================== Files 412 421 +9 Lines 11878 12277 +399 Branches 2364 2429 +65 ========================================== + Hits 10670 11066 +396 + Misses 681 662 -19 - Partials 527 549 +22 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

henon commented 2 weeks ago

Added to v7.0.0 Migration Guide #8447

digitaldirk commented 6 days ago

Just upgraded to 7 preview 2 and now when I toggle light/dark mode my drawer closes. I believe this pr is the change that is causing this.

I will try to make a repo to show the issue @ScarletKuro

ScarletKuro commented 6 days ago

I will try to make a repo to show the issue @ScarletKuro

Sure, we will wait for repo, as I couldn't reproduce it on dev.mudblazor.com (it uses latest changes) that also has a drawer and dark/light switch. It's weird since Drawer doesn't eve know anything about the light / dark theme, so it could be something else.

digitaldirk commented 6 days ago

@ScarletKuro Thanks for looking into this.

It seems to be related to the Breakpoint="Breakpoint.SmAndDown" property. When that is removed it works normally. After this issue occurs, if you reopen the drawer (takes 2 clicks to do so), then changing the theme doesn't close the drawer. I also noticed if you resize the browser or fullscreen you will encounter similar issues.

Issue demo: MudDrawerDemo

Repo: https://github.com/digitaldirk/DarkLightModeMudDrawerIssue

My real project (which is not public) has a mud drawer setup more like this, but I removed that complexity from the demo repo:

  <MudHidden Breakpoint="Breakpoint.SmAndDown">
    <MudDrawer Class="px-0" Elevation="5" Open="_drawerOpen" Breakpoint="Breakpoint.SmAndDown" Variant="@DrawerVariant.Persistent" Overlay="false" ClipMode="DrawerClipMode.Always">
      <NavMenu />
    </MudDrawer>
  </MudHidden>

  <MudHidden Breakpoint="Breakpoint.MdAndUp">
    <MudDrawer Elevation="5" Class="pa-0 ma-0" MiniWidth="50px" Fixed="true" OpenMiniOnHover="true" Variant="@DrawerVariant.Mini" Overlay="true" ClipMode="DrawerClipMode.Always">
      <NavMenu />
    </MudDrawer>
  </MudHidden>

ScarletKuro commented 5 days ago

Hi,

I've reviewed your repo, and I believe it now functions as expected. The previous behavior seemed incorrect.

There are two main issues with your code:

Not utilizing bind-Open for the drawer.
Using SmAndDown as the Breakpoint in the drawer (this is more of a bug than a feature, but let me explain).

It seems to be related to the Breakpoint="Breakpoint.SmAndDown"

It may not be easy to explain, but I'll start with the Breakpoint. Though it's not documented, in the current MudDrawer code, SmAndDown is considered "illegal". It only working with Xs, Sm, Md, Lg, Xl, and Xxl, unlike MudHidden.

Even before my changes, the drawer's breakpoint comparison was using MudDrawer.Breakpoint < currentBreakpoint (converting enum to int) instead of IsBreakpointWithinReferenceSizeAsync(currentBreakpoint, MudDrawer.Breakpoint). This means that the code always returned that the breakpoint was below (for SmAndDown and alike breakpoints) the needed value, indicating that the drawer should be in a closed state.

Previously, the drawer wasn't closing because the initial behavior wasn't functioning correctly. Since you're using a Persistent drawer, it won't react to resize changes and it gets unnoticed that drawer never changes its state. Now, with the understanding that the drawer always wants to close itself with SmAndDown, here's what it's happening next:

Since you're not using two-way binding, the drawer wants to close, but you have a variable with an initial value of "true" keeping the drawer open. However, when you click the theme toggle, it internally calls StateHasChanged, causing the internal state of the drawer to switch to its correct state, which is closed.

So the main reasons it was working before are:

The initial drawer state never worked correctly.
Our code was overriding the Open setter and getter, interfering with the parent-child communication.

If you change to @bind-Open="_drawerOpen" and Breakpoint="Breakpoint.Sm", it will function as intended. With the current code, Sm means that all breakpoints from Sm downwards will close the drawer. Based on your configuration, the intended behavior should be as follows: If it's Sm or below, the drawer should be closed; if it's above Sm, it should be open. Since it's persistent, it should not react to any subsequent resize changes. And this is what it will do if you fix this two things.

Perhaps we should consider making changes from our side to handle SmAndDown properly by using the IsBreakpointWithinReferenceSizeAsync method or throwing an exception indicating that it shouldn't be used.

ScarletKuro commented 5 days ago

I added a PR https://github.com/MudBlazor/MudBlazor/pull/8941 that makes Drawer to understand to work not only with Xs, Sm, Md, Lg, Xl, Xxl

digitaldirk commented 5 days ago

@ScarletKuro Thank you for the detailed explanation, that helped me understand the breakpoints and drawers better. Fixed my issue.

MudBlazor / MudBlazor