GoogleChrome / lighthouse

Automated auditing, performance metrics, and best practices for the web.
https://developer.chrome.com/docs/lighthouse/overview/
Apache License 2.0
28.19k stars 9.34k forks source link

Prevent destructive behavior in user flows #14336

Open ChristopherPHolder opened 2 years ago

ChristopherPHolder commented 2 years ago

Summary

So I followed this example (more or less) https://web.dev/lighthouse-user-flows/#looking-for-feedback

What I am trying to do is navigate to a page, scroll to the bottom, and scroll back to the top. However, if I put the scroll to the bottom in a timespan at the end it resets to the top of the page. How do I avoid this?

    const url = new URL(targetUrl);
    const browser = await launch({headless: false});
    const page = await browser.newPage();
    const flow = await new UserFlow(page);

    const scrollAction = new ScrollAction(page);

    await flow.navigate(url.href, { stepName: "Cold Initial Navigation" });

     await flow.startTimespan({ stepName: 'Scroll To Bottom Of Page' } );
     await scrollAction.swipeToPageBottom();
     await flow.endTimespan();

     await flow.startTimespan({ stepName: 'Scroll To Top Of Page' });
     await scrollAction!.swipeToPageTop();
     await flow.endTimespan();

     const report = await flow.createFlowResult();
     await browser.close();

and the scroll action

 import { CDPSession, Page, Protocol } from "puppeteer";

export class ScrollAction {
  private session?: CDPSession;
  constructor(private page: Page) {}

  async swipeToPageBottom(): Promise<void> {
    await this.page.waitForSelector("body");
    await this._synthesizeTouchScroll('down');
  }

  async swipeToPageTop(): Promise<void> {
    await this.page.waitForSelector("body");
    await this._synthesizeTouchScroll('up');
  }

  private async _synthesizeTouchScroll(direction: 'down' | 'up'): Promise<void> {
    const scrollParams = await this._getScrollParams(direction);
    if (!this.session) {
      this.session = await this.page.target().createCDPSession();
    }
    // https://chromedevtools.github.io/devtools-protocol/tot/Input/#method-synthesizeScrollGesture
    await this.session.send('Input.synthesizeScrollGesture', scrollParams);
  }

  private async _getScrollParams(direction: 'down' | 'up'): Promise<Protocol.Input.SynthesizeScrollGestureRequest> {
    const innerHeight = await this._getInnerContentHeight();
    const x = 200;
    const y = direction === 'up' ? 200 : 600;
    const scrollDistance = 1500;
    const speed = 800;
    const repeatDelayMs = 250;
    const yDistance = direction === 'up' ? scrollDistance : scrollDistance * (-1);
    const repeatCount = Math.ceil(innerHeight/scrollDistance) -1;
    const gestureSourceType = 'touch';
    return {x, y, yDistance, speed, repeatCount, repeatDelayMs, gestureSourceType};
  }

  private async _getInnerContentHeight(): Promise<number> {
    const innerContentHandle = await this.page.$("body");
    const innerContentBox = await innerContentHandle!.boundingBox(); 
    return innerContentBox!.height;
  }
}

Now what is interesting is that if I remove the timestamp it works fine. If I don't, its scrolls to the bottom and then appears at the top and tries to scroll from there.

What am I doing wrong or is it a config that I am missing? Thanks so much in advance :)

adamraine commented 2 years ago

At the end of every Lighthouse run, Lighthouse takes a screenshot of the entire page content, including content outside the normal viewport. To do this we resize the viewport to include everything and this scrolls to the top of the page.

It was designed to work with a standalone navigation mode, where the page state after the screenshot wasn't our concern. However you bring up a good example of how the screenshot (at least in its current form) can be obstructive to user flows.

We should discuss changing the screenshot for user flows.

In the meantime, you can disable the screenshot by using Lighthouse settings in the config:

const config = {
  extends: 'lighthouse:default',
  settings: {
    skipAudits: ['full-page-screenshot'],
  }
}

const browser = await puppeteer.launch();
const page = await browser.newPage();
const flow = await startFlow(page, {config});

The downside to this is that elements listed in the report won't have a visual callout.

ChristopherPHolder commented 2 years ago

Cool, will test out the solution now. I am hoping the additional config can be set to only be used in the timespan but will be happy ether way, as it is intended to be used in a cloud function its probably better anyway that I reduce the memory consumption. :)

By the way, I see every tutorial or demo using:

const flow = await startFlow(page);

instead of this:

const flow = await new UserFlow(page);

Is there a reason for that? Seems to me that it would be more logical

ChristopherPHolder commented 2 years ago

So I tested it out and it works like a charm, actually better cause it did not change the report and well I assume it reduced the size of the file.

adamraine commented 2 years ago

startFlow is the public API. We could add more pre-work to the startFlow function in a non-breaking release so it's safer to use startFlow.

At the time of writing this comment, there is not currently any difference between the two so new UserFlow should still work.

ChristopherPHolder commented 2 years ago

Thanks, then I will go ahead and make sure I use startFlow()

adamraine commented 2 years ago

Keeping this open as we need to discuss a long-term solution. skipAudits: ['full-page-screenshot'] is just a workaround.

connorjclark commented 2 years ago

For all steps in a user flow script we need to run full-page-screenshot in a "dehanced" version that doesn't modify the viewport height/dpr, so we don't possibly step on later timespans/snapshots.

Also axe likes to scroll the page! We need to disable that too.

Currently no reason to add this to Settings/flags, but if did want to add an option it would be to startFlow and would default to "don't muck with the page" mode.

umeshaccion commented 1 year ago

@adamraine https://github.com/GoogleChrome/lighthouse/pull/14610 is this PR merged

adamraine commented 1 year ago

No and it wouldn't immediately fix this issue