Skip to content

fix(react-drawer): tolerate fractional scroll metrics when detecting scroll bottom#36333

Open
Aidam1 wants to merge 5 commits into
microsoft:masterfrom
Aidam1:fix/36331-drawer-body-scroll-bottom-tolerance
Open

fix(react-drawer): tolerate fractional scroll metrics when detecting scroll bottom#36333
Aidam1 wants to merge 5 commits into
microsoft:masterfrom
Aidam1:fix/36331-drawer-body-scroll-bottom-tolerance

Conversation

@Aidam1

@Aidam1 Aidam1 commented Jun 23, 2026

Copy link
Copy Markdown

Previous Behavior

DrawerBody determines its scroll position (top / middle / bottom) in
getScrollState using an exact equality check:

if (scrollTop + clientHeight === scrollHeight) {
  return 'bottom';
}

At browser zoom levels above 100% (and on displays with fractional scaling), scrollTop, clientHeight, and scrollHeight can resolve to sub-pixel values, so scrollTop + clientHeight never exactly equals scrollHeight. The body then stays in the middle state even when fully scrolled to the bottom, which causes the separator between DrawerBody/NavDrawerBody and the footer to still be visible unexpectedly when the user reaches the end of the content.

New Behavior

The bottom check now allows a 1px tolerance so fractional scroll metrics still resolve to bottom. The separator now stays hidden when scrolled to the bottom, including at zoom levels of 110% and higher.

Related Issue(s)

@Aidam1

Aidam1 commented Jun 23, 2026

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

@Aidam1 Aidam1 marked this pull request as ready for review June 23, 2026 08:50
@Aidam1 Aidam1 requested review from a team and mainframev as code owners June 23, 2026 08:50
@tudorpopams tudorpopams requested a review from PaulGMardling July 7, 2026 12:04
Comment on lines +19 to +21
// Allow a 1px tolerance so fractional scroll metrics (browser zoom, display scaling)
// still resolve to 'bottom' instead of getting stuck at 'middle'
const SCROLL_BOTTOM_TOLERANCE = 1;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could we switch this to a short JSDoc on the constant instead of inline comments?
Since this is really documenting the meaning of SCROLL_BOTTOM_TOLERANCE, it feels easier to scan and maintain if that context lives with the constant itself.

I also think the wording could be a bit more explicit about the rationale.
Right now it explains that scroll metrics can be fractional under zoom/display scaling, but not quite that the tolerance is there to treat values that are visually at the bottom as bottom even when the measurements do not sum exactly.

Suggested change
// Allow a 1px tolerance so fractional scroll metrics (browser zoom, display scaling)
// still resolve to 'bottom' instead of getting stuck at 'middle'
const SCROLL_BOTTOM_TOLERANCE = 1;
/**
* Treat values within 1px of the scroll height as "bottom" to account for
* fractional scroll measurements caused by browser zoom and display scaling.
*/
const SCROLL_BOTTOM_TOLERANCE = 1;

}

if (scrollTop + clientHeight === scrollHeight) {
if (scrollTop + clientHeight >= scrollHeight - SCROLL_BOTTOM_TOLERANCE) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This makes the bottom-state check a bit easier to read and reason about by naming the remaining scroll distance explicitly. It also keeps the tolerance comparison focused on the thing we care about here: how far the viewport is from the bottom edge.

Suggested change
if (scrollTop + clientHeight >= scrollHeight - SCROLL_BOTTOM_TOLERANCE) {
const distanceFromBottom = scrollHeight - (scrollTop + clientHeight);
if (distanceFromBottom <= SCROLL_BOTTOM_TOLERANCE) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

we can also round the value to a nearest integer, something like:

Math.round(scrollTop + clientHeight) === scrollHeight

But both options should work

@PaulGMardling PaulGMardling left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You’ve done a great job tracking down the root cause here and landing on a more robust fix for the bottom scroll detection.

I had a couple of small non-blocking suggestions to tighten up the final shape a bit, mostly around readability and making the intent a little clearer for future readers.

I added some unit test coverage on to help lock in the regression scenario, but overall this looks good to me.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: NavDrawer shows divider above Footer when scrolled to the bottom

3 participants