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(fr): option to skip about:blank jumps #13375

Merged
merged 8 commits into from
Dec 13, 2021
Merged

Conversation

adamraine
Copy link
Member

Step 1 towards navigations triggered by user interactions (e.g. flow.navigate(() => page.click('button'))).

Could enabled about:blank jumps for the first navigation like how we handle disableStorageReset. I wanted to allow users to test a flow starting from another origin (e.g. clicking on the search result in google) without auditing that origin. That won't be possible if there are about:blank jumps on the first navigation, but I'm fine with either for now.

#11313

@adamraine adamraine requested a review from a team as a code owner November 15, 2021 23:40
@adamraine adamraine requested review from connorjclark and removed request for a team November 15, 2021 23:40
@google-cla google-cla bot added the cla: yes label Nov 15, 2021
@brendankenny
Copy link
Member

flow.navigate(() => page.click('button'))

I'm finding it really enjoyable when using the UserFlow-like approach where it's more like Lighthouse is just another step in the flow vs handing off control like this.

What are you thoughts on analysis of a trace with a single navigation in it -> treat it as if it were a navigation? Supporting that would unlock a few use cases:

  • this one
  • navigation + interaction (e.g. navigate+scroll to trigger CLS, navigate+click to trigger FID, etc)
  • possibly use cases like DevTools being able to use Lighthouse as a trace processor and get audit insights back

It's possible it would also allow us to avoid adding parameters for every step, which makes it a lot harder to test everything (e.g. if you were to always skipAboutBlank and you start a navigation on the same origin that you're navigating to, you won't clear ServiceWorkers. Maybe that's desirable, but we'd eventually need smoke tests for always setting it and only setting it on subsequent navigations, and that's just this one option). OTOH, there are other downsides, so maybe it's not sufficient.

Would it be helpful for us to set up some time to sketch out API shapes this week? I feel like it might be more concrete now that folks have experience with user flows vs 9 months ago when it was more hypothetical. Or do we have a new doc talking about goals and the APIs to meet them? I only know of the original design doc.

@adamraine
Copy link
Member Author

What are you thoughts on analysis of a trace with a single navigation in it -> treat it as if it were a navigation? Supporting that would unlock a few use cases:

I'm not sure about automatically converting a timespan to a navigation if there is a navigation event found. I would want to trigger navigations and timespans separately. What if the user ends the timespan before TTI? How do we know which gatherers to run if we don't know if we're in a timespan/navigation? It's possible, but also seems like a big refractor that would block one of our high-priority features.

It's worth discussing some sort of flow.startNavigation / flow.endNavigation if you want to keep LH events inline, but it might also require a large refactor of our navigation runner. I think my preference is still to use a callback here.

navigation + interaction (e.g. navigate+scroll to trigger CLS, navigate+click to trigger FID, etc)

Is there any use case here not already covered by using a navigation followed by a timespan?

Would it be helpful for us to set up some time to sketch out API shapes this week?

Yeah, SGTM.

@brendankenny
Copy link
Member

I'm not sure about automatically converting a timespan to a navigation if there is a navigation event found. I would want to trigger navigations and timespans separately.

Yeah, good point, not automatically, you wouldn't want to accidentally switch from timespan to navigation modes just because there's a navigation in your timespan. I guess I meant more the ability to treat a timespan with a navigation in it as a navigation, at least for the metrics that are includable, starting at navStart instead of the beginning of the trace, etc.

I'm not sure what the best approach for that would be, maybe an explicit fourth mode?

What if the user ends the timespan before TTI?

That's why I mentioned the DevTools trace processing integration, because we have the same problem there...did the trace taken by DevTools start early enough and last long enough to reliably take our metrics? etc.

How do we know which gatherers to run if we don't know if we're in a timespan/navigation?

yeah, this is probably an argument for a different mode or something

It's possible, but also seems like a big refractor that would block one of our high-priority features.

Definitely want to avoid a big refactor, which is an argument for adding to the API surface instead of changing existing stuff.

I don't think the trace validation will be difficult. We more or less know our requirements, just some are currently encoded directly as checks on the trace (e.g. how we find the right navStart) while others we would need to write fresh (e.g. check that the trace ended with network quiet/cpu quiet just through trace events instead of from protocol/in-page events).

But this is probably going too far down the path of thinking what's possible with artifacts instead of focusing on what's needed from the API first.

Is there any use case here not already covered by using a navigation followed by a timespan?

I think the main things are:

  • summation of metrics over both. e.g.
    • CLS is windowed and having two CLSs isn't equivalent to one value over the whole period
    • there are some things that could change the LCP into the timespan section of the flow, but wouldn't be captured by two separate steps
  • maintaining throttling settings across the entire period
  • ease of use: navigation + typical first interaction(s) is a natural thing to want to measure and it should be simple to do and see in a single place in a report

the answer to these also might not be a new mode, maybe it could be a higher level API that does the summarization of a navigation + timespan on behalf of the user or something

@Korrigan
Copy link

Hi @adamraine !

Thank you very much for your work on this, User Flows are definitely awesome!

I'm trying to use flow.navigate(() => page.click('button')) constructs before it event exists, by implementing it myself based on a fork of your branch. I've been able to come up with a quick & dirty POC but I quickly noticed an issue with the skip of about:blank jump and I thought you might be interested about it.

Basically, when we skip about:blank jumps on a page that has a registered event listener on "beforeunload" which performs a network request, an issue arise in PageDependencyGraph which throws an error "Root node was not in redirect chain of mainDocument". That's because the first request after we called flow.navigate is indeed the one made by the event listener and not the main document request as the code expects.

I created a gist with code to reproduce (there is an express server along with a User Flow implementation, and the generated report so you don't have to run it by yourself): https://gist.github.com/Korrigan/0c631848d28af120dc4173b362b146a9

I've worked around it by dumbly replacing the rootNode instead of throwing in my test implementation but I would be happy to provide a better fix if I can, though I'm not sure about the correct approach yet since I haven't really looked at the code in PageDependencyGraph.

By the way, I'll attempt a better flow.navigate(() => page.click('button')) implementation by next week, I'm quite not sure if the code I'll come up with will be good enough for a PR but if you're interested and willing to provide some guidance about what you expect for this, I'll be happy to try :)

@adamraine
Copy link
Member Author

Thanks for experimenting with this feature extremely early @Korrigan!

By the way, I'll attempt a better flow.navigate(() => page.click('button')) implementation by next week, I'm quite not sure if the code I'll come up with will be good enough for a PR but if you're interested and willing to provide some guidance about what you expect for this, I'll be happy to try :)

To be clear, we aren't even done designing this feature yet. The callback surface of flow.navigate(() => page.click('button')) may not be the final API we want to ship. For this reason, I would caution you against committing to a PR that may not align with our final design.

That being said, there is a very experimental branch (we may scrap it eventually) with a workaround for the root node issue already: https://github.com/GoogleChrome/lighthouse/compare/fr-navigation-requestor. If you're truly interested in playing around with this right now, you can start there.

@Korrigan
Copy link

Thanks for your advice @adamraine !

Your fr-navigation-requestor branch seems to be exactly what I need (I experimented with some very similar changes already), I'll hack around from there and see if I can make it work for my specific use case, which is far more simpler than what would be needed to make it work for all lighthouse users.
That way I'll be able to experiment and if it looks like a good implementation I'll be happy to open up a PR when the feature design is complete, if it aligns. Otherwise, I'll just switch to official implementation once it's done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants