Bug 174754 – Web Inspector: add context menu item for taking a screenshot of a node

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

.

[Devin Rousso]

Created attachment 316187 [details]
Patch

[Devin Rousso]

Created attachment 316188 [details]
[Image] After Patch is applied

[Devin Rousso]

Created attachment 316190 [details]
Patch

[Matt Baker]

Screenshot of the result?

[Devin Rousso]

(In reply to Matt Baker from comment #4)
> Screenshot of the result?

The file is downloaded to wherever the user selects with the same name format as the macOS screenshot.

Screen Shot ${year}-${month}-${day} at ${hours}.${minutes}.${seconds}.png

[Joseph Pecoraro]

Comment on attachment 316190 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316190&action=review

r- for lack of test for the utility function. But also cause I wanted to see a version 2.

Were we going to add a full page screenshot button in the navigation bar? Or is that going to be done separately?

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:1131
> +Object.defineProperty(Number, "pad",
> +{
> +    value(num, length)
> +    {
> +        let string = num.toLocaleString();
> +        while (string.length < length)
> +            string = "0" + string;
> +        return string;
> +    },
> +});

String.prototype.padStart already exists and is standard. It can be used here instead of the while loop.

How about making this Number.zeroPad / Number.prototype.zeroPad?

    let string = num.toLocaleString();
    return string.padStart(length, "0");

This should have a test in:
LayoutTests/inspector/unit-tests/number-utilities.html

> Source/WebInspectorUI/UserInterface/Models/DOMNode.js:468
> +            function scrollIntoView()

Since we are moving this, lets fix the name of this inner function to be:

    function inspectedPage_node_scrollIntoView

That is because this function is evaluated in the inspected page and the `this` value is the node.

We name these specially so (1) they are easy to find and (2) we are aware that we have to be backwards compatible with older iOS releases for the contents of these functions. We can't use `let` for instance (we support back to iOS 7).

> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:203
> +    contextMenu.appendItem(WebInspector.UIString("Screenshot Node"), () => {

We should wrap this in:

    if (window.PageAgent) {
        ...
    }

I don't think there exists a case where it is possible to have DOMNodes without a PageAgent, but lets do it anyways to be safe.

> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:218
> +                // url: "web-inspector:///Node.png",

Stale comment it seems.

[Blaze Burg]

Comment on attachment 316190 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316190&action=review

After taking my comments, please test saving a screenshot with Arabic system language. It should match the same filename format as a normal screen capture.

> Source/WebInspectorUI/ChangeLog:19
> +        Add Number.pad to add leading zeros up to a certain width.

It might be better to add support for pad to our String.format implementation.

>> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:203
>> +    contextMenu.appendItem(WebInspector.UIString("Screenshot Node"), () => {
> 
> We should wrap this in:
> 
>     if (window.PageAgent) {
>         ...
>     }
> 
> I don't think there exists a case where it is possible to have DOMNodes without a PageAgent, but lets do it anyways to be safe.

Two nits: 

1) "Screenshot" is not a verb, at least not enough in common usage that it is suitable for a menu item
2) Most people don't know node vs element, so just calling it Element is fine.

Together, I would prefer "Capture Element Screenshot" or "Take Element Screenshot". Either of these are workable with a similar "Take Viewport/Page Screenshot" context menu items.

> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:205
> +            if (error)

We should log an error to console so the user knows why nothing showed up.

> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:208
> +            let date = new Date;

Unfortunately we can't replicate what Cocoa does directly in JS because the Intl API doesn't do what NSDateFormatter does with arbitrary format strings. We can get around this, however, by using WebInspector.UIString("Screen Shot %d-%d-%d at %d.%d.%d").format(...). This will get us proper localization because UIString("...").format(...) will use the correct number characters for Arabic locales. AFAIK, in all other locales the date format is the same even if it's not what would be produced by default in the locale. For example, `new Date.toLocaleDateString()` will get you mm-dd-yyyy, instead of yyyy-mm-dd as the system does for whatever reason.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:-1524
> -    _scrollIntoView()

Nice cleanup.

[Devin Rousso]

Created attachment 316357 [details]
Patch

[Build Bot]

Comment on attachment 316357 [details]
Patch

Attachment 316357 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4182945

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html

[Build Bot]

Created attachment 316359 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Blaze Burg]

Comment on attachment 316357 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316357&action=review

r=me

> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:213
> +                    WebInspector.consoleLogViewController.appendConsoleMessage(consoleMessage);

Very nice, thanks.

> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:226
> +                let filename = WebInspector.UIString("Screen Shot %s-%s-%s at %s.%s.%s").format(...values);

Does .format do the right thing for %s? Hmm..

[WebKit Commit Bot]

Comment on attachment 316357 [details]
Patch

Clearing flags on attachment: 316357

Committed r219870: <http://trac.webkit.org/changeset/219870>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.