report: do not show element screenshot if out of bounds #11538
Conversation
connorjclark
requested review from
brendankenny
and removed request for
a team
There was a problem hiding this comment.
Fixes #11121
also you'll have to hash out if this DPR stuff is resolved with @patrickhulce and/or @paulirish if we want to close that :)
lighthouse-core/report/html/renderer/element-screenshot-renderer.js
Outdated
Show resolved
Hide resolved
| @@ -17,6 +17,22 @@ | |||
| /** @typedef {LH.Artifacts.Rect} Rect */ | |||
| /** @typedef {{width: number, height: number}} Size */ | |||
|
|
|||
| /** | |||
| * Returns whether rect2 is contained entirely within rect1; | |||
There was a problem hiding this comment.
Returns whether rect2 is contained entirely within rect1
should it not have a screenshot even if it's partially in bounds? e.g. what if just a few pixels are over the edge? Alternative would be to clip to bounds (and maybe return null if some percentage threshold isn't shown or something).
There was a problem hiding this comment.
eh, I'm inclined to keep it like this as it's simpler and this seems like an edge case (hehe).
lighthouse-core/report/html/renderer/element-screenshot-renderer.js
Outdated
Show resolved
Hide resolved
lighthouse-core/test/report/html/renderer/element-screenshot-renderer-test.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
This change LGTM, whether or not it fully resolves #11121. Maybe we should land this but leave that issue open until all parties agree and/or we discuss next eng sync?
ref #11121