Repository: GoogleChrome/lighthouse (js)

Issue:
full-page-screenshot: handle elements beyond MAX_SCREENSHOT_HEIGHT
ref https://github.com/GoogleChrome/lighthouse/pull/10716#issuecomment-657904592 and #11118 

i made https://output.jsbin.com/koqamuh/4/quiet as a test page for us. it has a (CSS) pixel ruler running down the side.

Here's a report of the page: https://marked-cable.surge.sh/color-contrast-over16000px-koqamuh.html

The elem screenshots start failing between 6200 and 6300 px

![image](https://user-images.githubusercontent.com/39191/87830901-f4daf180-c836-11ea-9c81-19e11c15149f.png)



---------

#### DPR stuff..

6300  is  roughly 16384 / 2.625.. so  it's to do with some DPR adjustments.. (I'm capturing this on my imacpro with a native dpr of 2.)


FWIW i tried removing the `/ dpr` in maxScreenshotHeight: 
https://github.com/GoogleChrome/lighthouse/blob/06d3e1b8d69c9b7eb0a9b94c08632b097eb1eeec/lighthouse-core/gather/gatherers/full-page-screenshot.js#L75

and after doing that, [it cut out](https://marked-cable.surge.sh/color-contrast-over16000px-koqamuh-nodprdivisor.html) at 16300 . (right before 16384, the real max)

![image](https://user-images.githubusercontent.com/39191/87831174-8c404480-c837-11ea-93dd-07f1bd5c1243.png)

that much seems good.
of course, the placement of the markers are off because of some other dpr math I didnt account for.




Pull request:
report: do not show element screenshot if out of bounds
ref #11121

![image](https://user-images.githubusercontent.com/4071474/95489017-e266e480-095b-11eb-8243-592ad73f9faf.png)


Patch:
diff --git a/lighthouse-core/report/html/renderer/details-renderer.js b/lighthouse-core/report/html/renderer/details-renderer.js
index 4a1e5ea74..5782cfed1 100644
--- a/lighthouse-core/report/html/renderer/details-renderer.js
+++ b/lighthouse-core/report/html/renderer/details-renderer.js
@@ -533,7 +533,7 @@ class DetailsRenderer {
       item.boundingRect,
       maxThumbnailSize
     );
-    element.prepend(elementScreenshot);
+    if (elementScreenshot) element.prepend(elementScreenshot);
 
     return element;
   }
diff --git a/lighthouse-core/report/html/renderer/element-screenshot-renderer.js b/lighthouse-core/report/html/renderer/element-screenshot-renderer.js
index 571039a00..edfb681c7 100644
--- a/lighthouse-core/report/html/renderer/element-screenshot-renderer.js
+++ b/lighthouse-core/report/html/renderer/element-screenshot-renderer.js
@@ -17,6 +17,18 @@
 /** @typedef {LH.Artifacts.Rect} Rect */
 /** @typedef {{width: number, height: number}} Size */
 
+/**
+ * @param {LH.Audit.Details.FullPageScreenshot} fullPageScreenshot
+ * @param {LH.Artifacts.Rect} rect
+ * @return {boolean}
+ */
+function screenshotContainsRect(fullPageScreenshot, rect) {
+  return rect.top >= 0 &&
+    rect.right <= fullPageScreenshot.width &&
+    rect.bottom <= fullPageScreenshot.height &&
+    rect.left >= 0;
+}
+
 /**
  * @param {number} value
  * @param {number} min
@@ -153,13 +165,16 @@ class ElementScreenshotRenderer {
         top: Number(el.dataset['rectTop']),
         bottom: Number(el.dataset['rectTop']) + Number(el.dataset['rectHeight']),
       };
-      overlay.appendChild(ElementScreenshotRenderer.render(
+      const screenshotElement = ElementScreenshotRenderer.render(
         dom,
         templateContext,
         fullPageScreenshot,
         elementRectSC,
         maxLightboxSize
-      ));
+      );
+      if (!screenshotElement) return;
+
+      overlay.appendChild(screenshotElement);
       overlay.addEventListener('click', () => {
         overlay.remove();
       });
@@ -188,14 +203,19 @@ class ElementScreenshotRenderer {
   /**
    * Renders an element with surrounding context from the full page screenshot.
    * Used to render both the thumbnail preview in details tables and the full-page screenshot in the lightbox.
+   * Returns null if element rect is outside screenshot bounds.
    * @param {DOM} dom
    * @param {ParentNode} templateContext
    * @param {LH.Audit.Details.FullPageScreenshot} fullPageScreenshot
    * @param {LH.Artifacts.Rect} elementRectSC Region of screenshot to highlight.
    * @param {Size} maxRenderSizeDC e.g. maxThumbnailSize or maxLightboxSize.
-   * @return {Element}
+   * @return {Element|null}
    */
   static render(dom, templateContext, fullPageScreenshot, elementRectSC, maxRenderSizeDC) {
+    if (!screenshotContainsRect(fullPageScreenshot, elementRectSC)) {
+      return null;
+    }
+
     const tmpl = dom.cloneTemplate('#tmpl-lh-element-screenshot', templateContext);
     const containerEl = dom.find('.lh-element-screenshot', tmpl);
 


Test patch:
diff --git a/lighthouse-core/test/report/html/renderer/element-screenshot-renderer-test.js b/lighthouse-core/test/report/html/renderer/element-screenshot-renderer-test.js
index 827a582cd..65740b8cd 100644
--- a/lighthouse-core/test/report/html/renderer/element-screenshot-renderer-test.js
+++ b/lighthouse-core/test/report/html/renderer/element-screenshot-renderer-test.js
@@ -21,6 +21,18 @@ const TEMPLATE_FILE = fs.readFileSync(
   'utf8'
 );
 
+/**
+ * @param {{left: number, top: number, width: number, height:number}} opts
+ * @returns {LH.Artifacts.Rect}
+ */
+function makeRect(opts) {
+  return {
+    ...opts,
+    right: opts.left + opts.width,
+    bottom: opts.top + opts.height,
+  };
+}
+
 describe('ElementScreenshotRenderer', () => {
   let dom;
 
@@ -43,12 +55,12 @@ describe('ElementScreenshotRenderer', () => {
       width: 1000,
       height: 1000,
     };
-    const elementRectSC = {
+    const elementRectSC = makeRect({
       left: 50,
       top: 50,
       width: 200,
       height: 300,
-    };
+    });
     const renderContainerSizeDC = {
       width: 500,
       height: 500,
@@ -81,6 +93,30 @@ describe('ElementScreenshotRenderer', () => {
     /* eslint-enable max-len */
   });
 
+  it('returns null if element is out of bounds', () => {
+    const fullPageScreenshot = {
+      width: 1000,
+      height: 1000,
+    };
+    const elementRectSC = makeRect({
+      left: 50,
+      top: 5000,
+      width: 200,
+      height: 300,
+    });
+    const renderContainerSizeDC = {
+      width: 500,
+      height: 500,
+    };
+    expect(ElementScreenshotRenderer.render(
+      dom,
+      dom.document(),
+      fullPageScreenshot,
+      elementRectSC,
+      renderContainerSizeDC
+    )).toBe(null);
+  });
+
   describe('getScreenshotPositions', () => {
     it('centers the screenshot on the highlighted area', () => {
       expect(