Convert configuration CSS selectors to pixel regions

RESOLVED FIXED in Firefox 58

Status

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jaws, Assigned: will2616, Mentored)

Tracking

(Blocks: 1 bug)

Version 3
mozilla58
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Bug 1403680 will provide CSS selectors for each region.

This bug should convert the CSS selectors to rectangles in pixel units relative to the window frame.

This should live in browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm

We should also expand the calculated rectangle by 10px on each side so as to include any shadows or other border effects.

This should be implemented along with automated tests that confirm the region calculated matches our expectations. The tests for this should live in a new file in the browser/tools/mozscreenshots directory (must start with `browser_`).
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8914577 [details]
Bug 1403682 - Add _findBoundingBox function and tests for mozscreenshots cropping;

https://reviewboard.mozilla.org/r/185916/#review190904

Making good progress, thanks for letting me see where you're at.

::: browser/tools/mozscreenshots/browser_boundingbox.js:10
(Diff revision 2)
> +"use strict";
> +
> +add_task(async function capture() {
> +  let rect = TestRunner._findBoundingBox("#tabbrowser-tabs");
> +  // Check width calculation on simple example
> +  ok(true, rect.width == Services.wm.getMostRecentWindow("navigator:browser").document.querySelector("#tabbrowser-tabs").getBoundingClientRect().width + 20);

Instead of using ok(true, a == b), use is(a, b).

Also, both ok() and is() take a third argument which is a message that will be printed. It is very useful to help describe what and why the assertion exists.

For example,
is(a.width, b.width, "The width of `a` should equal that of `b` at start of test");

::: browser/tools/mozscreenshots/browser_boundingbox.js:12
(Diff revision 2)
> +add_task(async function capture() {
> +  let rect = TestRunner._findBoundingBox("#tabbrowser-tabs");
> +  // Check width calculation on simple example
> +  ok(true, rect.width == Services.wm.getMostRecentWindow("navigator:browser").document.querySelector("#tabbrowser-tabs").getBoundingClientRect().width + 20);
> +  // Check height calculation on simple example
> +  ok(true, rect.height == Services.wm.getMostRecentWindow("navigator:browser").document.querySelector("#tabbrowser-tabs").getBoundingClientRect().height + 20);

`window` should exist in the current scope and it will be equal to `Services.wm.getMostRecentWindow("navigator:browser")`.

`document` should exist in the current scope and it will be equal to `window.document`.

JSM files do not have access to `window` or `document`, though the function you write could take `window` or `document` as an optional argument.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:172
(Diff revision 2)
> +  _findBoundingBox(selector) {
> +    // TODO: Add union functionality to take an array of selectors
> +    let browserWindow = Services.wm.getMostRecentWindow("navigator:browser");
> +    let element = browserWindow.document.querySelector(selector);
> +    let rect = browserWindow.document.getBoxObjectFor(element);
> +    return {x: rect.screenX - 10,

You should use Geometry.jsm here to convert the rect that you get from getBoxObjectFor to a Geometry.jsm Rect object.

Then you can use Geometry.jsm's Rect.inflate() method.

The only problem with this is that Rect.inflate takes scalars and here we want to inflate by a fixed amount not a percentage. I don't see any callers to Rect.inflate so we can just change how that function works.

This function should return a Geometry.jsm Rect.
Attachment #8914577 - Flags: review?(jaws)
(Assignee)

Comment 4

2 years ago
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> Making good progress, thanks for letting me see where you're at.

Thanks for the feedback, I'll get right on it.
Comment hidden (mozreview-request)
Comment on attachment 8914577 [details]
Bug 1403682 - Add _findBoundingBox function and tests for mozscreenshots cropping;

https://reviewboard.mozilla.org/r/185916/#review191206

Looks good, I think this is really close. After you make these changes this will probably be r+.

::: browser/tools/mozscreenshots/browser_boundingbox.js:1
(Diff revision 3)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> +* License, v. 2.0. If a copy of the MPL was not distributed with this
> +* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

As you are the author of this test you can decide which license you want to apply. MPL2 is fine, but we normally use Public Domain.

See https://www.mozilla.org/en-US/MPL/license-policy/ for more information.

The public domain header is as follows:

Any copyright is dedicated to the Public Domain.
http://creativecommons.org/publicdomain/zero/1.0/

::: browser/tools/mozscreenshots/browser_boundingbox.js:9
(Diff revision 3)
> +
> +"use strict";
> +
> +add_task(async function capture() {
> +  let rect = TestRunner._findBoundingBox(["#tabbrowser-tabs"]);
> +  let tabBar = window.document.querySelector("#tabbrowser-tabs").getBoundingClientRect();

`document` is a global on its own (also accessible from `window.document`), so this and other places can just be `document`.

::: browser/tools/mozscreenshots/browser_boundingbox.js:15
(Diff revision 3)
> +  // Check width calculation on simple example
> +  is(rect.width, tabBar.width + 20, "Checking _findBoundingBox width calculation");
> +  // Check height calculation on simple example
> +  is(rect.height, tabBar.height + 20, "Checking _findBoundingBox height caclulation");
> +
> +  rect = TestRunner._findBoundingBox(["#forward-button", "#TabsToolbar"]);

Should you add a test case for a selector that doesn't match anything? What should happen there?

::: browser/tools/mozscreenshots/browser_boundingbox.js:24
(Diff revision 3)
> +  let height = Math.max(tabToolbar.bottom, fButton.bottom) - Math.min(tabToolbar.top, fButton.top);
> +  // Check width calculation on simple union
> +  is(rect.width, width + 20, "Checking _findBoundingBox union width calculation");
> +  // Check height calculation on simple union
> +  is(rect.height, height + 20, "Checking _findBoundingBox union height calculation");
> +  // TODO: Add more test cases, specifically testing unions

This todo can be removed since you added a union test  case above.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:165
(Diff revision 3)
>  
>    // helpers
>  
> +  /**
> +  * Calculate the bounding box based on CSS selector from config for cropping
> +  * @param {String[]} selectors - array of CSS selectors for relevant DOM element

nit, please add a blank line between above this.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:173
(Diff revision 3)
> +  _findBoundingBox(selectors) {
> +    // TODO: Testing, see browser_boundingbox.js
> +
> +    // Not sure if we want this check or not
> +    // Added new Rect(0, 0, 0, 0) to get rid of lint error
> +    if (selectors.length == 0) {

Yeah, this is a good check.

You can simplify this to just `if (!selectors.length) {`

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:181
(Diff revision 3)
> +    for (let i = 1; i < selectors.length; i++) {
> +      let newElement = browserWindow.document.querySelector(selectors[i]);

Since you don't use `i` on its own, you can change this loop to a `for..of` loop.

`for (let selector of selectors) { }`
Attachment #8914577 - Flags: review?(jaws)
Comment hidden (mozreview-request)
Comment on attachment 8914577 [details]
Bug 1403682 - Add _findBoundingBox function and tests for mozscreenshots cropping;

https://reviewboard.mozilla.org/r/185916/#review192008

Looks good, we can land this once you make the following change.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:172
(Diff revisions 3 - 4)
> -
> -    // Not sure if we want this check or not
> -    // Added new Rect(0, 0, 0, 0) to get rid of lint error
> -    if (selectors.length == 0) {
>        log.info("_findBoundingBox: selectors argument is empty");
>        return new Rect(0, 0, 0, 0);

Since the other error condition (selector not found) returns null, we should just make this return null too.
Attachment #8914577 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)
Comment on attachment 8914577 [details]
Bug 1403682 - Add _findBoundingBox function and tests for mozscreenshots cropping;

https://reviewboard.mozilla.org/r/185916/#review192402

This patch should be updated to use the selectors that Mike is implemented in bug 1403680.
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8914577 - Attachment is obsolete: true
Comment on attachment 8918660 [details]
Bug 1403682 - Add _findBoundingBox function and tests for mozscreenshots cropping;

https://reviewboard.mozilla.org/r/189484/#review196874

r+ with the following changes made.

::: browser/tools/mozscreenshots/browser_boundingbox.js:7
(Diff revision 1)
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +"use strict";
> +
> +add_task(async function capture() {

the function name here seems to be holdover, and not actually related to the test. you can remove the test name and just make this anonymous.

add_task(async function() {
  ...
});

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:214
(Diff revision 1)
> +        finalRect = finalRect.union(newRect);
> +      }
> +    }
> +
> +    // Add fixed padding
> +    finalRect = finalRect.inflateFixed(10 * scale);

This constant (`10`) should be moved to a member variable, then the test can read the value.

The test currently makes an assumption about what the padding will be, and just reading the test it's not clear why the function adds 20 to the width.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:216
(Diff revision 1)
> +    let finalX = browserWindow.screenX * scale;
> +    let finalY = browserWindow.screenY * scale;
> +    let finalW = browserWindow.outerWidth * scale;
> +    let finalH = browserWindow.outerHeight * scale;

These variable names are a bit confusing compared to finalRect. It would seem that finalRect.left would be synonymous with finalRect.x.

I think better names for these would be screenLeft, screenTop, screenWidth, screenHeight.
Attachment #8918660 - Flags: review?(jaws) → review+
Comment hidden (mozreview-request)

Comment 15

a year ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/51fded9c47aa
Add _findBoundingBox function and tests for mozscreenshots cropping; r=jaws
https://hg.mozilla.org/mozilla-central/rev/51fded9c47aa
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
M-e10s(ss) fails now on mozilla-central (doesn't run on autoland):

https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=25bc10affd99b854d971b27f3111050616ab22d5&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-searchStr=ss

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=139152184&repo=mozilla-central

[task 2017-10-24T10:52:28.390Z] 10:52:28     INFO - TEST-START | browser/tools/mozscreenshots/browser_boundingbox.js
[task 2017-10-24T10:52:28.536Z] 10:52:28     INFO - TEST-INFO | started process screentopng
[task 2017-10-24T10:52:29.388Z] 10:52:29     INFO - TEST-INFO | screentopng: exit 0
[task 2017-10-24T10:52:29.388Z] 10:52:29     INFO - Buffered messages logged at 10:52:28
[task 2017-10-24T10:52:29.388Z] 10:52:29     INFO - Entering test bound setup
[task 2017-10-24T10:52:29.389Z] 10:52:29     INFO - installing extension temporarily
[task 2017-10-24T10:52:29.389Z] 10:52:29     INFO - Checking for mozscreenshots extension
[task 2017-10-24T10:52:29.389Z] 10:52:29     INFO - TEST-PASS | browser/tools/mozscreenshots/browser_boundingbox.js | The mozscreenshots extension should be installed - 
[task 2017-10-24T10:52:29.390Z] 10:52:29     INFO - Leaving test bound setup
[task 2017-10-24T10:52:29.394Z] 10:52:29     INFO - Entering test bound 
[task 2017-10-24T10:52:29.397Z] 10:52:29     INFO - Buffered messages finished
[task 2017-10-24T10:52:29.399Z] 10:52:29     INFO - TEST-UNEXPECTED-FAIL | browser/tools/mozscreenshots/browser_boundingbox.js | Checking _findBoundingBox width calculation - Got 1280, expected 1300
[task 2017-10-24T10:52:29.401Z] 10:52:29     INFO - Stack trace:
[task 2017-10-24T10:52:29.402Z] 10:52:29     INFO -     chrome://mochikit/content/browser-test.js:test_is:1011
[task 2017-10-24T10:52:29.404Z] 10:52:29     INFO -     chrome://mochitests/content/browser/browser/tools/mozscreenshots/browser_boundingbox.js:null:11
[task 2017-10-24T10:52:29.406Z] 10:52:29     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:798:9
[task 2017-10-24T10:52:29.408Z] 10:52:29     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:697:9
[task 2017-10-24T10:52:29.411Z] 10:52:29     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59

Related changesets:
https://hg.mozilla.org/mozilla-central/rev/51fded9c47aa
https://hg.mozilla.org/mozilla-central/rev/b46909f2b668
https://hg.mozilla.org/mozilla-central/rev/e91ca49ccf6c

Jared, can you care about a fix or let me know what revisions need to be backed out? Thanks.
Flags: needinfo?(jaws)
Backed out for failing browser-screenshots' browser/tools/mozscreenshots/browser_boundingbox.js on mozilla-central:

https://hg.mozilla.org/mozilla-central/rev/967c95cee709756596860ed2a3e6ac06ea3a053f

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=a80d568a417ea8410cd2d874c1e0267fb92888fe&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=139152799&repo=mozilla-central

[task 2017-10-24T10:54:38.352Z] 10:54:38     INFO - TEST-START | browser/tools/mozscreenshots/browser_boundingbox.js
[task 2017-10-24T10:54:38.477Z] 10:54:38     INFO - TEST-INFO | started process screentopng
[task 2017-10-24T10:54:39.270Z] 10:54:39     INFO - TEST-INFO | screentopng: exit 0
[task 2017-10-24T10:54:39.274Z] 10:54:39     INFO - Buffered messages logged at 10:54:38
[task 2017-10-24T10:54:39.276Z] 10:54:39     INFO - Entering test bound setup
[task 2017-10-24T10:54:39.279Z] 10:54:39     INFO - installing extension temporarily
[task 2017-10-24T10:54:39.284Z] 10:54:39     INFO - Checking for mozscreenshots extension
[task 2017-10-24T10:54:39.287Z] 10:54:39     INFO - TEST-PASS | browser/tools/mozscreenshots/browser_boundingbox.js | The mozscreenshots extension should be installed - 
[task 2017-10-24T10:54:39.297Z] 10:54:39     INFO - Leaving test bound setup
[task 2017-10-24T10:54:39.299Z] 10:54:39     INFO - Entering test bound 
[task 2017-10-24T10:54:39.299Z] 10:54:39     INFO - Buffered messages finished
[task 2017-10-24T10:54:39.300Z] 10:54:39     INFO - TEST-UNEXPECTED-FAIL | browser/tools/mozscreenshots/browser_boundingbox.js | Checking _findBoundingBox width calculation - Got 1280, expected 1300
[task 2017-10-24T10:54:39.301Z] 10:54:39     INFO - Stack trace:
[task 2017-10-24T10:54:39.301Z] 10:54:39     INFO -     chrome://mochikit/content/browser-test.js:test_is:1011
[task 2017-10-24T10:54:39.301Z] 10:54:39     INFO -     chrome://mochitests/content/browser/browser/tools/mozscreenshots/browser_boundingbox.js:null:11
[task 2017-10-24T10:54:39.303Z] 10:54:39     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:798:9
[task 2017-10-24T10:54:39.303Z] 10:54:39     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:697:9
[task 2017-10-24T10:54:39.304Z] 10:54:39     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
Status: RESOLVED → REOPENED
Flags: needinfo?(jaws) → needinfo?(will2616)
Resolution: FIXED → ---
Comment hidden (mozreview-request)

Comment 20

a year ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d5be34ec792e
Add _findBoundingBox function and tests for mozscreenshots cropping; r=jaws
https://hg.mozilla.org/mozilla-central/rev/d5be34ec792e
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
(Assignee)

Updated

a year ago
Flags: needinfo?(will2616)
You need to log in before you can comment on or make changes to this bug.