1568831 - Make the Eyedropper tool work with oop iframes

Status: RESOLVED FIXED

(DevTools :: Inspector, task, P3)

Description

[Patrick Brosset <:pbro>]

With Fission and oop iframes in documents, we want the Eyedropper tool to still be able to sample colors from the entire page.

This tool, when it starts, takes a screenshot of the entire page. So this depends on bug 1499845.

Comment 1

[Chris Peterson [:cpeterson]]

Tentatively moving all bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to the "?" triage milestone.

This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:

0ee3c76a-bc79-4eb2-8d12-05dc0b68e732

Comment 2

[Chris Peterson [:cpeterson]]

Tracking for Fission Nightly (M6)

Comment 3

[Patrick Brosset <:pbro>]

I spent some time investigating this a little bit. Here are my findings. Hopefully they can help fix this bug later.

The new API the EyeDropper should use to capture a screenshot of the page in order to display a magnified version of it that allows selecting colors is drawSnapshot (found on WindowGlobalParent).

This only works from the parent process. However it can draw any frames you want. You just need to call it on the right browsing context.
Here are 2 examples:

• To draw the inside of the selected tab, you'd do something like this (say, from the browser console): gBrowser.selectedBrowser.browsingContext.currentWindowGlobal.drawSnapshot().
• However for the entire browser: docShell.browsingContext.currentWindowGlobal.drawSnapshot().

The API requires 3 arguments:

• The first one is a DOMRect used to determine the position and size of the snapshot, it can be null if you want the entire viewport.
• the second is the scale.
• the third is the background color you need.

Example code you can run in the browser console:

var bc = gBrowser.selectedBrowser.browsingContext;

var img = await bc.currentWindowGlobal.drawSnapshot(null, 2, "white")

var c = document.createElement('canvas');
c.width = img.width;
c.height = img.height;
var cx = c.getContext('2d');
cx.drawImage(img, 0, 0);

c.toBlob(blob => {
  var url = window.URL.createObjectURL(blob);
	gBrowser.addTab(url, {triggeringPrincipal: document.nodePrincipal});
  window.URL.revokeObjectURL(url);
})

So, not only would we need to change the code inside this function to use drawSnapshot, but more importantly, we need to change how the EyeDropper itself is used. Right now we always instantiate it in the process the inspector targets. This means inside a normal tab, we end up instantiating the EyeDropper inside a content process, not the parent process. So we can't use drawSnapshot there.

Comment 4

[Chris Peterson [:cpeterson]]

dt-fission-m2-reserve bugs do not need to block Fission Nightly (M6). For now, let's track them for Fission riding the trains to Beta (M7) so we revisit these bugs before we ship Fission.

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Some STRs, so it's easily reproducible:

• Load http://janodvarko.cz/tests/fission/case1/index.html
• Open DevTools Toolbox and select the Inspector panel
• Click on the Pipette icon “Grab a color from the page”
• Try to pick green color from the remote iframe. The Eydroper shows always white color -> BUG

Honza

Comment 6

[Neha Kochar [:neha]]

Bulk move of all dt-fission-m3-mvp bugs to Fission MVP milestone.

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Comment 3 still applies here, we need to use drawSnapshot in order to retrieve content from remote frame that might be in the page.

Also, we might still keep the actor in a content process and use devtools/server/actors/utils/capture-screenshot.js, since we're going to modify it as well for other screenshot related feature that need to be fixed for fission + remote frame.

Comment 8

[Justin Peter]

This also applies to <object> tags, would that also fall under this bug?

Comment 9

[Chris Peterson [:cpeterson]]

Moving some dt-fission-m3-mvp bugs from Fission MVP to M7 (blocking Beta experiment).

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1568831 - [devtools] Turn EyeDropper into an class. r=ladybenko.

Depends on D103746

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1568831 - [devtools] Add a test for the eyedropper when the page is zoomed-in. r=ladybenko.

The behaviour of the eyedropper is impacted when the page is zoomed-in, but we
didn't have a test for that, so we're adding one here.

Depends on D103877

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1568831 - [devtools] Make eyedropper fission compatible. r=ladybenko.

The current implementation of the eyedropper was using drawWindow, which does
not handle remote frames.
To make the tool fission compatible, we re-use the screenshot-content actor which
do support remote frames. We had to do a few modification on it though so the
eyedropper would work the same as it does at the moment:

• add a disableFlash property to the screenshot options, so we don't play the
  flash animation for the eyedropper
• retrieve the page current zoom level from the screenshot-content actor so we
  can adjust the size of the resulting image
• split the dpr screenshot option into 2 distinct ones:
  • snapshotScale, which will be the scale passed to drawSnapshot. By default,
    this will be the dpr of the window, so the screenshot is as crisp as it can be.
  • fileScale, which will be the scale of the resulting file.
• we control the fileScale that is sent to the server with the ignoreDprForFileScale
  on the client. This is needed by the eyedropper so it doesn't draw images that are
  too big when the dpr is > 1.

With all those changes, browser_inspector_highlighter-eyedropper-frames.js is passing
with fission enabled, so we can remove the fail-if annotation it had.

Depends on D103878

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c04b2c82c45c
[devtools] Turn EyeDropper into an class. r=ladybenko.
https://hg.mozilla.org/integration/autoland/rev/259d8d1ed711
[devtools] Add a test for the eyedropper when the page is zoomed-in. r=ladybenko.
https://hg.mozilla.org/integration/autoland/rev/0bfd8c9d71a6
[devtools] Make eyedropper fission compatible. r=ladybenko.

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/c04b2c82c45c
https://hg.mozilla.org/mozilla-central/rev/259d8d1ed711
https://hg.mozilla.org/mozilla-central/rev/0bfd8c9d71a6