Closed Bug 1419840 Opened 6 years ago Closed 6 years ago

Black out the part of screenshots that is not included by the configuration's selectors

Categories

(Testing :: mozscreenshots, enhancement)

Version 3
enhancement
Not set
normal

Tracking

(firefox59 fixed)

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: jaws, Assigned: mill2540, Mentored)

Details

Attachments

(2 files)

Bug 1403686 implemented support for cropping, which uses CSS selectors to define which parts of the browser we are interested in. If there are multiple selectors applied for a configuration, we crop to the smallest area that includes all of these regions.

I have attached a screenshot that shows us cropping to #navigator-toolbox and the control center popup. However, in the screenshot you will see that the webpage scrollbars and webpage background are still visible.

We should black out these areas so that they are ignored by the comparisons since our testing framework has not accounted for what might get displayed there.
Mentor: mconley, jaws
Assignee: nobody → mill2540
Status: NEW → ASSIGNED
For this bug you'll want need to pass in the source rectangles (before they have been unioned) to the cropping function, and then when you create your canvas[1], after you call drawImage, you can set each pixel to black if it is not included in the source rectangles.

If you are able to figure out the inverse of the union, then you could use CanvasRenderingContext2D.fillRect() [2], otherwise you can use CanvasRenderingContext2D.getImageData() [3]/CanvasRenderingContext2D.putImageData() [4] to set each pixel to black.

[1] https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm#378-384

[2] https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/fillRect

[3] https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/getImageData

[4] https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/putImageData
Comment on attachment 8931953 [details]
Bug 1419840 - Draws rectangles for elements rather than the union of the selected regions.  .

https://reviewboard.mozilla.org/r/203012/#review208540

Using the default canvas background color instead of black is fine with me. I don't see what extra work still remains, but you said that there was some remaining work in an IRC message.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:217
(Diff revision 1)
> -    let finalRect = undefined;
> +    const windowLeft = browserWindow.screenX * scale;
> +    const windowTop = browserWindow.screenY * scale;
> +    const windowWidth = browserWindow.outerWidth * scale;
> +    const windowHeight = browserWindow.outerHeight * scale;
> +
> +    let bounds = undefined;

You can just write `let bounds;`, it will be `undefined` implicitly.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:251
(Diff revision 1)
>        } else {
> -        finalRect = finalRect.union(newRect);
> +        bounds = bounds.union(rect);
>        }
>      }
>  
> -    // Add fixed padding
> +    return [bounds, rects];

Array destructuing can be error-prone since it is necessary to maintain the order of the array in all places that it is used.

It is less error prone to use object destructuring since the order will not matter.

Please change this to `return {bounds, rects};` (We don't need to write `{bounds: bounds}` since ES5 supports object shorthand property names[1]).

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:333
(Diff revision 1)
>        for (const selector of obj.selectors) {
>          finalSelectors.push(selector);
>        }
>      }
>  
> -    const rect = this._findBoundingBox(finalSelectors, windowType);
> +    const [bounds, rects] = this._findBoundingBox(finalSelectors, windowType);

This would get changed to `const {bounds, rects} = ...`.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:400
(Diff revision 1)
>          // By drawing the image with the negative offset, the unwanted regions
>          // are drawn off canvas, and are not captured when the canvas is saved.
> -        ctx.drawImage(img, -rect.x, -rect.y);
> +        // ctx.drawImage(img, -rect.x, -rect.y);

This is not needed anymore it appers, please delete these three lines.
Attachment #8931953 - Flags: review?(jaws) → review+
Flags: needinfo?(jaws)
Comment on attachment 8931953 [details]
Bug 1419840 - Draws rectangles for elements rather than the union of the selected regions.  .

https://reviewboard.mozilla.org/r/203012/#review209114

Looks good, thanks!
Flags: needinfo?(jaws)
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d4bab755ea68
Draws rectangles for elements rather than the union of the selected regions.  r=jaws.
https://hg.mozilla.org/mozilla-central/rev/d4bab755ea68
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: