Skip to content
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 #9060

Merged
merged 4 commits into from
Jun 7, 2019

Conversation

patrickhulce
Copy link
Collaborator

@patrickhulce patrickhulce commented May 25, 2019

Reland of #8625 (reverted in #9059)

@patrickhulce patrickhulce changed the title Revert "misc(fix master): tmp revert of scroll position in #8625" core(gather-runner): always reset scroll position May 27, 2019
lighthouse-core/gather/gather-runner.js Outdated Show resolved Hide resolved
* @return {Promise<{x: number, y: number}>}
*/
getScrollPosition() {
return this.evaluateAsync(`({x: window.scrollX, y: window.scrollY})`, {useIsolation: true});
Copy link
Member

Choose a reason for hiding this comment

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

feels like the protocol should have something for this...Page.getLayoutMetrics?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not that I can find

image

image

Copy link
Member

Choose a reason for hiding this comment

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

looks like it's Page.getLayoutMetrics -> response.visualViewport.pageX and pageY, but it's possible in the end that it's worse perf than this evaluateAsync

*/
scrollTo(position) {
const scrollExpression = `window.scrollTo(${position.x}, ${position.y})`;
return this.evaluateAsync(scrollExpression, {useIsolation: true});
Copy link
Member

Choose a reason for hiding this comment

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

feels like the protocol should have somethign for this too, but I don't see anything

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You can synthesize a gesture but that's not what we want to do https://chromedevtools.github.io/devtools-protocol/tot/Input/#method-synthesizeScrollGesture

Copy link
Member

Choose a reason for hiding this comment

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

You can synthesize a gesture but that's not what we want to do

yeah, I meant it feels like there should be a method :)

Copy link
Member

Choose a reason for hiding this comment

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

apparently this is exactly what devtools does (e.g. "Scroll into view" in the devtools panel just remote evals scrollIntoViewIfNeeded(true) on the selected element) so we're in good company :)

return await this._evaluateInContext(expression, contextId);
} catch (err) {
// If we were using isolation and the context disappeared on us, retry one more time.
if (contextId && err.message.includes('Cannot find context')) {
Copy link
Member

Choose a reason for hiding this comment

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

is this still needed given the new event handling?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure what you mean by new event handling, but it's something we should do regardless. Our temporary context can be destroyed at any time and we used to see high volumes of this in sentry once upon a time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh do you just mean Runtime.executionContextDestroyed? It can still race if we're trying to eval this between when we receive the context destroyed event and we eval this, plus the aforementioned destroying of our context regardless of navigation :)

Copy link
Member

Choose a reason for hiding this comment

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

Oh do you just mean Runtime.executionContextDestroyed? It can still race

yeah, I guess it feels like we should be handling it the "correct way"™ or the "sloppily but effectively prevent race conditions way", but not both, but w/e :)

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

↕️ LGTM ↕️

@patrickhulce patrickhulce merged commit 0a28612 into master Jun 7, 2019
@patrickhulce patrickhulce deleted the revert-9059-revert-8625-reset_scroll_position branch June 7, 2019 01:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants