Breadcrumbs: fix clipped-item reveal and avoid redundant aligned scroll updates

[Copilot]

Description

In narrow editor groups, breadcrumb items could remain partially visible when their left edge was inside the viewport but their right edge was clipped. This change fixes reveal calculation at the source and now also avoids redundant scroll updates when the computed reveal target is already aligned with the current scroll position.

• Behavior change

  • BreadcrumbsWidget._reveal computes minimal horizontal scrolling for both clipping cases:
    • item is left-clipped (itemOffsetLeft < scrollLeft)
    • item is right-clipped (itemOffsetLeft + itemWidth > scrollLeft + width)
  • _select uses minimal reveal behavior (_reveal(selectedIndex, true)) to avoid click-induced left-edge pinning regressions.
  • Existing non-minimal semantics are preserved where explicitly requested (minimal === false still targets the item's left edge).
  • getBreadcrumbScrollLeft(...) now returns undefined when the computed target equals the current scrollLeft, so _reveal skips redundant setScrollPosition / setRevealOnScroll churn.

• Scope

  • Fix is centralized in reveal logic and applies consistently to focus, selection, keyboard navigation, and click paths that use minimal reveal.
  • Reveal math is explicit and unit-testable via getBreadcrumbScrollLeft(...).

• Coverage

  • Focused unit tests validate reveal math without private-field stubbing:
    • minimal reveal for left-clipped item
    • minimal reveal for right-clipped item
    • no scroll when fully visible
    • non-minimal mode pins to left edge
    • non-minimal mode returns no-op when already aligned

const newScrollLeft = getBreadcrumbScrollLeft(
	scrollLeft,
	width,
	node.offsetLeft,
	node.offsetWidth,
	minimal
);

Testing

• npm run transpile-client
• node test/unit/browser/index.js --run src/vs/base/test/browser/ui/breadcrumbs/breadcrumbsWidget.test.ts --browser chromium

Fixes #187463
Fixes #168824

[dmitrivMS]

@copilot See my comment above

[Copilot]

Review

I don't think this is the right fix. A few concerns, ordered roughly by severity:...

Addressed in cafaf5a.

I replaced the selection-specific change with a root-cause fix in _reveal: minimal reveal now handles both left-clipped and right-clipped items. _select now uses minimal reveal to avoid click-induced left-edge pinning regressions.

I also rewrote the tests to avoid private-field fakes and now cover left-clipped, right-clipped, fully-visible (no scroll), plus non-minimal behavior via getBreadcrumbScrollLeft unit tests.

[dmitrivMS]

Updated review — much better, with one suggested addition

The new revision addresses essentially every concern from my previous comment:

• ✅ Right-clipped case is now handled correctly — itemEnd - width is the proper minimal scroll, not "pin to left edge".
• ✅ _select switched to _reveal(_, true) — kills the click regression where every click pinned the breadcrumb to the left edge.
• ✅ Non-minimal semantics preserved for revealLast() / reveal().
• ✅ Tests are now pure unit tests of getBreadcrumbScrollLeft — no (widget as any) stubbing, no Object.defineProperty on layout properties, and they cover the four meaningful cases (left-clipped, right-clipped, fully visible no-op, non-minimal).

Couple of small notes on this PR by itself:

1. The reveal call newly added in _select is mostly redundant on the click path (_onClick calls _focus first, which already reveals). But it's harmless now that it's minimal and idempotent — and it usefully covers programmatic setSelection callers that don't call setFocused first (e.g. _onFocusEvent in breadcrumbsControl.ts). Worth a one-line comment explaining the latter, but not blocking.
2. getBreadcrumbScrollLeft is exported only for testing. Either inline a @internal/// exported for tests comment, or — since it's a pure function — move it to a non-exported helper and test via a small re-export module if you want to avoid widening the public surface of breadcrumbsWidget.ts. Minor.

---

Relationship to #315384

#315384 takes a different angle: it's a perf fix — early-return from _reveal when node.offsetLeft === scrollLeft to avoid redundant setScrollPosition/setRevealOnScroll toggles on hot paths (editor open/update via revealLast()).

They conflict textually — both edit the same _reveal body — and neither one fully subsumes the other:

Case	After only 315385	After only 315384
Right-clipped item, minimal	✅ correctly scrolls minimum amount	❌ still does nothing (original bug)
Fully-visible item, minimal	✅ no-op (helper returns undefined)	❌ still toggles setRevealOnScroll and calls setScrollPosition (only no-ops if scrollLeft happens to equal offsetLeft)
revealLast() when last item already at scrollLeft	❌ still calls setScrollPosition/setRevealOnScroll (helper returns itemOffsetLeft, not undefined)	✅ early-returns

So #315384's perf benefit is real and not covered by #315385 alone — specifically for the revealLast() / non-minimal path.

Recommendation: take #315385 as the base, fold in #315384's optimization, close #315384

The cleanest combined shape is to extend getBreadcrumbScrollLeft to also return undefined when the computed scroll position equals the current one. That generalizes #315384's check (it covers minimal and non-minimal, and any reveal path) while keeping all the logic in one pure, unit-testable function:

export function getBreadcrumbScrollLeft(scrollLeft: number, width: number, itemOffsetLeft: number, itemWidth: number, minimal: boolean): number | undefined {
    const itemEnd = itemOffsetLeft + itemWidth;

    let target: number;
    if (!minimal) {
        target = itemOffsetLeft;
    } else if (itemOffsetLeft < scrollLeft) {
        target = itemOffsetLeft;
    } else if (itemEnd > scrollLeft + width) {
        target = itemEnd - width;
    } else {
        return undefined; // already fully visible
    }

    return target === scrollLeft ? undefined : target; // no-op if already aligned
}

And add one more unit test mirroring #315384's intent: getBreadcrumbScrollLeft(40, 100, 40, 10, false) === undefined.

Drop #315384's test entirely — it has the same (widget as unknown as {...})._scrollable = … private-field stubbing pattern that I flagged on the original revision of this PR; the helper-level test covers the same intent without it.

Conflict logistics

Whichever PR lands second will need a trivial rebase against the other. Combining into one PR (this one) is cleaner: single _reveal rewrite, single set of tests, single review.

[dmitrivMS]

@copilot See my comment above

[Copilot]

Updated review — much better, with one suggested addition

The new revision addresses essentially every concern from my previous comment:...

Addressed in 195c069. I updated getBreadcrumbScrollLeft to return undefined when the computed target equals the current scrollLeft, and added the helper unit test non-minimal reveal does not scroll when item is already aligned.

[dmitrivMS]

Latest revision (195c0696a) looks good — getBreadcrumbScrollLeft now returns undefined when target === scrollLeft, which subsumes #315384's optimization across all reveal paths (including revealLast()), and the new test covers it. No further changes requested from me.

This PR now supersedes #315384 — closing that one in favor of this. The UX concern from #168824 is preserved: revealLast() still calls _reveal(_, false), which still targets itemOffsetLeft when not already aligned.

[Copilot]

Pull request overview

Fixes breadcrumbs horizontal reveal behavior so items are fully revealed when partially clipped (especially right-clipped cases in narrow editor groups), and avoids redundant scroll updates when the reveal target is already aligned.

Changes:

• Introduces getBreadcrumbScrollLeft(...) to compute minimal vs non-minimal reveal targets and return undefined for no-op scrolls.
• Updates BreadcrumbsWidget reveal/select paths to use minimal reveal by default and skip redundant scroll position churn.
• Adds focused unit tests covering left/right clipping, fully visible, and aligned no-op cases.

Show a summary per file

File	Description
src/vs/base/browser/ui/breadcrumbs/breadcrumbsWidget.ts	Centralizes reveal math in getBreadcrumbScrollLeft and updates reveal/selection to use minimal scrolling and avoid redundant scroll updates.
src/vs/base/test/browser/ui/breadcrumbs/breadcrumbsWidget.test.ts	Adds unit tests validating reveal target calculations for clipped/visible/aligned scenarios.

Copilot's findings

• Files reviewed: 2/2 changed files
• Comments generated: 1