Closed Bug 1403686 Opened 7 years ago Closed 7 years ago

Crop screenshots to the region that their configuration has requested

Categories

(Testing :: mozscreenshots, enhancement)

Product:
Testing
Component:
mozscreenshots
Version:
Version 3
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: jaws, Assigned: mill2540, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

Bug 1403686 - Crops screenshots to a region based on a selector. Initial Impl of cropping with Chris Cho.
59 bytes, text/x-review-board-request
jaws
: review+
Details
The cropped image
9.58 KB, image/png
Details
The difference between the original image and the cropped image
457 bytes, image/png
Details
The original image before cropping
9.59 KB, image/png
Details 
Bug 1403682 is implementing support to calculate the region that is important to a mozscreenshot configuration.

This bug is to implement cropping support for our screenshots given the region.

The code for this should live in /browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm. This should use Canvas[1] to crop the image[2]. The cropped image should overwrite the larger source image.

This should be implemented with automated tests that load the source image, confirm that the dimensions of the source image match expectations, crop the image, and confirm that the cropped image dimensions match expectations.

A further test could use a image that is all rgb(255,0,0) with one 10x10 rgb(0,255,0) square in the middle of it. After cropping we can use Canvas to load the cropped image and verify that each pixel matches rgb(0,255,0).

The tests for this should live in a new file in the browser/tools/mozscreenshots directory (must start with `browser_`).

[1] https://developer.mozilla.org/en-US/docs/Web/API/Canvas_API
[2] https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/drawImage
Mentor: jaws, mconley
Comment on attachment 8916286 [details]
Bug 1403686 - Crops screenshots to a region based on a selector. Initial Impl of cropping with Chris Cho.

https://reviewboard.mozilla.org/r/187512/#review192746

::: commit-message-f03dd:2
(Diff revision 1)
> +Bug 1403686 - Initial implementation of image cropping for screenshots, worked with Chris Cho. r?jaws
> +Currently, there is only one test implemented for this feature, which fails. It seems that drawing to a canvas is leaving some sort of artifact in the image which causes compareCanvases function to report a small difference.

Have you tried comparing them outside of your code to see which pixels are different?

You can use BeyondCompare (https://www.scootersoftware.com/download.php) to compare two images and it will show you which pixels differ.

::: browser/tools/mozscreenshots/browser_screenshots_cropping.js:22
(Diff revision 1)
> +      // Draw the image with an offset such that only the desired region will
> +      // be visible on the canvas.

This comment doesn't make much sense within draw().

::: browser/tools/mozscreenshots/browser_screenshots_cropping.js:49
(Diff revision 1)
> +  const diff =  nsIDOMWindowUtils.compareCanvases(testCanvas, expectedCanvas, {});
> +  return diff;
> +}
> +
> +async function cropAndCompare(window, src, expected, test) {
> +

nit, remove this blank line.

::: browser/tools/mozscreenshots/browser_screenshots_cropping.js:52
(Diff revision 1)
> +
> +async function cropAndCompare(window, src, expected, test) {
> +
> +    await TestRunner._cropImage(window, src, {x: 0, y: 0, width: 64, height: 64}, test);
> +
> +    await compareImages(window, expected, OS.Path.toFileURI(test));

This function is called with arguments in order of [window, expected, test].

compareImages is defined as accepting arguments in order of [window, test, expected].

::: browser/tools/mozscreenshots/browser_screenshots_cropping.js:59
(Diff revision 1)
> +
> +add_task(async function crop() {
> +  const window = Services.wm.getMostRecentWindow("navigator:browser");
> +
> +  const tmp = OS.Constants.Path.tmpDir;
> +  ok(await cropAndCompare(

Use is() instead of ok() since is() allows you to compare the two values and will output what the two values are to the test log.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:250
(Diff revision 1)
> +
> +    const promise = new Promise((resolve, reject) => {
> +      const img = new Image();
> +
> +      img.onload = function() {
> +        // Make sure that the source image is not larger than then region we want to crop to

This comment is confusing. Shouldn't the source image be required to be *larger* than the cropping region?

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:251
(Diff revision 1)
> +        if (img.naturalWidth < rect.x - rect.width ||
> +            img.naturalHeight < rect.y - rect.height) {

Why are rect.x and rect.y useful in this comparison?

Wouldn't `img.naturalWidth < rect.width || img.naturalHeight < rect.height` be correct?

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:260
(Diff revision 1)
> +        canvas.imageSmoothingEnabled = false;
> +        canvas.mozImageSmoothingEnabled = false;

According to the documentation these should only be useful when scaling the image, but it wouldn't hurt to leave them in.
Attachment #8916286 - Flags: review?(jaws) → review-
Attached image The cropped image — 

    
    

    
  


  

    
    

    
  
Comment on attachment 8916286 [details]
Bug 1403686 - Crops screenshots to a region based on a selector. Initial Impl of cropping with Chris Cho.

https://reviewboard.mozilla.org/r/187512/#review192944

::: commit-message-f03dd:2
(Diff revision 1)
> +Bug 1403686 - Initial implementation of image cropping for screenshots, worked with Chris Cho. r?jaws
> +Currently, there is only one test implemented for this feature, which fails. It seems that drawing to a canvas is leaving some sort of artifact in the image which causes compareCanvases function to report a small difference.

Yes. It appears to be a problem with outlines, being not quite the same in some cases, which is why I had the smoothing disabled on the off chance it helped. I will attach some example images to the bug

        Attachment #8916286 -
        Flags: review?(mill2540)

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8916286 [details]
Bug 1403686 - Crops screenshots to a region based on a selector. Initial Impl of cropping with Chris Cho.

https://reviewboard.mozilla.org/r/187512/#review196842

::: browser/tools/mozscreenshots/browser_screenshots_cropping.js:13
(Diff revision 3)
> +  const { document, Image } = window;
> +
> +  const promise = new Promise((resolve, reject) => {
> +    const img = new Image();
> +
> +    img.onload = function() {

Please add an img.onerror function and have that fail that test.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:249
(Diff revision 3)
> +    const { document, Image } = window;
> +
> +    const promise = new Promise((resolve, reject) => {
> +      const img = new Image();
> +
> +      img.onload = function() {

Please also add an img.onerror function here as well that should reject the promise.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:262
(Diff revision 3)
> +        canvas.imageSmoothingEnabled = false;
> +        canvas.mozImageSmoothingEnabled = false;

The canvas element doesn't have a `imageSmoothingEnabled` nor `mozImageSmoothingEnabled` property so this is in effect dead code.

You should set this on the context. Also, the `mozImageSmoothingEnabled` property is dprecated. You should only use the unprefixed `imageSmoothingEnabled` property.

        Attachment #8916286 -
        Flags: review?(jaws) → review+

    
    

    
  
Comment on attachment 8916286 [details]
Bug 1403686 - Crops screenshots to a region based on a selector. Initial Impl of cropping with Chris Cho.

https://reviewboard.mozilla.org/r/187512/#review197406

Along with fixing the issues outlined in my previous pass, this code should now be updated to use the `_findBoundingBox` function from bug 1403682 and the selectors that were added in bug 1403680.

    
    

    
      
  

    
    

    
  
Comment on attachment 8916286 [details]
Bug 1403686 - Crops screenshots to a region based on a selector. Initial Impl of cropping with Chris Cho.

https://reviewboard.mozilla.org/r/187512/#review198188

::: commit-message-f03dd:4
(Diff revision 4)
> +Bug 1403686 - Initial implementation of integration with 1403682 & 1403680 as well as fixing minor issues from review r?jaws
> +Can now crop screenshots properly, for basic cases. Still needs work to add support for more complex cases such as DevTools or new windows.
> +Removed redundant smoothing flags and added error handleing.
> +This is somewhat WIP at the moment, since I think some changes still need to be made in the part of TestRunner._performCombo that chooses the windowType. I'll need to check with Mike tomorrow to see if he has fixed it yet.

Is this still a WIP? Do you still need to check with Mike?

Please update this commit message to give a one-sentence high level description as the first line. Then after a blank line below that, you can describe more in detail.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8916286 [details]
Bug 1403686 - Crops screenshots to a region based on a selector. Initial Impl of cropping with Chris Cho.

https://reviewboard.mozilla.org/r/187512/#review201596

::: browser/tools/mozscreenshots/mozscreenshots/extension/Screenshot.jsm:106
(Diff revision 9)
> -        if (windowID && Services.sysinfo.getProperty("version").indexOf("10.") !== 0) {
> -          // Capture only that window on 10.7+
> -          args.push("-l");
> -          args.push(windowID);
> -        }
> +        // if (windowID && Services.sysinfo.getProperty("version").indexOf("10.") !== 0) {
> +        //   // Capture only that window on 10.7+
> +        //   args.push("-l");
> +        //   args.push(windowID);
> +        // }

We should remove unused code, instead of commenting it out.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:23
(Diff revision 9)
> +// TODO: Should this be XPCOMUtils.defineLazyModuleGetter(this, "Screenshot", "chrome://mozscreenshots/content/Screenshot.jsm")?
> +Cu.import("chrome://mozscreenshots/content/Screenshot.jsm");

I don't think it's necessary - this is for a test run. You can remove the TODO.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:386
(Diff revision 9)
> +        canvas.toBlob((blob) => {
> +              // Use a filereader to convert the raw binary blob into a writable buffer
> +              const fr = new FileReader();
> +              fr.onload = function(e) {
> +                const buffer = new Uint8Array(e.target.result);
> +                // Save the file and complete the promise
> +                OS.File.writeAtomic(targetPath, buffer, {}).then(resolve);
> +              };
> +              // Do the conversion
> +              fr.readAsArrayBuffer(blob);
> +        });

Alignment seems to be a bit busted here - we're doing 2 space indentation everywhere else.

    
    

    
  
Comment on attachment 8916286 [details]
Bug 1403686 - Crops screenshots to a region based on a selector. Initial Impl of cropping with Chris Cho.

https://reviewboard.mozilla.org/r/187512/#review201622

::: browser/tools/mozscreenshots/browser_screenshots_cropping.js:47
(Diff revision 9)
> +async function cropAndCompare(window, src, expected, test, region) {
> +    await TestRunner._cropImage(window, src, region, test);
> +
> +    return compareImages(window, expected, OS.Path.toFileURI(test));

The indentation here is inconsistent with the rest of the file. Please use 2-space indentation.

    
    

    
      
  

    
    

    
  
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d3ed36f4fb7a
Crops screenshots to a region based on a selector.  r=jaws

    
    

    
      
  

    
    

    
  
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fd1ee37cb15c
Crops screenshots to a region based on a selector.  r=jaws
Flags: needinfo?(mill2540)

    
    

    
      
  

    
    

    
  
Comment on attachment 8916286 [details]
Bug 1403686 - Crops screenshots to a region based on a selector. Initial Impl of cropping with Chris Cho.

https://reviewboard.mozilla.org/r/187510/#review204020

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/TabsInTitlebar.jsm:34
(Diff revision 12)
> +        if (Services.appinfo.OS == "Linux") {
> +          return Promise.reject("TabsInTitlebar isn't supported on Linux");
> +        }

This may not be the best solution, since it will cause any tests using this configuration on Linux to not capture any screenshots. Another solution Rand recomeneded might be to replace 
```
selectors: ["#navigator-toolbox", "#titlebar"]
```
with something like
```
selectors: Services.appinfo.OS == "Linux ? ["#navigator-toolbox", "#titlebar"] : ["#navigator-toolbox"] 
```

    
    

    
  
Comment on attachment 8916286 [details]
Bug 1403686 - Crops screenshots to a region based on a selector. Initial Impl of cropping with Chris Cho.

https://reviewboard.mozilla.org/r/187512/#review204178

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/TabsInTitlebar.jsm:34
(Diff revision 12)
> +        if (Services.appinfo.OS == "Linux") {
> +          return Promise.reject("TabsInTitlebar isn't supported on Linux");
> +        }

This may not be the best solution, as it will disable all screenshots on Linux for any test which uses TabsInTitlebar, since both branches will get deactivated. One solution that Rand suggested would be to replace 
```
selectors: ["#navigator-toolbox", "#titlebar"]
```
with 
```
selectors: Services.appinfo.OS == "Linux" : ["#navigator-toolbox"] ? ["#navigator-toolbox", "#titlebar"]
```

    
  
Blocks: 1412357

    
    

    
  
(In reply to Robin Miller from comment #27)
> Another solution Rand
> recommended might be to replace 
> ```
> selectors: ["#navigator-toolbox", "#titlebar"]
> ```
> with something like
> ```
> selectors: Services.appinfo.OS == "Linux ? ["#navigator-toolbox",
> "#titlebar"] : ["#navigator-toolbox"] 
> ```

+1 for this solution.
Flags: needinfo?(mill2540)

    
    

    
  
Comment on attachment 8916286 [details]
Bug 1403686 - Crops screenshots to a region based on a selector. Initial Impl of cropping with Chris Cho.

https://reviewboard.mozilla.org/r/187512/#review204726

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:351
(Diff revision 12)
> -        .then(() => {
> -          this.completedCombos++;
> -        });
> -    };
>  
> +    let browserWindow = Services.wm.getMostRecentWindow("navigator:browser");

How is this handling our capturing of non-browser windows e.g. the devtools pop-out window?

    
    

    
  
Comment on attachment 8916286 [details]
Bug 1403686 - Crops screenshots to a region based on a selector. Initial Impl of cropping with Chris Cho.

https://reviewboard.mozilla.org/r/187512/#review204726

> How is this handling our capturing of non-browser windows e.g. the devtools pop-out window?

The window we get with `navigator:browser` here is just being used as a place to hold the off screen canvas that `_cropImage` needs to crop screenshots. 

To capture a different window, such as the devtools pop-out window, the relevant configuration specifies a different window type [like this](https://searchfox.org/mozilla-central/source/browser/tools/mozscreenshots/mozscreenshots/extension/configurations/DevTools.jsm#62), which is then used at the end of the `_performCombo` function. The window type and selectors get passed into the `_findBoundingBox` which gets the bounds of the region we want to capture in screen coordinates. Since all the screenshots capture the whole desktop before cropping, when they are cropped to the screen coordinates, only the relevant window is left.

One thing to note is that this *does not* support capturing elements in different windows within the same combo. I don't think should be a problem though, since that would always run the risk of the window manager placing the new window over one the of elements you wanted to capture.

    
    

    
      
  

    
    

    
  
Comment on attachment 8916286 [details]
Bug 1403686 - Crops screenshots to a region based on a selector. Initial Impl of cropping with Chris Cho.

https://reviewboard.mozilla.org/r/187512/#review196842

> The canvas element doesn't have a `imageSmoothingEnabled` nor `mozImageSmoothingEnabled` property so this is in effect dead code.
> 
> You should set this on the context. Also, the `mozImageSmoothingEnabled` property is dprecated. You should only use the unprefixed `imageSmoothingEnabled` property.

I these can just be removed, since they were just me trying to figure out the problem with the images made in GIMP.

    
  
Flags: needinfo?(jaws)

    
    

    
  
Previous (all-green) try push of screenshots job, https://treeherder.mozilla.org/#/jobs?repo=try&revision=43da649a8cbf

I just pushed to tryserver again to make sure that the mochitest and xpcshell tests also pass.
Flags: needinfo?(jaws)

    
    

    
      
  

    
    

    
  
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da9706e0d3c2
Crops screenshots to a region based on a selector. Initial Impl of cropping with Chris Cho.  r=jaws

    
    

    
  
Backout by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a1f2e0532e8e
Backed out changeset da9706e0d3c2 for ESlint failures on browser_screenshots_cropping.js r=backout on a CLOSED TREE

    
    

    
      
  

    
    

    
  
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b184c87f7606
Crops screenshots to a region based on a selector. Initial Impl of cropping with Chris Cho.  r=jaws
Flags: needinfo?(jaws)

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/b184c87f7606
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox59:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59

    
  
Depends on: 1525773

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: