Fix MudStaticNavGroup Visibility and Animation#70
Conversation
- Updated JavaScript initialization for NavGroups to correctly manage the 'invisible' class on expansion. - Implemented smooth height transitions using scrollHeight for a native feel in Static SSR. - Updated NavMenu unit tests to accommodate animation timing and class changes. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Review Summary by QodoFix MudStaticNavGroup visibility and add smooth animations
WalkthroughsDescription• Fix MudStaticNavGroup visibility issue in Static SSR mode • Implement smooth height-based animations for expand/collapse • Add invisible class management for proper state handling • Update unit tests with animation timing and class assertions Diagramflowchart LR
A["NavGroup Click Event"] --> B["Check Expansion State"]
B --> C{Will Expand?}
C -->|Yes| D["Remove invisible class"]
D --> E["Animate height to scrollHeight"]
E --> F["Add mud-collapse-entered"]
C -->|No| G["Animate height to 0"]
G --> H["Add invisible class"]
H --> I["Update aria-hidden"]
F --> I
I --> J["Update expandIcon transform"]
File Changes1. src/wwwroot/Extensions.MudBlazor.StaticInput.lib.module.js
|
Code Review by Qodo
1.
|
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a per-element WeakSet initialization tracker, guards re-initialization across static inputs, wires an exported afterWebStarted(blazor) initializer with enhancedload and a MutationObserver, and reworks nav-group expand/collapse into staged height-based animations with updated ARIA, focus, and transient-state handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Blazor as Blazor (.NET)
participant JS as StaticInput JS
participant DOM as Browser DOM
Blazor->>JS: call exported afterWebStarted(blazor)
JS->>JS: attach enhancedload handler and run initialize()
JS->>DOM: query static input elements
loop for each new element
JS->>DOM: initXxx(element) [guard: initializedElements]
DOM-->>JS: element marked initialized
end
DOM->>JS: MutationObserver notices added nodes
JS->>DOM: re-run initialization for added nodes
Note over JS,DOM: Nav-group interactions: staged height changes, aria updates, icon transforms, timeouts cleared
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a visibility issue in the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully addresses the visibility issue with MudStaticNavGroup in Static SSR by implementing a JavaScript-based animation and visibility toggle. The logic is sound and the accompanying test updates validate the fix. My review includes a suggestion to refactor a hardcoded animation duration in the JavaScript file for better maintainability. Additionally, I've recommended replacing Task.Delay in the unit tests with a more robust waiting mechanism provided by Playwright to prevent flaky tests. Overall, this is a good fix.
Test Results50 tests 50 ✅ 13s ⏱️ Results for commit 058a8f9. ♻️ This comment has been updated with latest results. |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/StaticInput.UnitTests/Components/NavMenuTests.cs (1)
30-32: Replace hard-codedTask.Delay(500)sleeps with condition-based waits onaria-hidden/invisiblestate.The JS animation uses a 250ms transition, so the 500ms sleep is arbitrary and makes tests timing-sensitive. This file already uses
Page.WaitForFunctionAsync(...)for drawers; apply the same pattern here. The test checks botharia-hiddenandinvisibleclass, so polling for either would make the nav-group test deterministic and robust to CI slowness or animation-duration changes.Locations
Lines 30–32 and 41–43🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/StaticInput.UnitTests/Components/NavMenuTests.cs` around lines 30 - 32, Replace the hard-coded Task.Delay(500) calls after await button.ClickAsync() in NavMenuTests (both occurrences) with a condition-based wait using Page.WaitForFunctionAsync that polls until the target nav-group element has either aria-hidden="true" or contains the "invisible" class; locate the element being toggled in the test (the same selector used later to assert visibility) and wait for the expression that checks element.getAttribute('aria-hidden') === 'true' || element.classList.contains('invisible') before proceeding to assertions, mirroring the pattern used for drawers in other tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/wwwroot/Extensions.MudBlazor.StaticInput.lib.module.js`:
- Around line 520-551: The transition timers are never cleared, causing stale
callbacks to corrupt classes/ARIA; modify the logic around collapseContainer
(and where wrapper is used) to store the pending timer ID on the element (e.g.,
collapseContainer._transitionTimer), call
clearTimeout(collapseContainer._transitionTimer) before scheduling a new
setTimeout, and then assign the new timer ID back to that property; also
remove/reset transient animation classes ('mud-collapse-entering',
'mud-collapse-exiting') and ensure aria-hidden is set consistently before
starting the new transition so previous state cannot conflict with the new one.
---
Nitpick comments:
In `@tests/StaticInput.UnitTests/Components/NavMenuTests.cs`:
- Around line 30-32: Replace the hard-coded Task.Delay(500) calls after await
button.ClickAsync() in NavMenuTests (both occurrences) with a condition-based
wait using Page.WaitForFunctionAsync that polls until the target nav-group
element has either aria-hidden="true" or contains the "invisible" class; locate
the element being toggled in the test (the same selector used later to assert
visibility) and wait for the expression that checks
element.getAttribute('aria-hidden') === 'true' ||
element.classList.contains('invisible') before proceeding to assertions,
mirroring the pattern used for drawers in other tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f294f05a-2d1e-48c4-b740-2784ee6a4f0d
📒 Files selected for processing (2)
src/wwwroot/Extensions.MudBlazor.StaticInput.lib.module.jstests/StaticInput.UnitTests/Components/NavMenuTests.cs
…on issues - Fixed visibility of NavGroup items by explicitly managing the 'invisible' class and 'display' style. - Implemented smooth height transitions using scrollHeight for expansion and collapse. - Added 'mud-nav-group-expanded' class to fix background highlighting when expanded. - Introduced WeakSet-based element tracking in JS initializer to prevent double-binding and fix visibility issues after enhanced navigation. - Updated unit tests to support animation timing and state changes. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/wwwroot/Extensions.MudBlazor.StaticInput.lib.module.js (1)
540-575:⚠️ Potential issue | 🟠 MajorCancel the previous nav-group transition before starting the next one.
A second click inside the 250ms window leaves both callbacks live. The stale one can re-apply
invisibleormud-collapse-enteredafter the state has already flipped, so the group can end up visually and semantically out of sync again.🛠️ Suggested fix
- // Clear any existing timeouts or styles that might interfere - collapseContainer.style.transition = 'height 250ms cubic-bezier(0.4, 0, 0.2, 1)'; + clearTimeout(collapseContainer._transitionTimer); + collapseContainer.classList.remove('mud-collapse-entering', 'mud-collapse-exiting'); + collapseContainer.style.transition = 'height 250ms cubic-bezier(0.4, 0, 0.2, 1)'; if (willExpand) { navElement.classList.add('mud-nav-group-expanded'); collapseContainer.classList.remove('invisible'); collapseContainer.style.display = 'block'; // Ensure it's not display: none collapseContainer.setAttribute('aria-hidden', 'false'); collapseContainer.classList.add('mud-collapse-entering'); const height = wrapper.scrollHeight; collapseContainer.style.height = height + 'px'; - setTimeout(() => { + collapseContainer._transitionTimer = setTimeout(() => { collapseContainer.classList.remove('mud-collapse-entering'); collapseContainer.classList.add('mud-collapse-entered'); collapseContainer.style.height = 'auto'; + collapseContainer._transitionTimer = null; }, 250); } else { navElement.classList.remove('mud-nav-group-expanded'); const height = wrapper.scrollHeight; collapseContainer.style.height = height + 'px'; // Force reflow collapseContainer.offsetHeight; collapseContainer.classList.remove('mud-collapse-entered'); collapseContainer.classList.add('mud-collapse-exiting'); collapseContainer.style.height = '0px'; - setTimeout(() => { + collapseContainer._transitionTimer = setTimeout(() => { collapseContainer.classList.remove('mud-collapse-exiting'); collapseContainer.classList.add('invisible'); collapseContainer.style.display = ''; // Reset display collapseContainer.setAttribute('aria-hidden', 'true'); + collapseContainer._transitionTimer = null; }, 250); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wwwroot/Extensions.MudBlazor.StaticInput.lib.module.js` around lines 540 - 575, Before starting a new toggle transition, cancel any previous pending transition callback by storing the setTimeout id on the collapse container (e.g., collapseContainer._mudTransitionId) and calling clearTimeout if present; also remove transient transition classes (mud-collapse-entering, mud-collapse-exiting) and any stale inline height/aria/display state that could be reapplied, then proceed with the current logic that adds/removes mud-nav-group-expanded, mud-collapse-entering/mud-collapse-exiting, sets collapseContainer.style.height and schedules the new timeout—ensure both branches (willExpand true/false) set collapseContainer._mudTransitionId = setTimeout(...) so future toggles can clear it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/wwwroot/Extensions.MudBlazor.StaticInput.lib.module.js`:
- Around line 540-575: Before starting a new toggle transition, cancel any
previous pending transition callback by storing the setTimeout id on the
collapse container (e.g., collapseContainer._mudTransitionId) and calling
clearTimeout if present; also remove transient transition classes
(mud-collapse-entering, mud-collapse-exiting) and any stale inline
height/aria/display state that could be reapplied, then proceed with the current
logic that adds/removes mud-nav-group-expanded,
mud-collapse-entering/mud-collapse-exiting, sets collapseContainer.style.height
and schedules the new timeout—ensure both branches (willExpand true/false) set
collapseContainer._mudTransitionId = setTimeout(...) so future toggles can clear
it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56af86da-f94c-4263-b607-26dde63656d3
📒 Files selected for processing (1)
src/wwwroot/Extensions.MudBlazor.StaticInput.lib.module.js
…g issues - Fixed visibility of NavGroup items by explicitly managing 'invisible' class and 'display' style. - Implemented smooth height transitions using scrollHeight for expansion and collapse. - Added robust timeout management to prevent race conditions during rapid clicks. - Addressed persistent highlight issue by using 'button.blur()' and optimizing class usage. - Introduced WeakSet-based element tracking in JS initializer to support enhanced navigation. - Updated unit tests to support animation timing and state changes. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/wwwroot/Extensions.MudBlazor.StaticInput.lib.module.js (1)
547-547: Extract the nav-group animation duration into one constant.
250msis duplicated in the transition string and both timeout paths here, and the tests already wait on a different value intests/StaticInput.UnitTests/Components/NavMenuTests.cs:30-43. Centralizing it will keep the JS behavior and test timing from drifting again.♻️ Proposed refactor
+const NAV_GROUP_ANIMATION_MS = 250; +const NAV_GROUP_ANIMATION_EASING = 'cubic-bezier(0.4, 0, 0.2, 1)'; + function initNavGroups() { const navGroups = document.querySelectorAll('[data-mud-static-type="nav-group"]'); navGroups.forEach(navGroup => { @@ - collapseContainer.style.transition = 'height 250ms cubic-bezier(0.4, 0, 0.2, 1)'; + collapseContainer.style.transition = `height ${NAV_GROUP_ANIMATION_MS}ms ${NAV_GROUP_ANIMATION_EASING}`; @@ - }, 250); + }, NAV_GROUP_ANIMATION_MS); } else { @@ - }, 250); + }, NAV_GROUP_ANIMATION_MS); }Also applies to: 559-564, 575-581
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wwwroot/Extensions.MudBlazor.StaticInput.lib.module.js` at line 547, Extract the duplicated "250ms" into a single constant (e.g. NAV_GROUP_ANIMATION_DURATION or NAV_GROUP_ANIMATION_MS) and use that constant wherever the nav-group animation duration appears: in the collapseContainer.style.transition assignment (compose the transition string using the constant) and in the setTimeout calls that currently use numeric timeouts; replace both string and numeric occurrences so the transition string and timeout values are driven by the same constant, and update the three affected blocks around the existing collapse/expand logic (the collapseContainer.style.transition usage and the two timeout branches) to reference that constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/wwwroot/Extensions.MudBlazor.StaticInput.lib.module.js`:
- Around line 587-588: The code currently calls button.blur() unconditionally
after activation which steals focus from keyboard users; remove the
unconditional call to button.blur() and instead only remove focus for pointer
interactions — e.g., detect pointer-initiated activation by checking
event.pointerType (mouse/touch) or track a pointerdown flag set on pointerdown
and cleared on keydown, then call button.blur() only when the activation was
pointer-driven; if detecting pointer input isn't feasible, resolve the
persistent hover highlight by toggling a CSS class on the button rather than
blurring it. Ensure you update the click/activation handler that currently
invokes button.blur() so it uses the pointer-only check or CSS class approach.
---
Nitpick comments:
In `@src/wwwroot/Extensions.MudBlazor.StaticInput.lib.module.js`:
- Line 547: Extract the duplicated "250ms" into a single constant (e.g.
NAV_GROUP_ANIMATION_DURATION or NAV_GROUP_ANIMATION_MS) and use that constant
wherever the nav-group animation duration appears: in the
collapseContainer.style.transition assignment (compose the transition string
using the constant) and in the setTimeout calls that currently use numeric
timeouts; replace both string and numeric occurrences so the transition string
and timeout values are driven by the same constant, and update the three
affected blocks around the existing collapse/expand logic (the
collapseContainer.style.transition usage and the two timeout branches) to
reference that constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 20e5d8c3-61b8-45c8-ab89-4aa8b46e70e4
📒 Files selected for processing (2)
src/wwwroot/Extensions.MudBlazor.StaticInput.lib.module.jstests/StaticInput.UnitTests/Components/NavMenuTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/StaticInput.UnitTests/Components/NavMenuTests.cs
- Fixed visibility of NavGroup items by managing 'invisible' class and 'display' style. - Implemented smooth, reliable height transitions (250ms) using scrollHeight. - Guaranteed smooth first-time expansion by forcing reflow from height 0px. - Resolved persistent background highlight by using 'button.blur()' and avoiding 'mud-nav-group-expanded'. - Added robust timeout management and WeakSet tracking to prevent state corruption and double-initialization. - Updated unit tests to match new animation timings. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/wwwroot/Extensions.MudBlazor.StaticInput.lib.module.js (1)
592-593:⚠️ Potential issue | 🟠 MajorUnconditional
blur()breaks keyboard navigation.This call removes focus after every activation, causing keyboard users to lose their position in the nav menu. They cannot immediately re-toggle the same group without re-navigating to it.
Consider detecting the interaction type and only blurring for pointer events:
🔧 Suggested approach
- button.addEventListener('click', (event) => { + let lastInteractionWasPointer = false; + button.addEventListener('pointerdown', () => { + lastInteractionWasPointer = true; + }); + button.addEventListener('keydown', () => { + lastInteractionWasPointer = false; + }); + button.addEventListener('click', (event) => {Then at the end of the click handler:
- // Remove focus to fix the persistent "hover" highlight issue - button.blur(); + // Only blur for pointer interactions to fix hover highlight + // while preserving keyboard navigation + if (lastInteractionWasPointer) { + button.blur(); + lastInteractionWasPointer = false; + }Alternatively, resolve the persistent highlight via CSS using
:focus-visibleinstead of:focusstyling, which inherently distinguishes keyboard from pointer focus.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wwwroot/Extensions.MudBlazor.StaticInput.lib.module.js` around lines 592 - 593, The unconditional call to button.blur() inside the click handler removes keyboard focus and breaks keyboard navigation; change the handler so it only blurs for pointer interactions (e.g., check the event type or event.pointerType / event.detail to detect mouse/touch) and call button.blur() only when a pointer initiated the activation, leaving keyboard activations focused, or alternatively remove JavaScript blur and fix the visual by switching CSS focus styles to :focus-visible instead of :focus; locate the click handler that invokes button.blur() and implement the conditional logic around that call (or remove it and update CSS).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/wwwroot/Extensions.MudBlazor.StaticInput.lib.module.js`:
- Around line 592-593: The unconditional call to button.blur() inside the click
handler removes keyboard focus and breaks keyboard navigation; change the
handler so it only blurs for pointer interactions (e.g., check the event type or
event.pointerType / event.detail to detect mouse/touch) and call button.blur()
only when a pointer initiated the activation, leaving keyboard activations
focused, or alternatively remove JavaScript blur and fix the visual by switching
CSS focus styles to :focus-visible instead of :focus; locate the click handler
that invokes button.blur() and implement the conditional logic around that call
(or remove it and update CSS).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a544da74-7424-4bd6-934e-0b7629d64c53
📒 Files selected for processing (1)
src/wwwroot/Extensions.MudBlazor.StaticInput.lib.module.js
Anu6is
deleted the
fix/nav-group-visibility-and-animation-16877831955550158464
branch
1 participant
This change fixes a bug in
MudStaticNavGroupwhere nested items remained invisible after expanding the group from an initially collapsed state in Static SSR.The fix involves:
initNavGroupsinExtensions.MudBlazor.StaticInput.lib.module.jsto explicitly remove theinvisibleclass upon expansion and re-add it upon collapse.scrollHeightof the content wrapper and a 250ms CSS transition, mimicking standard MudBlazor behavior without requiring an interactive lifecycle.NavMenuTests.csto include necessary delays for these animations and verify the presence/absence of theinvisibleclass.Visual verification was performed using the TestViewer project, confirming that items appear correctly and the expansion is smooth.
PR created automatically by Jules for task 16877831955550158464 started by @Anu6is
Summary by CodeRabbit
New Features
Bug Fixes
Tests