Scroll the element that actually scrolls (issue #61) - #80
Merged
Conversation
--scroll called window.scrollBy, which moves nothing on a site whose body is fixed to the viewport height with the document inside an inner container. That is how Feishu, Notion, Linear and most dashboards are built, so --scroll did nothing at all on them and the lazy-loaded content was never captured. The loop also stopped once the distance travelled reached document.body.scrollHeight. On an app shell that is a single viewport, so it exited after one step, and even on an ordinary page it compared a running total of intended steps against a height that grows as content loads. kage now picks the largest genuinely scrollable element, reads scrollTop back after each step rather than assuming the step landed, and keeps going until both the position and the height stop changing. The scroll is bounded by half the render timeout, at least five seconds, so a page that appends content forever cannot hold a worker. The app-shell test fails before this change and the window-scrolled one passes, so the ordinary case is covered too.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the
--scrollhalf of #61.autoScrollcalledwindow.scrollBy. On a site whose body is pinned to the viewport height with the document inside an inneroverflow-y:autocontainer, the window has nothing to scroll, so--scrolldid nothing at all. Feishu is built that way, and so are Notion, Linear and most dashboards.The exit condition was wrong too. It summed the steps it intended to take and stopped once that total reached
document.body.scrollHeight. On an app shell that height is one viewport, so the loop gave up after a single step. On an ordinary page it compared intended distance against a height that grows as content loads, and it never checked whether a step actually moved anything.What it does now:
overflow-y, falling back todocument.scrollingElement. That is the window on a normal page and the inner container on an app shell.scrollTopback after each step instead of assuming the step landed, and steps by 80 percent of the container height rather than a fixed 800px.--render-timeoutraises it.Tests are in
browser/pool_test.go.TestAutoScrollTriggersLazyContentserves two pages, an app shell and an ordinary tall page, each appending a marker element on scroll, and asserts the marker is in the saved HTML. The app-shell case fails on main and the window case passes, so the ordinary path is covered as a regression.TestAutoScrollLeavesAStaticPageAlonechecks that a page with nothing to scroll returns quickly rather than burning its budget, andTestScrollBudgetcovers the bound. The Chrome-driven ones skip under-shortand when no Chrome is found, as the others do.Not in scope: the virtualised editor case, where a document only ever holds a window of nodes in the DOM and scrolling past them destroys what came before. Capturing that needs incremental DOM collection during the scroll rather than one serialisation at the end, and it is worth its own issue. This change is what makes lazy loading work at all on that class of site.