MudScrollListener: Fix document scroll#11978
Conversation
mudbot
bot
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR fixes document-level scroll position detection in the scroll listener by adding proper handling for when the scroll target is the document itself. Instead of trying to read scroll properties directly from the document object (which doesn't work), it now uses document.scrollingElement to get accurate scroll positions.
Key Changes:
- Added detection for document-level scroll events and proper scroll position retrieval using
document.scrollingElement - Changed error logging from
console.logtoconsole.errorfor better error visibility
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an issue with scroll position detection for full-document scrolling by checking if the event target is the document and using document.scrollingElement to retrieve accurate scroll values. The change to use console.error for logging errors is also a good improvement. I've included one suggestion to refactor the new logic to be more concise and to avoid redundant variable assignments, which aligns better with the repository's style guide. Overall, the changes are positive and address the intended problem.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
If you think it's solid I would just merge it |
mudbot
bot
added
bug
It's solid, simple, and shouldn't affect anything written on the public API. |
While I can't speak to the intentions when this was built it seems like it was created to capture page scroll in addition to Selector scroll. This change just adds in a check when the element is "document" and grabs the scroll position. Tested with the new Service docs before porting to it's own PR. It has literal 0 effect if someone was calculating it manually via firstChild (possible) but instead returns the proper scroll positions for the first scrollable element of a document.
Checklist:
dev).