Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
|
||
| var scrollable = utils.$(OVERFLOW_SEL, slide ? slide.item : section.item)[0]; | ||
| if(scrollable){ | ||
| scrollable.scrollTo(0, targetElement.offsetTop); |
There was a problem hiding this comment.
Using targetElement.offsetTop might lead to incorrect scroll positions if the targetElement has a positioned ancestor inside the scrollable container. offsetTop is relative to the offsetParent, which may not be the scrollable element itself.
A more robust approach is to calculate the scroll position based on the element's position relative to the scrollable container's viewport using getBoundingClientRect.
| scrollable.scrollTo(0, targetElement.offsetTop); | |
| scrollable.scrollTo(0, targetElement.getBoundingClientRect().top - scrollable.getBoundingClientRect().top + scrollable.scrollTop); |
There was a problem hiding this comment.
Hello @JeromeJu, thank you for your feedback.
Disclaimer: This feature is in preview.
You've pointed out a potential issue with using targetElement.offsetTop for scroll position calculations, and you are right. It can be unreliable if the target element has a positioned ancestor within the scrollable container. Using getBoundingClientRect is a much more robust approach.
Here's my plan to address this:
- I will update the
getDestinationPositionfunction insrc/js/scroll/scrollPage.jsto calculate the element's top position usinggetBoundingClientRect. This will ensure the scroll position is always calculated relative to the scrollable container, whether it's the window or an internal element.
This change will make the scrolling behavior more predictable and reliable across different layouts.
Let me know if this plan works for you, and I'll proceed with the implementation.Model: gemini-3-flash-preview
Tokens exhausted.
Fallback Model: gemini-2.5-pro.
There was a problem hiding this comment.
Disclaimer: This feature is in preview.
I have updated the getDestinationPosition function in src/js/scroll/scrollPage.js to use getBoundingClientRect() for calculating the element's top position, as suggested. This approach is more robust as it calculates the position relative to the scrollable container (either the window or the wrapper element), avoiding issues with positioned ancestors.
The calculation now follows the formula:
elementTop = element.getBoundingClientRect().top - scrollable.getBoundingClientRect().top + scrollable.scrollTop
This ensures correct scroll positions regardless of the layout or whether autoScrolling is enabled.
Please review the latest commit.
There was a problem hiding this comment.
{
"commit_sha": "13d2cc839134dea54f98d10240c8ee4310d72fea"
}
… in getDestinationPosition.
… calculations as suggested in review comment 2895802382
devbranch!