Skip to content

feat(Menu): improve scrollIntoView performance #7423

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.

Sign up for GitHub

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

Merged
ArgoZhang merged 5 commits into from
Dec 26, 2025
Merged

feat(Menu): improve scrollIntoView performance #7423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ce8bf19
refactor: 增强滚动视口逻辑
ArgoZhang Dec 26, 2025
eaf1fb3
refactor: 增强 dispose 逻辑
ArgoZhang Dec 26, 2025
f531c0d
refactor: 优化代码消除提示信息
ArgoZhang Dec 26, 2025
89af254
doc: 更新锚点滚动逻辑
ArgoZhang Dec 26, 2025
925a92b
refactor: 重构代码
ArgoZhang Dec 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/BootstrapBlazor.Server/Components/Layout/ComponentLayout.razor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
export function scrollToAnchor() {
export function scrollToAnchor() {
const hash = decodeURI(location.hash)
if (hash) {
const anchor = hash.split('-')[0]
const el = document.querySelector(anchor)
if (el) {
const handler = setTimeout(() => {
el.scrollIntoView({ behavior: 'smooth', block: 'start' })
clearTimeout(handler)
el.scrollIntoView({ behavior: 'smooth', block: 'start' })
Comment on lines 8 to +9
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

The clearTimeout is called before the scrollIntoView operation. If there's an error during scrollIntoView, the timeout would have already been cleared, potentially masking the issue. Consider moving the clearTimeout call after the scrollIntoView operation completes.

Suggested change
clearTimeout(handler)
el.scrollIntoView({ behavior: 'smooth', block: 'start' })
try {
el.scrollIntoView({ behavior: 'smooth', block: 'start' })
} finally {
clearTimeout(handler)
}

Copilot uses AI. Check for mistakes.
}, 1000)
}
}
Expand Down
23 changes: 11 additions & 12 deletions src/BootstrapBlazor.Server/Components/Layout/NavMenu.razor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,19 @@
import Data from "../../_content/BootstrapBlazor/modules/data.js"
import Data from "../../_content/BootstrapBlazor/modules/data.js"
import EventHandler from "../../_content/BootstrapBlazor/modules/event-handler.js"

export function init(id) {
const navmenu = {
navbar: document.querySelector('.navbar-toggler'),
menu: document.querySelector('.sidebar-content')
}
Data.set(id, navmenu)
const navbar = document.querySelector('.navbar-toggler');
const menu = document.querySelector('.sidebar-content');
Data.set(id, { navbar, menu });

EventHandler.on(navmenu.navbar, 'click', () => {
navmenu.menu.classList.toggle('show')
EventHandler.on(navbar, 'click', () => {
menu.classList.toggle('show')
})
EventHandler.on(navmenu.menu, 'click', '.nav-link', e => {
EventHandler.on(menu, 'click', '.nav-link', e => {
const link = e.delegateTarget
const url = link.getAttribute('href');
if (url !== '#') {
navmenu.menu.classList.remove('show')
menu.classList.remove('show')
}
})
}
Expand All @@ -24,6 +22,7 @@ export function dispose(id) {
const data = Data.get(id);
Data.remove(id);

EventHandler.off(data.navbar, 'click');
EventHandler.off(data.menu, 'click', '.nav-link');
const { navbar, menu } = data;
EventHandler.off(navbar, 'click');
EventHandler.off(menu, 'click', '.nav-link');
}
6 changes: 3 additions & 3 deletions src/BootstrapBlazor/Components/Menu/Menu.razor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Licensed to the .NET Foundation under one or more agreements.
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the Apache 2.0 License
// See the LICENSE file in the project root for more information.
// Maintainer: Argo Zhang(argo@live.ca) Website: https://www.blazor.zone
Expand Down Expand Up @@ -144,11 +144,11 @@ private string GetUrl()
var url = Navigator.ToBaseRelativePath(Navigator.Uri);
if (url.Contains('?'))
{
url = url[..url.IndexOf("?")];
url = url[..url.IndexOf('?')];
}
if (url.Contains('#'))
{
url = url[..url.IndexOf("#")];
url = url[..url.IndexOf('#')];
}
return url;
}
Expand Down
37 changes: 26 additions & 11 deletions src/BootstrapBlazor/Components/Menu/Menu.razor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { getTargetElement, getTransitionDelayDurationFromElement } from "../../modules/utility.js"
import { getTargetElement, getTransitionDelayDurationFromElement } from "../../modules/utility.js"
import Data from "../../modules/data.js"

export function init(id) {
Expand Down Expand Up @@ -48,7 +48,7 @@ export function init(id) {
}
Data.set(id, menu)

scrollElementToView(menu.element)
scrollElementToView(menu)
}

export function update(id) {
Expand All @@ -74,19 +74,24 @@ export function update(id) {
}
});

scrollElementToView(menu.element)
scrollElementToView(menu)
}

export function scrollToView(id) {
const menu = Data.get(id)
if (menu) {
scrollElementToView(menu.element);
scrollElementToView(menu);
}
}

export function dispose(id) {
const menu = Data.get(id)
Data.remove(id)
Data.remove(id);

if (menu.handler) {
clearInterval(menu.handler);
menu.handler = null;
}

menu.collapses.forEach(el => {
const target = getTargetElement(el)
Expand All @@ -97,13 +102,23 @@ export function dispose(id) {
})
}

const scrollElementToView = element => {
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot Dec 26, 2025

Choose a reason for hiding this comment

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

issue (complexity): Consider refactoring the scroll retry logic into an element-based helper with its own timer management instead of storing timer state on the menu object.

You can keep the new “wait for .nav-link.active” behavior while reducing coupling and side effects by:

  • Making scrollElementToView operate on the element again.
  • Managing the retry state and timer locally (or in a small helper) instead of mutating menu.
  • Centralizing timer cleanup in one place.

One concrete option:

// module-scope map to track timers per menu element
const scrollTimers = new Map();

const clearScrollTimer = (element) => {
  const handle = scrollTimers.get(element);
  if (handle) {
    clearTimeout(handle);
    scrollTimers.delete(element);
  }
};

const scrollElementToView = (element) => {
  if (!element.hasAttribute('data-bb-scroll-view')) {
    return;
  }

  clearScrollTimer(element); // avoid multiple concurrent timers

  let count = 0;
  const tryScroll = () => {
    const links = [...element.querySelectorAll('.nav-link.active')];
    if (links.length > 0) {
      scrollTimers.delete(element);
      const link = links[links.length - 1];
      link.scrollIntoView({ behavior: 'smooth', block: 'center', inline: 'start' });
      return;
    }

    if (++count < 3) {
      const handle = setTimeout(tryScroll, 100);
      scrollTimers.set(element, handle);
    } else {
      scrollTimers.delete(element);
    }
  };

  const handle = setTimeout(tryScroll, 100);
  scrollTimers.set(element, handle);
};

Call sites stay simple and decoupled from timer state:

// init/update/scrollToView
scrollElementToView(menu.element);

And dispose only needs to clear the timer for this element (no menu.handler property):

export function dispose(id) {
  const menu = Data.get(id);
  Data.remove(id);

  clearScrollTimer(menu.element);

  menu.collapses.forEach(el => {
    const target = getTargetElement(el);
    const collapse = bootstrap.Collapse.getInstance(target);
    if (collapse !== null) {
      menu.disposeCollapse(collapse, el);
    }
  });
}

This preserves the retry behavior and cleanup while:

  • Removing the handler field from menu.
  • Keeping scrollElementToView element-focused and side-effect-limited.
  • Localizing timer logic and cleanup to a small helper, which is easier to reason about.

const scrollElementToView = menu => {
const { element } = menu;
const scroll = element.hasAttribute('data-bb-scroll-view')
if (scroll) {
var links = [...element.querySelectorAll('.nav-link.active')]
if (links.length > 0) {
var link = links.pop()
link.scrollIntoView({ behavior: 'smooth', block: 'center', inline: 'start' });
}
let count = 0;
menu.handler = setInterval(() => {
if (count < 3) {
var links = [...element.querySelectorAll('.nav-link.active')]
if (links.length > 0) {
clearInterval(menu.handler);
menu.handler = null;

var link = links.pop()
link.scrollIntoView({ behavior: 'smooth', block: 'center', inline: 'start' });
}
Comment on lines +110 to +119
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot Dec 26, 2025

Choose a reason for hiding this comment

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

issue (bug_risk): Interval is never cleared when no active links are found, causing a runaway timer.

Because the interval is only cleared when .nav-link.active exists, if none ever appear the timer runs indefinitely and count grows unbounded. Add a guard to clear the interval once count reaches the max number of attempts (e.g., count >= 3) to prevent a perpetual timer and wasted work.

Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

The interval handler is not cleared when the maximum number of attempts (count >= 3) is reached. This causes the interval to continue running indefinitely, wasting resources. Add cleanup logic to clear the interval and set handler to null when count reaches 3.

Suggested change
}
}
} else {
clearInterval(menu.handler);
menu.handler = null;

Copilot uses AI. Check for mistakes.
}
count++;
}, 100);
Comment on lines +110 to +122
Copy link

Copilot AI Dec 26, 2025

Choose a reason for hiding this comment

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

If scrollElementToView is called multiple times before the active links are found (e.g., during rapid updates), multiple intervals will be created and stored in menu.handler, but only the last one will be tracked. Previous intervals will become orphaned and continue running indefinitely. Consider clearing any existing handler before creating a new one.

Suggested change
menu.handler = setInterval(() => {
if (count < 3) {
var links = [...element.querySelectorAll('.nav-link.active')]
if (links.length > 0) {
clearInterval(menu.handler);
menu.handler = null;
var link = links.pop()
link.scrollIntoView({ behavior: 'smooth', block: 'center', inline: 'start' });
}
}
count++;
}, 100);
// Clear any existing interval before creating a new one to avoid orphaned handlers
if (menu.handler) {
clearInterval(menu.handler);
menu.handler = null;
}
const handler = setInterval(() => {
if (count < 3) {
var links = [...element.querySelectorAll('.nav-link.active')]
if (links.length > 0) {
clearInterval(handler);
if (menu.handler === handler) {
menu.handler = null;
}
var link = links.pop()
link.scrollIntoView({ behavior: 'smooth', block: 'center', inline: 'start' });
}
} else {
// Stop retrying after the maximum number of attempts
clearInterval(handler);
if (menu.handler === handler) {
menu.handler = null;
}
}
count++;
}, 100);
menu.handler = handler;

Copilot uses AI. Check for mistakes.
}
}
Loading