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)
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.
Reporter | ||
Updated•6 years ago
|
Mentor: mconley, jaws
Reporter | ||
Updated•6 years ago
|
Assignee: nobody → mill2540
Status: NEW → ASSIGNED
Reporter | ||
Comment 1•6 years ago
|
||
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 hidden (mozreview-request) |
Reporter | ||
Comment 3•6 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(jaws)
Reporter | ||
Comment 5•6 years ago
|
||
mozreview-review |
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!
Reporter | ||
Updated•6 years ago
|
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.
Comment 7•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d4bab755ea68
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•