Skip to content

MudOverlay: Fix scrollable background with visible dialog#12121

Merged
ScarletKuro merged 2 commits intoMudBlazor:devfrom
timmac-qmc:dev
Nov 19, 2025
Merged

MudOverlay: Fix scrollable background with visible dialog#12121
ScarletKuro merged 2 commits intoMudBlazor:devfrom
timmac-qmc:dev

Conversation

@timmac-qmc
Copy link
Contributor

Fixes #12078

Added a visibility check on MudOverlay first render to avoid incorrectly decrementing the overlay lock count.

  • I've read the contribution guidelines
  • My code follows the style of this project
  • I've added or updated relevant unit tests

@mudbot mudbot bot added the bug Unexpected behavior or functionality not working as intended label Nov 18, 2025

// Calling HandleLockScrollChange when the overlay isn't intially set to
// visible will incorrectly decrement the lock count
if (Visible)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be _visibleState.Value, we don't read state properties directly, please verify it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Literally a couple of lines down the code is already doing

if (Visible && !Modal && AutoClose)

This seems somewhat conflicting?

Copy link
Member

@ScarletKuro ScarletKuro Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this was my oversight from when I initially began developing this #8258
This should be changed too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed a change that updates property access for the whole class.

@ScarletKuro ScarletKuro changed the title Fixes scrollable background with visible dialog MudOverlay: Fix scrollable background with visible dialog Nov 18, 2025
Fixes scrollable background with visible dialog & Fix visible property access.

Fixes #12078
@ScarletKuro
Copy link
Member

@versile2 mind to double check this later?

@versile2
Copy link
Contributor

@versile2 mind to double check this later?

The fix looks good and works. I do believe it should have a unit test attached that verifies lock scroll. I know it's a complex test but if you added the trymud attached to the issue to the viewer project your test would look like the below. I've verified this fails without your changes and passes with your changes.

    [Test]
    public void Overlay_StartClosedTest()
    {
        var jsRuntimeMock = new Mock<IJSRuntime>(MockBehavior.Loose);

        Context.Services.AddSingleton(typeof(IJSRuntime), jsRuntimeMock.Object);
        // verifies lockScroll was called once and 2 arguments were supplied
        jsRuntimeMock
            .Setup(x => x.InvokeAsync<IJSVoidResult>("mudScrollManager.lockScroll", It.Is<object[]>(y => y.Length == 2)))
            .ReturnsAsync(Mock.Of<IJSVoidResult>())
            .Verifiable();

        // Expect unlockScroll to NOT be called
        jsRuntimeMock
            .Setup(x => x.InvokeAsync<IJSVoidResult>(
                "mudScrollManager.unlockScroll",
                It.IsAny<object[]>()))
            .Throws(new Exception("unlockScroll should not be called!"));

        var dialog = Context.RenderComponent<MudDialogProvider>();
        var comp = Context.RenderComponent<OverlayScrollLockedTest>();
        // click the button opening dialog
        var button = comp.Find("button");
        button.Click();
        // verify dialog is open
        comp.WaitForAssertion(() => dialog.FindComponent<MudOverlay>().Should().NotBeNull());

        // verify lockScroll was called
        jsRuntimeMock.Verify(
            x => x.InvokeAsync<IJSVoidResult>(
                "mudScrollManager.lockScroll",
                It.IsAny<object[]>()),
            Times.Once);

        // verify unlockScroll was NOT called
        jsRuntimeMock.Verify(
            x => x.InvokeAsync<IJSVoidResult>(
                "mudScrollManager.unlockScroll",
                It.IsAny<object[]>()),
            Times.Never);
    }

Copy link
Contributor

@versile2 versile2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add unit test

@mudbot mudbot bot added the needs: tests A maintainer has explicitly asked for associated test cases to be added to this pull request label Nov 19, 2025
@timmac-qmc
Copy link
Contributor Author

Added the provided test.

@mudbot mudbot bot removed the needs: tests A maintainer has explicitly asked for associated test cases to be added to this pull request label Nov 19, 2025
Copy link
Contributor

@versile2 versile2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work! Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Unexpected behavior or functionality not working as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MudDialog with nested MudOverlay does not lock background page scroll

3 participants