core(font-size): don't allow a deleted node to fail gatherer #9928
Conversation
| failingNode.cssRule = cssRule; | ||
| } catch (err) { | ||
| // The node was deleted. Don't set `cssRule` in that case. | ||
| log.error('font-size', err.message); |
There was a problem hiding this comment.
This seems like this could lead to error fatigue, and is it anyone's error? fetchSourceRule already returns undefined if we can't find a font rule for the failing element, and we just silently drop that case. This seems basically equivalent in importance for logging
There's nothing to do in this case (and the element's not even around anymore). I could see maybe log.verbose, but there doesn't seem to be a good use for the logged results.
| return failingNode; | ||
| }); | ||
|
|
||
| const analyzedFailingNodesData = await Promise.all(analysisPromises); | ||
| const analyzedFailingNodesData = (await Promise.all(analysisPromises)) | ||
| // Throw out the nodes that got deleted. |
There was a problem hiding this comment.
(also when fetchSourceRule returns undefined)
There was a problem hiding this comment.
does it?
we want to keep if it is undefined, I think
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| const analyzedFailingNodesData = await Promise.all(analysisPromises); | ||
| const analyzedFailingNodesData = (await Promise.all(analysisPromises)) | ||
| // Throw out the nodes that got deleted. | ||
| .filter(data => 'cssRule' in data); |
There was a problem hiding this comment.
any reason not to do .filter(data => data.cssRule);? Usually we reserve in for making tsc happy on poorly discriminated unions
There was a problem hiding this comment.
this is also filtered down on https://github.com/GoogleChrome/lighthouse/pull/9928/files#diff-aed43c42d5d8fb3c5d70f6775865c8deR351, so could remove one of them
There was a problem hiding this comment.
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)
connorjclark
changed the title
Fixes #9255