-
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
report: use entity classification to filter third-parties #14697
Changes from all commits
7833354
0a154eb
3212aa6
c71a8ab
14163bd
46d706c
b5caa4f
8689903
5b67f2e
0abc053
170adcb
5e18f75
f0674c7
69725b7
ab5d847
04bfff8
e8c1c7e
0a87d78
d953829
aea71bf
cb16f02
1e73c13
e79f44b
6f80513
d299ae8
f462dc8
9189494
f1d9893
1ad67ac
0230896
abf81f9
73cc72d
1daccab
8cd7347
4a44dc6
60c609a
39753fe
0459e38
a9e5d6a
f7adfe3
21a53ba
da02b4f
8780384
880c09e
86fc747
122486e
d14d703
ac3f59e
d373c45
171d563
1059e6a
ad43021
02f212a
99a9a67
793b1ae
6eef166
2667564
36bff06
2f50be6
093d60c
9e29828
c3efb66
49bce13
388e557
42cb558
932b029
8ebe3d9
4c9d8be
b57fb8e
730705a
673162a
d3a68c7
38bf2f7
6cba854
a839ecc
0224365
5304284
e328f66
c31aca9
f54852d
2934c84
737fdd0
a3ca3e0
608fe9e
aa0e54f
f70b5d3
39c441d
35ebe83
0b82a99
dcabac8
399a92a
145b9d5
56d36ce
d11f3bb
f532bf7
811f103
7971218
47553d0
ca82fe3
7568caf
831fa7c
1ef0970
8b1a3a4
8a87ec2
ebfa00d
dba8d79
1fb6ad7
45b63b4
a1a293d
c08fb30
157e3b5
c6876d6
8d97490
c142ed6
ded03b6
3b56d0b
656287c
0a0136f
118f97d
250d4f6
9c80a3e
aec61da
11ec2b1
e8760bb
eeb341a
53d77a3
a42dd62
edbff41
2d3c251
c858df8
31c41ae
5e363d0
dd8771c
2cc6b44
6f392b8
2466ec6
8a6c2cc
c2625a4
f3a16d3
41aa773
e850f7e
1265edb
c69cb3d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -244,7 +244,9 @@ export class ReportUIFeatures { | |
|
||
tablesWithUrls.forEach((tableEl) => { | ||
const rowEls = getTableRows(tableEl); | ||
const thirdPartyRows = this._getThirdPartyRows(rowEls, Util.getFinalDisplayedUrl(this.json)); | ||
const primaryRowEls = rowEls.filter(rowEl => !rowEl.classList.contains('lh-sub-item-row')); | ||
const thirdPartyRowEls = this._getThirdPartyRows( | ||
primaryRowEls, Util.getFinalDisplayedUrl(this.json)); | ||
|
||
// create input box | ||
const filterTemplate = this._dom.createComponent('3pFilter'); | ||
|
@@ -253,9 +255,9 @@ export class ReportUIFeatures { | |
filterInput.addEventListener('change', e => { | ||
const shouldHideThirdParty = e.target instanceof HTMLInputElement && !e.target.checked; | ||
let even = true; | ||
let rowEl = rowEls[0]; | ||
let rowEl = primaryRowEls[0]; | ||
while (rowEl) { | ||
const shouldHide = shouldHideThirdParty && thirdPartyRows.includes(rowEl); | ||
const shouldHide = shouldHideThirdParty && thirdPartyRowEls.includes(rowEl); | ||
|
||
// Iterate subsequent associated sub item rows. | ||
do { | ||
|
@@ -272,12 +274,12 @@ export class ReportUIFeatures { | |
}); | ||
|
||
this._dom.find('.lh-3p-filter-count', filterTemplate).textContent = | ||
`${thirdPartyRows.length}`; | ||
`${thirdPartyRowEls.length}`; | ||
this._dom.find('.lh-3p-ui-string', filterTemplate).textContent = | ||
Globals.strings.thirdPartyResourcesLabel; | ||
|
||
const allThirdParty = thirdPartyRows.length === rowEls.length; | ||
const allFirstParty = !thirdPartyRows.length; | ||
const allThirdParty = thirdPartyRowEls.length === primaryRowEls.length; | ||
const allFirstParty = !thirdPartyRowEls.length; | ||
|
||
// If all or none of the rows are 3rd party, hide the control. | ||
if (allThirdParty || allFirstParty) { | ||
|
@@ -319,25 +321,29 @@ export class ReportUIFeatures { | |
* @return {Array<HTMLElement>} | ||
*/ | ||
_getThirdPartyRows(rowEls, finalDisplayedUrl) { | ||
/** @type {Array<HTMLElement>} */ | ||
const thirdPartyRows = []; | ||
const finalDisplayedUrlRootDomain = Util.getRootDomain(finalDisplayedUrl); | ||
const firstPartyEntityName = this.json.entities?.firstParty; | ||
|
||
/** @type {Array<HTMLElement>} */ | ||
const thirdPartyRowEls = []; | ||
for (const rowEl of rowEls) { | ||
if (rowEl.classList.contains('lh-sub-item-row')) continue; | ||
|
||
const urlItem = rowEl.querySelector('div.lh-text__url'); | ||
if (!urlItem) continue; | ||
|
||
const datasetUrl = urlItem.dataset.url; | ||
if (!datasetUrl) continue; | ||
const isThirdParty = Util.getRootDomain(datasetUrl) !== finalDisplayedUrlRootDomain; | ||
if (!isThirdParty) continue; | ||
if (firstPartyEntityName) { | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We haven't discussed that. Should we have a tracker issue labeled |
||
const urlItem = rowEl.querySelector('div.lh-text__url'); | ||
if (!urlItem) continue; | ||
const datasetUrl = urlItem.dataset.url; | ||
if (!datasetUrl) continue; | ||
const isThirdParty = Util.getRootDomain(datasetUrl) !== finalDisplayedUrlRootDomain; | ||
if (!isThirdParty) continue; | ||
} | ||
|
||
thirdPartyRows.push(rowEl); | ||
thirdPartyRowEls.push(rowEl); | ||
} | ||
|
||
return thirdPartyRows; | ||
return thirdPartyRowEls; | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -90,6 +90,9 @@ class ReportUtils { | |
} | ||
} | ||
|
||
// Attach table/opportunity items with entity information. | ||
ReportUtils.classifyEntities(result.entities, audit); | ||
|
||
// TODO: convert printf-style displayValue. | ||
// Added: #5099, v3 | ||
// Removed: #6767, v4 | ||
|
@@ -187,6 +190,75 @@ class ReportUtils { | |
return clone; | ||
} | ||
|
||
/** | ||
* Given an audit's details, identify and return a URL locator function that | ||
* can be called later with an `item` to extract the URL of it. | ||
* @param {LH.FormattedIcu<LH.Audit.Details.TableColumnHeading[]>} headings | ||
* @return {{(item: LH.FormattedIcu<LH.Audit.Details.TableItem>): string|undefined}=} | ||
alexnj marked this conversation as resolved.
Show resolved
Hide resolved
|
||
*/ | ||
static getUrlLocatorFn(headings) { | ||
// The most common type, valueType=url. | ||
const urlKey = headings.find(heading => heading.valueType === 'url')?.key; | ||
if (urlKey && typeof urlKey === 'string') { | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. technically table items can override their There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not that I've seen in any of our audits. Why would we allow that, vs. introducing a new |
||
}; | ||
} | ||
|
||
// The second common type, valueType=source-location. | ||
const srcLocationKey = headings.find(heading => heading.valueType === 'source-location')?.key; | ||
if (srcLocationKey) { | ||
// Return a function that extracts item.source.url. | ||
return (item) => { | ||
const sourceLocation = item[srcLocationKey]; | ||
if (typeof sourceLocation === 'object' && sourceLocation.type === 'source-location') { | ||
return sourceLocation.url; | ||
} | ||
}; | ||
} | ||
|
||
// More specific tests go here, as we need to identify URLs in more audits. | ||
} | ||
|
||
/** | ||
* Mark TableItems/OpportunityItems with entity names. | ||
* @param {LH.Result.Entities|undefined} entityClassification | ||
* @param {import('../../types/lhr/audit-result').Result} audit | ||
*/ | ||
static classifyEntities(entityClassification, audit) { | ||
if (!entityClassification) return; | ||
if (audit.details?.type !== 'opportunity' && audit.details?.type !== 'table') { | ||
Comment on lines
+231
to
+232
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Intent might be clearer if these checks are moved out into In here, the params could then be narrowed, dropping the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So I was passing |
||
return; | ||
} | ||
|
||
// If details.items are already marked with entity attribute during an audit, nothing to do here. | ||
const {items, headings} = audit.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. | ||
const urlLocatorFn = ReportUtils.getUrlLocatorFn(headings); | ||
if (!urlLocatorFn) return; | ||
|
||
for (const item of items) { | ||
const url = urlLocatorFn(item); | ||
if (!url) continue; | ||
|
||
let origin; | ||
try { | ||
// Non-URLs can appear in valueType: url columns, like 'Unattributable' | ||
origin = Util.parseURL(url).origin; | ||
} catch {} | ||
if (!origin) continue; | ||
|
||
const entityIndex = entityClassification.entityIndexByOrigin[origin]; | ||
if (entityIndex === undefined) return; | ||
const entity = entityClassification.list[entityIndex]; | ||
item.entity = entity.name; | ||
} | ||
} | ||
|
||
/** | ||
* @param {LH.Result['configSettings']} settings | ||
* @return {!{deviceEmulation: string, screenEmulation?: string, networkThrottling: string, cpuThrottling: string, summary: string}} | ||
|
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.
Was removing this intentional?
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 I've introduced a regression in third-party entity count shown on the filter, with that line removed. We don't have a unit test to check if the count displayed is correct, so I'm going to add one for that as well.
Longer version:
The legacy filter checkbox visibility works by matching rows that have a
lh-text__url
and adataset.url
attached to it. The filter checkbox is hidden ifcount(such rows) === count(all table rows)
in which case all rows are third-parties, or ifcount(such rows)
equals to zero, in which case it assumes all rows are 1st parties.For an audit like
third-party-summary
, this still works when the third-party count mismatches and fails the all-third-party check. The 1st party check (incorrectly) succeeds. I think we can rewrite this section with both comparisons basing on primary rows only.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.
Refactored the code; I think it should be good for a re-review. We're now passing only first-level/primary rows into this function.