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.

Sign up for GitHub

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(font-size): don't allow a deleted node to fail gatherer #9928

Merged
merged 5 commits into from
Nov 8, 2019
Merged

core(font-size): don't allow a deleted node to fail gatherer #9928

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e2c008b
core(font-size): don't allow a delete node to fail gatherer
connorjclark Nov 6, 2019
276c282
pr
connorjclark Nov 6, 2019
a3c1f15
no log
connorjclark Nov 6, 2019
5ca763f
comments
connorjclark Nov 6, 2019
1d2c7d8
fix
connorjclark Nov 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 29 additions & 18 deletions lighthouse-core/gather/gatherers/seo/font-size.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,24 +192,29 @@ function getNodeTextLength(node) {
/**
* @param {Driver} driver
* @param {LH.Crdp.DOM.Node} node text node
* @returns {Promise<NodeFontData['cssRule']|undefined>}
* @returns {Promise<?NodeFontData['cssRule']|undefined>}
*/
async function fetchSourceRule(driver, node) {
const matchedRules = await driver.sendCommand('CSS.getMatchedStylesForNode', {
nodeId: node.nodeId,
});
const sourceRule = getEffectiveFontRule(matchedRules);
if (!sourceRule) return undefined;

return {
type: sourceRule.type,
range: sourceRule.range,
styleSheetId: sourceRule.styleSheetId,
parentRule: sourceRule.parentRule && {
origin: sourceRule.parentRule.origin,
selectors: sourceRule.parentRule.selectors,
},
};
try {
const matchedRules = await driver.sendCommand('CSS.getMatchedStylesForNode', {
nodeId: node.nodeId,
});
const sourceRule = getEffectiveFontRule(matchedRules);
if (!sourceRule) return undefined;

return {
type: sourceRule.type,
range: sourceRule.range,
styleSheetId: sourceRule.styleSheetId,
parentRule: sourceRule.parentRule && {
origin: sourceRule.parentRule.origin,
selectors: sourceRule.parentRule.selectors,
},
};
} catch (err) {
Sentry.captureException(err, {tags: {gatherer: 'FontSize'}});
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
return null;
}
}

/**
Expand Down Expand Up @@ -294,11 +299,17 @@ class FontSize extends Gatherer {
.sort((a, b) => b.textLength - a.textLength)
.slice(0, MAX_NODES_SOURCE_RULE_FETCHED)
.map(async failingNode => {
failingNode.cssRule = await fetchSourceRule(driver, failingNode.node);
const cssRule = await fetchSourceRule(driver, failingNode.node);
// If cssRule is null, then the node was deleted. Don't set `cssRule` in that case.
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
if (cssRule !== null) {
failingNode.cssRule = cssRule;
}
return failingNode;
});

const analyzedFailingNodesData = await Promise.all(analysisPromises);
const analyzedFailingNodesData = (await Promise.all(analysisPromises))
// Throw out the nodes that got deleted.
Copy link
Member

Choose a reason for hiding this comment

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

(also when fetchSourceRule returns undefined)

Copy link
Collaborator Author

@connorjclark connorjclark Nov 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

does it?

image

we want to keep if it is undefined, I think

Copy link
Member

@brendankenny brendankenny Nov 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Oh I see. Good catch, but this distinction is definitely going to be overlooked like I just did the next time someone is doing something in here :) (at least without a test for it, which is going to be annoying/hard)

Should we just treat these two failures the same to keep it simpler (e.g. count them both in the total length since the elements were at least in the DOM at some point)? Or rearrange the mapping/filtering to make it clearer.

Copy link
Collaborator Author

@connorjclark connorjclark Nov 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think we shouldn't keep the nodes we know were deleted, because in the case of a timer (that can be modified many many many times) the percentages will be thrown off what is visible at any one point.

I added some comments to make it more clear. Importantly, that second filter isn't filtering result out of the artifact, it is just assigning the stylesheet object to the nodes that we could attribute a stylesheet to.

Copy link
Member

Choose a reason for hiding this comment

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

I think we shouldn't keep the nodes we know were deleted, because in the case of a timer (that can be modified many many many times) the percentages will be thrown off what is visible at any one point.

Wouldn't it be more correct to include it, though? The gatherer is using DOM.getFlattenedDocument to get a snapshot of the DOM at a single point in time, so the percentage including it would be accurate for that moment (which is when all the text lengths come from, not from when we finally get to calling CSS.getMatchedStylesForNode)

e.g. if the timer is the only too-small text on screen, dropping it would give an analyzedFailingTextLength of 0 and no node listing when we do know the node that was failing, and in theory have a decent selector to that would indicate the issue was the timer element.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmmm this sounds compelling. I was thinking we'd somehow have many copies of the "same" node, but that doesn't make sense b/c we just take a snapshot.

.filter(data => 'cssRule' in data);
Copy link
Member

Choose a reason for hiding this comment

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

any reason not to do .filter(data => data.cssRule);? Usually we reserve in for making tsc happy on poorly discriminated unions

Copy link
Member

Choose a reason for hiding this comment

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

this is also filtered down on https://github.com/GoogleChrome/lighthouse/pull/9928/files#diff-aed43c42d5d8fb3c5d70f6775865c8deR351, so could remove one of them

Copy link
Collaborator Author

@connorjclark connorjclark Nov 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

hmm, i think the former check (the one here) needs to keep undefined b/c it still counts towards the total text length (we just failed to attribute to a specific sheet)


const analyzedFailingTextLength = analyzedFailingNodesData.reduce(
(sum, {textLength}) => (sum += textLength),
Expand Down