Bug 174754 - Web Inspector: add context menu item for taking a screenshot of a node
Summary: Web Inspector: add context menu item for taking a screenshot of a node
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-07-22 02:19 PDT by Devin Rousso
Modified: 2020-05-22 21:17 PDT (History)
7 users (show)

See Also:


Attachments
Patch (9.90 KB, patch)
2017-07-22 02:28 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (229.80 KB, image/png)
2017-07-22 02:29 PDT, Devin Rousso
no flags Details
Patch (9.92 KB, patch)
2017-07-22 02:30 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (28.50 KB, patch)
2017-07-24 23:13 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (9.91 MB, application/zip)
2017-07-25 00:40 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2017-07-22 02:19:31 PDT
.
Comment 1 Devin Rousso 2017-07-22 02:28:49 PDT
Created attachment 316187 [details]
Patch
Comment 2 Devin Rousso 2017-07-22 02:29:02 PDT
Created attachment 316188 [details]
[Image] After Patch is applied
Comment 3 Devin Rousso 2017-07-22 02:30:53 PDT
Created attachment 316190 [details]
Patch
Comment 4 Matt Baker 2017-07-22 11:46:10 PDT
Screenshot of the result?
Comment 5 Devin Rousso 2017-07-22 12:46:47 PDT
(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
Comment 6 Joseph Pecoraro 2017-07-24 11:35:51 PDT
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.
Comment 7 BJ Burg 2017-07-24 11:43:39 PDT
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.
Comment 8 Devin Rousso 2017-07-24 23:13:06 PDT
Created attachment 316357 [details]
Patch
Comment 9 Build Bot 2017-07-25 00:40:40 PDT
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
Comment 10 Build Bot 2017-07-25 00:40:42 PDT
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
Comment 11 BJ Burg 2017-07-25 09:38:06 PDT
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..
Comment 12 WebKit Commit Bot 2017-07-25 10:07:25 PDT
Comment on attachment 316357 [details]
Patch

Clearing flags on attachment: 316357

Committed r219870: <http://trac.webkit.org/changeset/219870>
Comment 13 WebKit Commit Bot 2017-07-25 10:07:27 PDT
All reviewed patches have been landed.  Closing bug.