report: use entity classification to filter third-parties #14697
Conversation
There was a problem hiding this comment.
FYI, you can find audits that will display the filter by searching for the selector .lh-3p-filter:not([hidden]) in the elements panel. There are fewer than before, so I had to do this to find some relevant audits.
Looking at bootup-time in the sample reports, something unexpected was that "Unattributable" is filtered out when the checkbox is unchecked. We shouldn't consider that to be third-party, I think... Maybe just hardcode to ignore "Unattributable" in _getThirdPartyRows?
Co-authored-by: Connor Clark <cjamcl@google.com>
This sounds like a bug. Unattributable shouldn't get classified as 3p. |
connorjclark
changed the title
Co-authored-by: Connor Clark <cjamcl@google.com>
There was a problem hiding this comment.
late comments, sorry, maybe good for incorporation into #14655
| // Avoid injecting entity names into audits that would would | ||
| // make the diff at the end of this test difficult. | ||
| delete clonedSampleResult.entities; | ||
|
|
There was a problem hiding this comment.
this is a good reason for the split of augmentation vs back compat in #14701
| if (!entityClassification) return; | ||
| if (audit.details?.type !== 'opportunity' && audit.details?.type !== 'table') { |
There was a problem hiding this comment.
Intent might be clearer if these checks are moved out into prepareReportResult, so as you're reading that method you know you only need to step into classifyEntities if it's a table or opportunity (right now you have to trust the comment isn't out of date).
In here, the params could then be narrowed, dropping the undefined on entityClassification, and audit could become details of LH.Audit.Details.Opportunity|LH.Audit.Details.Table
There was a problem hiding this comment.
These are to keep TS happy in the rest of the function, as we're passing the full audit here. Is there a better way to do it?
There was a problem hiding this comment.
I mean something like
diff --git a/report/renderer/report-utils.js b/report/renderer/report-utils.js
index a38b52d2b..739cc6210 100644
--- a/report/renderer/report-utils.js
+++ b/report/renderer/report-utils.js
@@ -91,7 +91,11 @@ class ReportUtils {
}
// Attach table/opportunity items with entity information.
- ReportUtils.classifyEntities(result.entities, audit);
+ if (result.entities && audit.details) {
+ if (audit.details.type === 'opportunity' || audit.details.type === 'table') {
+ ReportUtils.classifyEntities(result.entities, audit.details);
+ }
+ }
// TODO: convert printf-style displayValue.
// Added: #5099, v3
@@ -224,17 +228,12 @@ class ReportUtils {
/**
* Mark TableItems/OpportunityItems with entity names.
- * @param {LH.Result.Entities|undefined} entityClassification
- * @param {import('../../types/lhr/audit-result').Result} audit
+ * @param {LH.Result.Entities} entityClassification
+ * @param {LH.FormattedIcu<LH.Audit.Details.Opportunity|LH.Audit.Details.Table>} details
*/
- static classifyEntities(entityClassification, audit) {
- if (!entityClassification) return;
- if (audit.details?.type !== 'opportunity' && audit.details?.type !== 'table') {
- return;
- }
-
+ static classifyEntities(entityClassification, details) {
// If details.items are already marked with entity attribute during an audit, nothing to do here.
- const {items, headings} = audit.details;
+ const {items, headings} = details;
if (!items.length || items.some(item => item.entity)) return;
// Identify a URL-locator function that we could call against each item to get its URL.There was a problem hiding this comment.
So I was passing audit.details or more specific params earlier, I think. But I now think it would be beneficial to pass the full audit to get the id as well, in case if any audit needs special handling in future.
| // Return a function that extracts item.url. | ||
| return (item) => { | ||
| const url = item[urlKey]; | ||
| if (typeof url === 'string') return url; |
There was a problem hiding this comment.
technically table items can override their valueType themselves, so this would just opt them out even if they could still be classified from item[urlKey].value. Maybe a corner case worth dealing with after #14655, though (not sure if any audit actually does this with url)
There was a problem hiding this comment.
Not that I've seen in any of our audits. Why would we allow that, vs. introducing a new valueType?
| // We rely on entity-classification for new LHRs that support it. | ||
| if (!rowEl.dataset.entity || rowEl.dataset.entity === firstPartyEntityName) continue; | ||
| } else { | ||
| // Without 10.0's entity classification, fallback to the older root domain-based filtering. |
There was a problem hiding this comment.
Not sure if this was already discussed, but maybe after a certain amount of time (circa 11.0?) we should drop this code? We want to render old LHRs, but I'm not sure 3p filtering of old LHRs is that important after a while
There was a problem hiding this comment.
We haven't discussed that. Should we have a tracker issue labeled 11.0?
|
Added a couple of points there that could use clarity, @brendankenny. I'll add these changes into #14655. |
Report changes split from #14622. This PR rewires third-parties filter checkbox on reports to use
LHR.entitiesentity classification. For legacy reports, it will continue falling back to the origin string match based filter.mainafter core: add entity classification of origins to the LHR #14622 lands.