core(full-page-screenshot): do not render zero size rects #11853
Conversation
| */ | ||
| /* istanbul ignore next */ | ||
| function getBoundingClientRect(element) { | ||
| // The protocol does not serialize getters, so extract the values explicitly. | ||
| const rect = element.getBoundingClientRect(); | ||
| if (rect.width === 0 || rect.height === 0) return; |
There was a problem hiding this comment.
other places use this function and might expect a 0x0 rect returned, though...
There was a problem hiding this comment.
darn @ts-expect-error and stringification! I'll look closer.
There was a problem hiding this comment.
nope, this is only used in two places: page-function getNodeDetails, and the FPS gatherer.
There was a problem hiding this comment.
Can the silliness be avoided through other means like ignoring head contents?
I agree that doing this for all elements that happen to not have size might be a mistake. The location can often still provide value in hidden element cases.
There was a problem hiding this comment.
yeah, I guess I meant a function called getBoundingClientRect seems like the wrong place to determine if it's worth keeping the rect from the element or not. getBoundingClientRectIfItHasSize, maybe :)
Agreed it is a little weird that all the uses of NodeDetails keep their boundingRect, though. e.g. LinkElements didn't have any before #11405. Maybe we need an easy way for callers of getNodeDetails to indicate that they know that 0x0 bounding rects aren't important and can be discarded?
There was a problem hiding this comment.
points well taken! I've reversed the approach here.
| const rect = | ||
| (item.lhId ? this._fullPageScreenshot.nodes[item.lhId] : null) || item.boundingRect; | ||
| if (!rect) return element; | ||
| const rect = item.lhId ? this._fullPageScreenshot.nodes[item.lhId] : item.boundingRect; |
There was a problem hiding this comment.
I'm thinking the fallback of item.boundingRect doesn't make sense... should we remove?
There was a problem hiding this comment.
Why doesn't it make sense? It's not obvious to me what the issue is.
There was a problem hiding this comment.
i think the idea here was to show something in case the node doesn't show in _fullPageScreenshot.nodes, which could happen if LH registered an element but then it is deleted by the time the full page screenshot gatherer runs. So the user might get an idea of where the element was. Could that be more confusing than helpful?
There was a problem hiding this comment.
i think the idea here was to show something in case the node doesn't show in
_fullPageScreenshot.nodes, which could happen if LH registered an element but then it is deleted by the time the full page screenshot gatherer runs. So the user might get an idea of where the element was. Could that be more confusing than helpful?
I think you're right. In theory if it's not in fullPageScreenshot.nodes, it didn't exist in the DOM anymore when the screenshot was taken, so what's the point in trying to show a spot where it wasn't in the screenshot? That could still be wrong (removed during the screenshot taking so not around when nodes are gathered?) but it seems better to miss a few corner case nodes that could have had a screenshot than to show confusing/misleading element screenshots.
There was a problem hiding this comment.
I see. In that case, I agree as well that it doesn't make sense.
connorjclark
changed the title
connorjclark
changed the title
avoids silly things like this:
at some point during implementation there was a size check here, which would not set
node.boundingRectif zero sized. got lost/replaced by just a size check in the FPS gatherer, but that only covers the "shifted" rect...the intention the entire time was to have
boundingRectbe undefined if zero sized. that way usage code can just doif (rect) ..., and notif (rect && rect.width && rect.height)...