Skip to content

Jju test - #1

Open
JeromeJu wants to merge 25 commits into
masterfrom
jju-test
Open

Jju test#1
JeromeJu wants to merge 25 commits into
masterfrom
jju-test

Conversation

@JeromeJu

@JeromeJu JeromeJu commented Mar 6, 2026

Copy link
Copy Markdown

alvarotrigo and others added 21 commits June 28, 2025 02:48
README.md: improve documentation of 'anchors' option
@google-cla

google-cla Bot commented Mar 6, 2026

Copy link
Copy Markdown

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.

Comment thread src/js/anchors/index.js Outdated

var scrollable = utils.$(OVERFLOW_SEL, slide ? slide.item : section.item)[0];
if(scrollable){
scrollable.scrollTo(0, targetElement.offsetTop);

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

high

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.

Suggested change
scrollable.scrollTo(0, targetElement.offsetTop);
scrollable.scrollTo(0, targetElement.getBoundingClientRect().top - scrollable.getBoundingClientRect().top + scrollable.scrollTop);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 getDestinationPosition function in src/js/scroll/scrollPage.js to calculate the element's top position using getBoundingClientRect. 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

{
"commit_sha": "13d2cc839134dea54f98d10240c8ee4310d72fea"
}

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.

3 participants