Closed
Bug 1403682
Opened 7 years ago
Closed 7 years ago
Convert configuration CSS selectors to pixel regions
Categories
(Testing :: mozscreenshots, enhancement)
Tracking
(firefox58 fixed)
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: jaws, Assigned: will2616, Mentored)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
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_`).
Reporter | ||
Updated•7 years ago
|
Assignee: chochri5 → will2616
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 3•7 years ago
|
||
mozreview-review |
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•7 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) |
Reporter | ||
Comment 6•7 years ago
|
||
mozreview-review |
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) |
Reporter | ||
Updated•7 years ago
|
Mentor: jaws, mconley
Reporter | ||
Comment 8•7 years ago
|
||
mozreview-review |
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) |
Reporter | ||
Comment 10•7 years ago
|
||
mozreview-review |
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) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8914577 -
Attachment is obsolete: true
Reporter | ||
Comment 13•7 years ago
|
||
mozreview-review |
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•7 years ago
|
||
Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/51fded9c47aa Add _findBoundingBox function and tests for mozscreenshots cropping; r=jaws
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/51fded9c47aa
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment 17•7 years ago
|
||
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)
Comment 18•7 years ago
|
||
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•7 years ago
|
||
Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d5be34ec792e Add _findBoundingBox function and tests for mozscreenshots cropping; r=jaws
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d5be34ec792e
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(will2616)
You need to log in
before you can comment on or make changes to this bug.
Description
•