Closed Bug 1332936 Opened 5 years ago Closed 5 years ago

"Screenshot node" should not include inspector's highlight of node


(DevTools :: Inspector, defect, P2)



(firefox55 fixed)

Firefox 55
Tracking Status
firefox55 --- fixed


(Reporter: Tobbi, Assigned: zer0)


(Blocks 1 open bug)



(1 file)

When using the "Screenshot Node" function from the Developer Tool's inspector, I found that it includes the light-blue color that is used to mark the selected node.

However, in most cases, this isn't what I wanted when I wanted to use the Screenshot node function. In most cases I wanted the raw element layout without Inspector interfering.
Inspector bug triage (filter on CLIMBING SHOES).
Assignee: nobody → zer0
Priority: -- → P2
Tobias, I can't really reproduce it, could you let me know the STR and in which platform?

I can totally see why it would happens, but currently I'm not able to make it happens (at least on OS X).
Flags: needinfo?(tobbi.bugs)
No problem. As far as I can tell, this bug was mitigated since the element loses focus as soon as you have mouse-focus on the context menu of the Inspector.

But, you can still reproduce it by following these steps:

1. Open, for example, Google and the Inspector.
2. Right-click on a node in the inspector, I used the Google logo for this. The node gets highlighted with a light-blue color.
3. With the arrow keys of your keyboard navigate to the "Screenshot node" item of the context menu. Once there, press Enter.
4. Check the resulting png file. It contains a blue overlay.
Flags: needinfo?(tobbi.bugs)
Thanks! I was able to reproduce this way.
Comment on attachment 8847543 [details]
Bug 1332936 - hiding box model before taking the node's screenshot from inspector;

::: devtools/client/inspector/inspector.js:1770
(Diff revision 1)
>    },
>    /**
>     * Initiate gcli screenshot command on selected node.
>     */
> -  screenshotNode: function () {
> +  screenshotNode: Task.async(function* () {

This now returns a promise, which could fail I imagine. So I think we should catch rejections when we call it. See line 1147 where we call this.

menu.append(new MenuItem({
  id: "node-menu-screenshotnode",
  label: INSPECTOR_L10N.getStr("inspectorScreenshotNode.label"),
  disabled: !isScreenshotable,
  click: () => this.screenshotNode().catch(e => console.error(e)),

::: devtools/client/inspector/inspector.js:1775
(Diff revision 1)
> -  screenshotNode: function () {
> +  screenshotNode: Task.async(function* () {
>      const command = Services.prefs.getBoolPref("devtools.screenshot.clipboard.enabled") ?
>        "screenshot --file --clipboard --selector" :
>        "screenshot --file --selector";
> +
> +    // Bug 1332936 - it possible calls `screenshotNode` when the box model's highlighter

Minor rephrasing proposal for the 2 first sentences: 

It's possible to call `screenshotNode` while the boxmodel highlighter is still visible, therefore showing it in the picture.

::: devtools/client/inspector/inspector.js:1779
(Diff revision 1)
> +
> +    // Bug 1332936 - it possible calls `screenshotNode` when the box model's highlighter
> +    // is still visible. Therefore it will be included in the picture.
> +    // To avoid that, we have to hide it before taking the screenshot. The `hideBoxModel`
> +    // will do that, calling `hide` for the highlighter only if previously shown.
> +    yield this.highlighter.hideBoxModel();

Please add a new line after this line.
Attachment #8847543 - Flags: review?(pbrosset) → review+
Pushed by
hiding box model before taking the node's screenshot from inspector; r=pbro
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
I have reproduced this bug with Nightly 53.0a1(2017-01-22) on Windows 10, 64 bit!

The Bug's fix is now verified on Latest Nightly 55.0a1

Build ID 	20170329030240
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0

Duplicate of this bug: 1358265
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.