-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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(visibility): new artifact to check page visibility during gathering #9650
Conversation
crossing my fingers that the smoke test works in CI |
truth. IMO there's room for a new |
wdyt of going with the inpage impl. for now, and investigating extending the protocol later? |
yeah, that makes sense, but sticking to @patrickhulce's |
I don't think we have to worry too much about protocol method timeouts (similar to the |
I think you're focusing just on the case where we should provide a better error message (NO_FCP -> "hey make sure the page is visible, I see it's not right now"). But there's also the case where the page wasn't visible for a short period, which will affect the correctness of the metrics. We should still display a warning in that case. And a new artifact was the simplest way I saw to do that + test it (the smoke test). Are you suggesting we just do the error message enhancement now, and defer the other part for when the new |
re-reading all the discussion here - as I understand, @patrickhulce and @brendankenny are suggesting removing the gatherer/artifact and doing just I think this needs to be in a gatherer because we want to track any visibility change, as a single change of a long enough duration can be enough to affect the metrics. And this is probably simpler to do in a gatherer than within the gather-runner? so there's 1) better error message when FCP happens and 2) a warning that metrics might be inaccurate. The warning shown to solve 2) is probably enough to also solve 1)? hope this post helps in necro'ing this PR :) |
/** | ||
* @description Warning that the page wasn't visible during the entire Lighthouse run. Metrics may be inaccurate. | ||
*/ | ||
warningVisibility: 'Window was hidden for part of all of the audit. Metrics may be inaccurate.', |
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.
part or all?
@@ -118,6 +127,10 @@ class Metrics extends Audit { | |||
items: [metrics], | |||
}; | |||
|
|||
if (artifacts.Visibility.some(event => event.state === 'hidden')) { | |||
context.LighthouseRunWarnings.push(str_(UIStrings.warningVisibility)); |
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.
i think we want to issue a different error than NO_FCP if there was no fcp and there was a visibility issue. introduce a TAB_BACKGROUNDED_DURING_AUDIT error-ish.
if we do have FCP but have visibility issues, then this warning sgtm.
we only look at the trace (and thus hit NO_FCP) in a computed artifact after we've disconnected from gathering. So we can't evaluate then. the closest equivalent would be getting But I dont think that works for the goals here. AFAICT, we need to know if it went invisible at any point during the trace in order to tell them not to do that. |
We dropped priority on #4139 and think this PR has higher priority now |
I think Paul is with 2) as laid out in my last comment #9650 (comment) is this the direction we want? maybe we can do periodic checks during page load inside gather runner for visibility, instead of making it an artifact? or maybe we just do a report-level warning, instead of leaving to the metrics audit, and then we can just do this in gather-runner (won't even need a base artifact). |
we also get a lighthouse/lighthouse-core/gather/driver/wait-for-condition.js Lines 76 to 78 in 3a820a3
For the after the fact, "something may have messed with your metrics" warning, what about the |
This is maybe a positive, certainly not a negative, but seems we've been fine all these years without it. So closing but with the expectation that it can be revived sometime in the future if needed. |
fixes #6154
New artifact for logging
document.visibilityState
as it changes.Goal
Goal is to warn that the metrics may not be accurate, as Chrome possibly wasn't visible when it wanted to paint / do stuff / and was throttling the page.
smoke tests
Needed to add a chrome flag (
--disable-popup-blocking
) for this smoke test, so I introducedcli-flags
to the smoke test setup. The flag is harmless for other runs, so we could also just add it and not support arbitrary cli options per smoke test.NO_FCP
We also want to give a better warning alongside NO_FCP if it happens b/c the page was occluded for the entire run. However, none of the gatherers finish if N_FCP happens, so to actually support this visibility checking would need to be done within
gather-runner.js
. Maybe that means this shouldn't be an actual gatherer? Better to move all the gathering logic to the runner, or to replicate the logic a bit in the runner?Discussion: doc
Example:
yarn static-server node lighthouse-cli/ http://localhost:10200/visibility.html?hidden --view --chrome-flags=--disable-popup-blocking