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(responsiveness): add element screenshot to INP diagnostic #13984

Merged
merged 4 commits into from
May 11, 2022

Conversation

brendankenny
Copy link
Member

@brendankenny brendankenny commented May 10, 2022

part of #13916

https://crrev.com/c/3632661 added a nodeId for interaction events, so we can add it to the TraceElements gatherer and tell users which element they interacted with that caused the INP event. Right now it's just an outerHTML/snippet, but with #13983 fixed, it should get the element screenshot treatment (when available).


Possibly more controversial:

This PR combines the element screenshot with the thread-breakdown table introduced in #13982 in the same audit.

Typically we align audits with tasks, e.g. we don't have an inefficient images audit, we have optimized-images/modern-image-formats/image-size-responsive audits, because they all involve different types of tasks.

Trying to figure out what is going on with an INP result is a single task. Knowing what element was clicked on (or typed in) adds context to the thread-breakdown information, and wouldn't be helpful beyond an FYI as its own audit.

I was wishing for an auditDetails that just lets you have more or less a repeated auditDetails in it, with no special surrounding styling, and it turns out we do have that, we just never use it for anything: {type: 'list'} :) It was introduced for some of the JSON-LD audits in progress at the time, but then never actually used. From that PR:

we already have several audits using single-column tables to get an ugly list, and switching them to a dedicated list type (sometime in the future) would be great, so let's leave that path open to them. So:

export interface DetailsRendererList {
 type: 'list',
 items: DetailsRendererSnippet[]
} 

aka items still has to be an array of only snippets, but that will allow us to easily expand what's allowed in items in the future.

so we just need to expand the type of items allowed in a list. We also have never used the snippet details type/renderer, so I've removed it for now.

new INP work breakdown audit

@brendankenny brendankenny requested a review from a team as a code owner May 10, 2022 16:25
@brendankenny brendankenny requested review from connorjclark and removed request for a team May 10, 2022 16:25
@brendankenny
Copy link
Member Author

Base automatically changed from work-audit to master May 10, 2022 17:39
@connorjclark connorjclark mentioned this pull request May 11, 2022
5 tasks
Copy link
Collaborator

@connorjclark connorjclark left a comment

Choose a reason for hiding this comment

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

TIL about list too. Can you think of any other audits where we could be using that (or maybe combine into one "task" audit)?

lighthouse-core/audits/work-during-interaction.js Outdated Show resolved Hide resolved
* @param {LH.Gatherer.FRTransitionalContext} context
* @return {Promise<Array<TraceElementData>>}
*/
static async getResponsivenessElement(trace, context) {
Copy link
Collaborator

@connorjclark connorjclark May 11, 2022

Choose a reason for hiding this comment

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

nit: getResponsivenessElement -> getResponsivenessElements

Copy link
Member Author

Choose a reason for hiding this comment

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

nit: getResponsivenessElement -> getResponsivenessElements

Maybe I'll split the difference and keep the singular name but change it to return a single TraceElementData, then the caller can wrap it in an array like the lcp node id

listContainer.appendChild(snippetEl);
const listItem = this.render(item);
if (!listItem) return;
listContainer.appendChild(listItem);
Copy link
Collaborator

Choose a reason for hiding this comment

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

was gonna suggest append over appendChild but I see we use it in so many places in report anyway. #13991

Copy link
Member Author

Choose a reason for hiding this comment

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

was gonna suggest append over appendChild but I see we use it in so many places in report anyway. #13991

it's always a good time to prefer append :)

@brendankenny
Copy link
Member Author

TIL about list too. Can you think of any other audits where we could be using that (or maybe combine into one "task" audit)?

not any off the top of my head, but it seems like they must exist :) We should maybe go through in one of the bug bashes/eng syncs.

If we add CLS attribution, it may make sense to combine with layout-shift-elements

Comment on lines +61 to +62
// NOTE: any `Details` type *should* be usable in `items`, but check
// styles/report-ui-features are good before adding.
Copy link
Member Author

Choose a reason for hiding this comment

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

also added a note that in theory any details should work in a list, it's just repeatedly adding elements in the lh-details div, but in practice they may interfere with each other due to styling, or assumptions in details renderer (e.g. they're the only child in their parent), etc. Need to double check they work before adding another.

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.

3 participants