1163332 - 'Screenshot of this element' context menu item in markup view

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Tim Nguyen :ntim]

I sometimes want to take a screenshot of a specific element for various purposes, it'd be nice to get a context menu item for that.

The GCLI code is probably reusable here.

Uservoice request : https://ffdevtools.uservoice.com/forums/246087-firefox-developer-tools-ideas/suggestions/6060256-add-screenshot-of-selected-element-to-inspector

Comment 1

[Patrick Brosset <:pbro>]

I like this idea very much, and you're right, the gcli code can be reused here.

Comment 2

[Jeff Griffiths (:canuckistani) (:⚡︎)]

is this a 'good first/next bug'?

Comment 3

[Patrick Brosset <:pbro>]

Yes it could very well be. I haven't looked too much at how the gcli command could be reused easily from the context menu item, but that shouldn't be too hard. I've already set myself as mentor on this bug (I think some people use this field to find good <something> bugs to work on), but I'll also add the whiteboard flag.

Comment 4

[Sébastien Blin [:sblin] [:amarok]]

If I understand, you will reuse the code of the screenshot gcli command ?
With this code, we can take a screenshot of a selector. For example, on this page, if I open the gcli and enter "screenshot --selector #short_desc_nonedit_display" it takes a screenshot of "'Screenshot of this element' context menu item in markup view".
But if you want to add an item in the context menu, I think we will add a real screenshot menu (with a --fullpage button too for example)

Comment 5

[Patrick Brosset <:pbro>]

(In reply to Sébastien Blin [:sblin] [:amarok] from comment #4)
> If I understand, you will reuse the code of the screenshot gcli command ?
> With this code, we can take a screenshot of a selector. For example, on this
> page, if I open the gcli and enter "screenshot --selector
> #short_desc_nonedit_display" it takes a screenshot of "'Screenshot of this
> element' context menu item in markup view".
> But if you want to add an item in the context menu, I think we will add a
> real screenshot menu (with a --fullpage button too for example)

We already have a button in the UI to take fullpage screenshots (go in the devtools settings panel and check the "Take a fullpage screenshot" box under the "Available Toolbox Buttons" section).
So I don't think we should add a new item in the context menu of the inspector for this. Also because this menu is contextual to the node you right-click on.
It occurs to me that you might have been talking about the *browser* context menu (the one that appears when you right click on the page, not in the inspector). If so, I think bug 1152021 is probably the closest you'll find that's about that. It's not about adding a context menu item, but it's in browser-land, not in devtools-land.

Comment 6

[Léon McGregor]

I've fixed a few bugs before, and I'd be interested in taking this one, if you have a few pointers as to where to get started.

Comment 7

[Patrick Brosset <:pbro>]

Thanks, here are a few pointers:

The contextual menu is defined here:
- the markup is here: /browser/devtools/inspector/inspector.xul, id = inspector-node-popup
- it is setup just before being shown on a node in /browser/devtools/inspector/inspector-panel.js, function _setupNodeMenu
- in this function, menu items are hidden/shown/disabled/enabled depending on which node was right-clicked

We have a GCLI (the command line that appears on Shift+F2) command to take screenshots of the page or a given element:
- the whole command is in /toolkit/devtools/gcli/commands/screenshot.js
- the argument that causes it to take a screenshot of one element only is "selector"

The tricky part of this bug is to figure out how to call this command, with the right arguments, from the context menu action.
I have never looked into this myself so far, but it should be doable pretty easily because all our gcli commands can already be called from what we call Toolbar Buttons (for instance, if you go in the settings panel, then activate the screenshot toolbar button, you'll see a new icon in the toolbar, and clicking on it really just calls through to the command). So you should be able to dig in this direction, trying to figure out where is this done.
If you need more help with this, let me know, I'll spend some more time looking into it.

Comment 8

[Léon McGregor]

I think I've found a way to get a solution, but for some reason I can't take screenshots. The main screenshot.js file fires correctly, but the version of the code that I'm using seems to have trouble with downloads, specifically DownloadIntegration.jsm. It can't save anything or do anything to do with downloads. I'm on the latest version of the main branch, any ideas how to fix this?

Comment 9

[Patrick Brosset <:pbro>]

(In reply to Léon McGregor from comment #8)
> I think I've found a way to get a solution, but for some reason I can't take
> screenshots. The main screenshot.js file fires correctly, but the version of
> the code that I'm using seems to have trouble with downloads, specifically
> DownloadIntegration.jsm. It can't save anything or do anything to do with
> downloads. I'm on the latest version of the main branch, any ideas how to
> fix this?
It would help if you attached the work-in-progress patch that's causing the problem. Nothing comes to mind off the top of my head without looking at your code.

I quickly looked into running gcli commands from the node context-menu and it seemed to work for me (you should look at toolbox.js and the _buildButtons function to learn how to create a requisition and then at DeveloperToolbar.jsm and the createButtons functiont to learn how to use the requisition to execute a command).

Feel free to join our #devtools channel on IRC whenever you have specific questions too.

Comment 10

[Léon McGregor]

Attachment: bug1163332_addnodescreenshot.diff

Attached is the broken patch version. Note that it logs into the browser toolbox as a success even though capturing a screenshot fails.

Having done further digging, I found the problem is in gcli/cli.js line 2061, where something is being evaluated as not valid.

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8613496 [details] [diff] [review]
bug1163332_addnodescreenshot.diff

Review of attachment 8613496 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/inspector/inspector-panel.js
@@ +34,3 @@
>  const LAYOUT_CHANGE_TIMER = 250;
>  
> +XPCOMUtils.defineLazyGetter(this, "gcliInit", function() {

You don't seem to need gcliInit here, get rid of this lazy getter.

@@ +1095,5 @@
> +      //document: 
> +      //commandOutputManager: 
> +    };
> +    CommandUtils.createRequisition(this._target, reqOps).then(requisition => {
> +      requisition.exec(cmdOps);

I think that's where the problem is, my understanding is that you first need to update the requisition with the command to be called, and then you call exec with no particular options.
There's also a shortcut method called updateExec which is nicer to use.
This short code snippet works for me:

CommandUtils.createRequisition(this._target, {
  environment: CommandUtils.createEnvironment(this, '_target')
}).then(requisition => {
  requisition.updateExec("screenshot --selector " + this.selectionCssSelector);
});

I suggest you either ping jwalker on IRC or needinfo him on this bug for more info, he's the author of GCLI and can explain why this works this way better than me.

::: browser/devtools/inspector/inspector.xul
@@ +56,5 @@
>          label="&inspectorShowDOMProperties.label;"
>          oncommand="inspector.showDOMProperties()"/>
> +      <menuitem id="node-menu-screenshotnode"
> +        label="&inspectorScreenshotNode.label;"
> +        oncommand="inspector.screenshotNode()" />

I wonder if this is the best place for this menu item, maybe it would be better further down below, maybe next to the "Scroll into view" item.
My bet is that it will be used a lot less than the copy/showDomProperties/edit/... items, and so it shouldn't be as high in the list.

::: browser/locales/en-US/chrome/browser/devtools/inspector.dtd
@@ +104,5 @@
>       the DOM properties of the current node. When triggered, this item
>       opens the split Console and displays the properties in its side panel. -->
>  <!ENTITY inspectorShowDOMProperties.label       "Show DOM Properties">
> +
> +<!-- LOCALIZATION NOTE (inspectorShowDOMProperties.label): This is the label

Please update the key name between the parenthesis to match the actual key name.

@@ +106,5 @@
>  <!ENTITY inspectorShowDOMProperties.label       "Show DOM Properties">
> +
> +<!-- LOCALIZATION NOTE (inspectorShowDOMProperties.label): This is the label
> +     shown in the inspector contextual-menu for the item that lets users take
> +     a screenshot of that specific item. -->

a screenshot of the currently selected node.

Comment 12

[Léon McGregor]

Attachment: bug1163332_addnodescreenshot.diff - ver 2

Ah, thanks. I get the impression I was over-complicating things, and this version of the patch should fix that.

Attached is a working patch based on the snippet you provided. However, I've noticed that you can take screenshots of <head>, elements of height 0 and similarly hidden-styled elements. 

This doesn't cause a crash, it just creates an empty .png file. Should I find a way of detecting this and disabling the screenshot button, or just assume that the person taking the screenshot knows that these actions won't produce any meaningful output?

Comment 13

[Tim Nguyen :ntim]

(In reply to Léon McGregor from comment #12)
> Created attachment 8614065 [details] [diff] [review]
> bug1163332_addnodescreenshot.diff - ver 2
> 
> Ah, thanks. I get the impression I was over-complicating things, and this
> version of the patch should fix that.
> 
> Attached is a working patch based on the snippet you provided. However, I've
> noticed that you can take screenshots of <head>, elements of height 0 and
> similarly hidden-styled elements. 
> 
> This doesn't cause a crash, it just creates an empty .png file. Should I
> find a way of detecting this and disabling the screenshot button, or just
> assume that the person taking the screenshot knows that these actions won't
> produce any meaningful output?
It would probably make sense to disable/remove the menu item in that case. It should be possible to detect if an item is visible or not (we actually apply a lower opacity on items that are hidden).

Comment 14

[Léon McGregor]

Attachment: bug1163332_addnodescreenshot.diff - ver 3

This new patch should be ready to go.

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 8614660 [details] [diff] [review]
bug1163332_addnodescreenshot.diff - ver 3

Review of attachment 8614660 [details] [diff] [review]:
-----------------------------------------------------------------

This looks really good, thanks for cleaning up the code in inspector-panel.js, very easy to read now.
We need a test to make sure this will continue working in the future when the code changes.
We don't need a test that makes sure the screenshot is taken, that's the responsibility of the screenshot gcli command tests. We just need a test that verifies that:
- the menu item is here
- the menu item is disabled for non-displayed nodes
- the menu item is enabled otherwise
- ...
For this, I don't think you need to create a whole new test, adding new test cases to /browser/devtools/inspector/test/browser_inspector_menu-01-sensitivity.js should be enough.
There's some documentation about how to run the tests here: https://wiki.mozilla.org/DevTools/Hacking#DevTools_Mochitests

Comment 16

[Léon McGregor]

Attachment: bug1163332_addnodescreenshot.diff - ver 4

I've added the screenshot node to various parts of the existing tests and added tests for a few cases.

Comment 17

[Patrick Brosset <:pbro>]

Comment on attachment 8615385 [details] [diff] [review]
bug1163332_addnodescreenshot.diff - ver 4

Review of attachment 8615385 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there. I just have a comment about one corner case, explained below:

::: browser/devtools/inspector/inspector-panel.js
@@ +655,5 @@
>      let isEditableElement = isSelectionElement &&
>                              !this.selection.isAnonymousNode();
> +    let isScreenshotable = isSelectionElement &&
> +                           this.canGetUniqueSelector &&
> +                           this.selection.nodeFront.isDisplayed;

Unfortunately this isn't enough, a node may be displayed but inside a hidden node.
Try this:
- open about:home
- try to take a screenshot of the node #searchEngineLogo
The menu item is enabled because that node has its isDisplayed flag set to true, but since it's a child of a hidden node, it's really not displayed.
So you need to loop through the parents too.

I suggest adding the following handy getter in NodeFront (in toolkit/devtools/server/actors/inspector.js, in NodeFront, next to 'get isDisplayed'):

  get isTreeDisplayed() {
    let parent = this;
    while (parent) {
      if (!parent.isDisplayed) {
        return false;
      }
      parent = parent.parentNode();
    }
    return true;
  },

And then change, here, the condition to:

let isScreenshotable = isSelectionElement &&
                       this.canGetUniqueSelector &&
                       this.selection.nodeFront.isTreeDisplayed;

@@ +705,5 @@
>      if (!this.canGetUniqueSelector) {
>        unique.hidden = true;
>      }
>  
> +    if(isScreenshotable){

nit: please add a space between if and ( and between ) and {

@@ +1089,5 @@
> +    }).then(requisition => {
> +      requisition.updateExec("screenshot --selector " + this.selectionCssSelector);
> +    });
> +  },
> +  

nit: please remove these 2 trailing spaces.

::: browser/devtools/inspector/test/browser_inspector_menu-01-sensitivity.js
@@ +138,5 @@
> +  {
> +    desc: "<element> that isn't visible on the page, empty clipboard",
> +    selector: "#hiddenElement",
> +    disabled: PASTE_MENU_ITEMS.concat([
> +      "node-menu-copyimagedatauri", 

nit: please remove the extra trailing space.

@@ +141,5 @@
> +    disabled: PASTE_MENU_ITEMS.concat([
> +      "node-menu-copyimagedatauri", 
> +      "node-menu-screenshotnode",
> +    ]),
> +  },

Can you add one more test case that verifies that non-hidden elements inside hidden elements have the screenshotnode item disabled?

Comment 18

[Léon McGregor]

Attachment: bug1163332_addnodescreenshot.diff - ver 5

Fixed nits, added extra test case.

Comment 19

[Patrick Brosset <:pbro>]

Comment on attachment 8616660 [details] [diff] [review]
bug1163332_addnodescreenshot.diff - ver 5

Review of attachment 8616660 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for addressing the comments. This looks great.
We need to push to TRY to verify that all relevant tests still pass on the test platforms, but the server is closed for now, I will try again later.
Anyway, R+ from me. 
Can you modify the commit message to:
Bug 1163332 - Adds a 'screenshot this node' button to the inspector popup menu; r=pbrosset

Comment 20

[Patrick Brosset <:pbro>]

Reminder to push to try.

Comment 21

[Léon McGregor]

Attachment: bug1163332_addnodescreenshot.diff - ver 6

Added review info to bug commit message

Comment 22

[Patrick Brosset <:pbro>]

Comment on attachment 8617270 [details] [diff] [review]
bug1163332_addnodescreenshot.diff - ver 6

TRY is still closed right now.

Comment 23

[Patrick Brosset <:pbro>]

Pending try push:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e402f2c9399

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/846253a39b05

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/846253a39b05

Comment 26

[Lawrence Mandel [:lmandel] (use needinfo)]

Release Note Request (optional, but appreciated)
[Why is this notable]: New screenshot node/element context menu item (very cool)
[Suggested wording]: Screenshot a node or element from markup view with the Screenshot Node context menu item
[Links (documentation, blog post, etc)]:
https://www.evernote.com/l/AAF_RALHcBhMNrdNN4ui2PVtZklDut3WJasB/image.png

Comment 27

[Will Bamberg [:wbamberg]]

Updated https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_HTML#Element_popup_menu