-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core(gather-runner): always reset scroll position #8625
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like a good idea? I don't see strong downsides. Is scrollTo
a no-op if scrolling to where the page already is?
@@ -319,6 +319,10 @@ class GatherRunner { | |||
await driver.setThrottling(passContext.settings, {useThrottling: false}); | |||
log.time(apStatus, 'verbose'); | |||
|
|||
// Some gatherers scroll the page which can cause unexpected results for other gatherers. | |||
// We reset the scroll position in between each gatherer. | |||
const scrollPosition = pageLoadError ? null : await driver.getScrollPosition(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pageLoadError
control flow is seriously the worst :S
Co-Authored-By: patrickhulce <patrick.hulce@gmail.com>
let's land this after we ship 5.0.0 |
It is now post-5.0.0, reviews welcome :) |
yeah, no, bad merge, bad :) |
This reverts commit 6bc4f89.
Summary
Came up a few times now, but we should give gatherers a consistent view of the page. This PR saves the scroll position of the page after we're done loading and scrolls back to it after every gatherer.
Related Issues/PRs
#7778 (comment)