Bug 194279

Summary: Web Inspector: provide a way to capture a screenshot of a node from within the page
Product: WebKit Reporter: Devin Rousso <drousso>
Component: Web InspectorAssignee: Devin Rousso <drousso>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, drousso, ews, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, mkwst, msaboff, rniwa, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
[Image] After Patch is applied
none
Archive of layout-test-results from ews104 for mac-highsierra-wk2
none
Archive of layout-test-results from ews102 for mac-highsierra
none
Archive of layout-test-results from ews113 for mac-highsierra
none
Archive of layout-test-results from ews121 for ios-simulator-wk2
none
Archive of layout-test-results from ews201 for win-future
none
Patch
none
Archive of layout-test-results from ews102 for mac-highsierra
none
Archive of layout-test-results from ews106 for mac-highsierra-wk2
none
Archive of layout-test-results from ews113 for mac-highsierra
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Archive of layout-test-results from ews201 for win-future
none
Patch
none
Patch
none
[Image] After Patch is applied
none
Archive of layout-test-results from ews201 for win-future
none
Patch none

Description Devin Rousso 2019-02-04 22:45:06 PST
Similar to the "Capture Screenshot" DOM tree context menu item, there should be a way to visualize a screenshot from within the console.
Comment 1 Devin Rousso 2019-02-04 22:45:16 PST
<rdar://problem/10731573>
Comment 2 Devin Rousso 2019-02-04 23:07:04 PST
Created attachment 361160 [details]
Patch

There is a lot of opportunity to expand `console.screenshot` to include other types (e.g. CanvasRenderingContext, Pattern (Canvas2D), Gradient (Canvas2D), ImageBitmap, ImageData, Texture (WebGL), etc.)
Comment 3 Devin Rousso 2019-02-04 23:09:14 PST
Created attachment 361161 [details]
[Image] After Patch is applied
Comment 4 Build Bot 2019-02-04 23:09:38 PST
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment 5 Devin Rousso 2019-02-04 23:10:22 PST
(In reply to Devin Rousso from comment #3)
> Created attachment 361161 [details]
> [Image] After Patch is applied
I'm not set on having the shadow, but I'm a fan of it from the Canvas tab and I thought it was a nice way to differentiate the fact that this isn't a "normal" log result.
Comment 6 Build Bot 2019-02-05 00:06:19 PST Comment hidden (obsolete)
Comment 7 Build Bot 2019-02-05 00:06:21 PST Comment hidden (obsolete)
Comment 8 Build Bot 2019-02-05 00:12:51 PST Comment hidden (obsolete)
Comment 9 Build Bot 2019-02-05 00:12:53 PST Comment hidden (obsolete)
Comment 10 Build Bot 2019-02-05 00:47:43 PST Comment hidden (obsolete)
Comment 11 Build Bot 2019-02-05 00:47:44 PST Comment hidden (obsolete)
Comment 12 Build Bot 2019-02-05 00:49:12 PST Comment hidden (obsolete)
Comment 13 Build Bot 2019-02-05 00:49:14 PST Comment hidden (obsolete)
Comment 14 Build Bot 2019-02-05 01:49:44 PST Comment hidden (obsolete)
Comment 15 Build Bot 2019-02-05 01:49:55 PST Comment hidden (obsolete)
Comment 16 Devin Rousso 2019-02-05 10:00:13 PST
Comment on attachment 361160 [details]
Patch

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

> Source/JavaScriptCore/inspector/protocol/Page.json:349
> +            "name": "consoleScreenshotted",

Rather than create a new event on the `Page` domain specifically for `console.screenshot`, we could also add "image" to `Console.ConsoleMessage.Type` and leverage the existing console message channels.  I will try that approach, as it requires less custom infrastructure to support.
Comment 17 Joseph Pecoraro 2019-02-05 10:49:35 PST
Comment on attachment 361160 [details]
Patch

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

I think it would be worth adding a `screenshot()` to the CommandLineAPI as well. It should be as easy as:

    screenshot: function()
    {
        return inspectedWindow.console.screenshot.apply(inspectedWindow.console, arguments)
    },

And adding the "screenshot" string in a few places (just like "queryObjects")

    WebInspectorUI/UserInterface/Controllers/JavaScriptRuntimeCompletionProvider.js
    321:    "queryObjects",

    WebCore/inspector/CommandLineAPIModuleSource.js
    85:    "queryObjects",

>> Source/JavaScriptCore/inspector/protocol/Page.json:349
>> +            "name": "consoleScreenshotted",
> 
> Rather than create a new event on the `Page` domain specifically for `console.screenshot`, we could also add "image" to `Console.ConsoleMessage.Type` and leverage the existing console message channels.  I will try that approach, as it requires less custom infrastructure to support.

I like the sound of that better. It seems weird to me that a Console API would interact with the Page domain but I realize `console.takeHeapSnapshot` and `console.record` do this with other domains though.

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:303
> +                let date = new Date;
> +                let values = [
> +                    date.getFullYear(),
> +                    Number.zeroPad(date.getMonth() + 1, 2),
> +                    Number.zeroPad(date.getDate(), 2),
> +                    Number.zeroPad(date.getHours(), 2),
> +                    Number.zeroPad(date.getMinutes(), 2),
> +                    Number.zeroPad(date.getSeconds(), 2),
> +                ];
> +                let filename = WI.UIString("Screen Shot %s-%s-%s at %s.%s.%s").format(...values);

Given this is now in a few places, should this go in Utilities / ImageUtilities?
Comment 18 Brian Burg 2019-02-05 10:49:52 PST
Comment on attachment 361160 [details]
Patch

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

LGTM, let's try folding into the other console events though.

> Source/JavaScriptCore/inspector/JSGlobalObjectConsoleClient.cpp:171
> +void JSGlobalObjectConsoleClient::screenshot(ExecState*, Ref<ScriptArguments>&&) { }

Shouldn't this be stubbed with warnUnimplemented for JSContexts?

>> Source/JavaScriptCore/inspector/protocol/Page.json:349
>> +            "name": "consoleScreenshotted",
> 
> Rather than create a new event on the `Page` domain specifically for `console.screenshot`, we could also add "image" to `Console.ConsoleMessage.Type` and leverage the existing console message channels.  I will try that approach, as it requires less custom infrastructure to support.

I agree.

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

This may not be suitable for localization if a different locale needs to use different date methods/formatting. Let's do this for now, hard to know for sure until someone tries it ;-)

> LayoutTests/inspector/console/console-screenshot.html:15
> +        description: "Test that console.screenshot is works with nodes.",

Nit: is works
Comment 19 Brian Burg 2019-02-05 10:52:21 PST
Comment on attachment 361160 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:959
> +        let contextMenu = WI.ContextMenu.createFromEvent(event);

It would be nice to have a "Show Element" context menu item to jump to the Elements Tab. You'd need to plumb the element as an argument, somehow.
Comment 20 Brian Burg 2019-02-05 10:52:38 PST
Comment on attachment 361160 [details]
Patch

Clearing r?.
Comment 21 Devin Rousso 2019-02-06 14:05:53 PST
Created attachment 361326 [details]
Patch
Comment 22 Build Bot 2019-02-06 15:07:29 PST Comment hidden (obsolete)
Comment 23 Build Bot 2019-02-06 15:07:31 PST Comment hidden (obsolete)
Comment 24 Build Bot 2019-02-06 15:22:25 PST Comment hidden (obsolete)
Comment 25 Build Bot 2019-02-06 15:22:27 PST Comment hidden (obsolete)
Comment 26 Build Bot 2019-02-06 15:57:31 PST Comment hidden (obsolete)
Comment 27 Build Bot 2019-02-06 15:57:33 PST Comment hidden (obsolete)
Comment 28 Build Bot 2019-02-06 16:07:38 PST Comment hidden (obsolete)
Comment 29 Build Bot 2019-02-06 16:07:40 PST Comment hidden (obsolete)
Comment 30 Build Bot 2019-02-06 16:55:42 PST Comment hidden (obsolete)
Comment 31 Build Bot 2019-02-06 16:55:54 PST Comment hidden (obsolete)
Comment 32 Devin Rousso 2019-02-06 18:17:02 PST
Created attachment 361358 [details]
Patch
Comment 33 Joseph Pecoraro 2019-03-14 00:40:02 PDT
Comment on attachment 361358 [details]
Patch

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

> Source/WebCore/page/PageConsoleClient.cpp:281
> +    if (dataURL.isEmpty())
> +        return;

Should we send a log message that we failed to snapshot <object> so it at least logs the object instead of doing nothing with it?

> Source/WebCore/page/PageConsoleClient.cpp:286
> +    // Log the argument before sending the image for it.
> +    String messageText;
> +    arguments->getFirstArgumentAsString(messageText);
> +    addMessage(std::make_unique<Inspector::ConsoleMessage>(MessageSource::ConsoleAPI, MessageType::Log, MessageLevel::Log, messageText, arguments.copyRef()));

I'm not sure I understand this. I think this makes sense if the user provides an element, so we would log the element and then have the screenshot right after. However your Image attachment doesn't show that, it just shows a screenshot after the line. So what is this for?

I think this is the part we might want to think more carefully about. Especially if we expect other browsers to follow suit we should better understand what we want to do here.

It seems like:

    console.screenshot(node);

Is becoming something like:

    log(Log, node, ...);
    log(Image, node.toDataURL())

It seems like maybe we should just be sending a single message with the image data and arguments.

    logImage(Image, node.toDataURL(), node, ...)

Which the frontend can decide to do whatever it wants with all arguments.

Likewise, console.screenshot seems to only care about the first argument. What if you're given a NodeList or set of elements, or no elements (something like the entire page):

     console.screenshot(document.querySelectorAll(".active"));
     console.screenshot(elem1, elem2);
     console.screenshot();

Should we support these use cases? If not, why not? Maybe it would be hard to make this fit in the console in a good way?

It is important that we try to do things reasonably well when first introducing a new API like this. I think limiting to a single node is fine but it would be nice to justify why. I think `console.screenshot()` capturing the entire page (document.documentElement / document.body) is reasonable as well.

> Source/WebInspectorUI/UserInterface/Models/ConsoleMessage.js:62
> -    get messageText() { return this._messageText; }
> +    get data() { return this._data; }

I almost think `content` might better than `text`/`data`. We use `content` for resource bodies.

> Source/WebInspectorUI/UserInterface/Models/NativeFunctionParameters.js:178
> +        screenshot: "object",

I think we'd rather say "node" instead of "object".

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.css:76
> +    max-width: 500px;
> +    max-height: 500px;

Why limit the max size?

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:295
> +                image.title = filename;

Given we generate this string why would we want to give it a tooltip of this generated string? It doesn't seem useful. If we wanted a way to get the timestamp of messages we would do that differently.

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:948
> +        let image = this._messageBodyElement.querySelector("img");

This seems a little risky. What if `console.trace`, or ObjectTreeView have an <img> somewhere. Maybe they aren't in the body element, but maybe something could. It might be better to include a class name on the <img> we create for Console Images. `.console-image` seems appropriate.
Comment 34 Devin Rousso 2019-03-14 01:06:50 PDT
Comment on attachment 361358 [details]
Patch

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

>> Source/WebCore/page/PageConsoleClient.cpp:281
>> +        return;
> 
> Should we send a log message that we failed to snapshot <object> so it at least logs the object instead of doing nothing with it?

Sure.

>> Source/WebCore/page/PageConsoleClient.cpp:286
>> +    addMessage(std::make_unique<Inspector::ConsoleMessage>(MessageSource::ConsoleAPI, MessageType::Log, MessageLevel::Log, messageText, arguments.copyRef()));
> 
> I'm not sure I understand this. I think this makes sense if the user provides an element, so we would log the element and then have the screenshot right after. However your Image attachment doesn't show that, it just shows a screenshot after the line. So what is this for?
> 
> I think this is the part we might want to think more carefully about. Especially if we expect other browsers to follow suit we should better understand what we want to do here.
> 
> It seems like:
> 
>     console.screenshot(node);
> 
> Is becoming something like:
> 
>     log(Log, node, ...);
>     log(Image, node.toDataURL())
> 
> It seems like maybe we should just be sending a single message with the image data and arguments.
> 
>     logImage(Image, node.toDataURL(), node, ...)
> 
> Which the frontend can decide to do whatever it wants with all arguments.
> 
> Likewise, console.screenshot seems to only care about the first argument. What if you're given a NodeList or set of elements, or no elements (something like the entire page):
> 
>      console.screenshot(document.querySelectorAll(".active"));
>      console.screenshot(elem1, elem2);
>      console.screenshot();
> 
> Should we support these use cases? If not, why not? Maybe it would be hard to make this fit in the console in a good way?
> 
> It is important that we try to do things reasonably well when first introducing a new API like this. I think limiting to a single node is fine but it would be nice to justify why. I think `console.screenshot()` capturing the entire page (document.documentElement / document.body) is reasonable as well.

You are correct in that this would log the "target" and then show the screenshot for the "target" immediately after.  The Image attachment was for an older patch, which didn't yet have this.

As far as logging the "target", I'm not really that set-in-stone on the idea of doing that, but it seemed like it would be especially nice for cases where `console.screenshot` was in a script (vs called in the prompt) so you could see where the screenshot "came from".

Trying to capture a screenshot of multiple things doesn't make a lot of sense to me.  I'm somewhat "modeling" this off of how the system takes screenshots of windows (⌘⇧4, and then Space), in that it only can do one at a time and it's very highly defined/constrained to a window (for `console.screenshot`, it'd be a node).

I do think that calling `console.screenshot()` with no arguments is a good idea, but I think it would be more interesting/useful to have that capture exactly what is visible from the viewport (as if you'd taken a picture of your screen).  If someone wanted to screenshot the entire page (including the parts offscreen), I think it's fair to ask them to pass `document.documentElement`/`document.body` to get that functionality is reasonable as well.

>> Source/WebInspectorUI/UserInterface/Models/ConsoleMessage.js:62
>> +    get data() { return this._data; }
> 
> I almost think `content` might better than `text`/`data`. We use `content` for resource bodies.

Considering that the data/content for all message types is still just a string, I think I'll change this back to `messageText` so it makes the diff smaller.  I was originally going to try to change the payload to actually use an object, but decided not to since that would probably be way more intense.

>> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.css:76
>> +    max-height: 500px;
> 
> Why limit the max size?

I was running into a lot of issues trying to get things to play nicely with a maximum width/height, especially if the image is larger than the inspector window itself (this is very likely on higher DPI screens as well).  It doesn't make much sense to me to allow an image to scale itself to be the full height of the inspector window.  If you want to see the image at a bigger size (or zoom/pan/rotate/etc), it's very easy to download it and do that natively.

>> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:295
>> +                image.title = filename;
> 
> Given we generate this string why would we want to give it a tooltip of this generated string? It doesn't seem useful. If we wanted a way to get the timestamp of messages we would do that differently.

I don't understand what you mean.  Is there an existing way of getting the time when a message was added?  I do agree that we should probably be calculating the timestamp when the `WI.ConsoleMessage` is constructed, but I don't see why we wouldn't want to set that as a tooltip.
Comment 35 Devin Rousso 2019-03-14 01:27:50 PDT
Comment on attachment 361358 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:948
>> +        let image = this._messageBodyElement.querySelector("img");
> 
> This seems a little risky. What if `console.trace`, or ObjectTreeView have an <img> somewhere. Maybe they aren't in the body element, but maybe something could. It might be better to include a class name on the <img> we create for Console Images. `.console-image` seems appropriate.

This shouldn't be an issue because the "contextmenu" listener is only added for `WI.ConsoleMessage.MessageType.Image`.  I'll still change it though.
Comment 36 Joseph Pecoraro 2019-03-14 01:41:34 PDT
Comment on attachment 361358 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:295
>>> +                image.title = filename;
>> 
>> Given we generate this string why would we want to give it a tooltip of this generated string? It doesn't seem useful. If we wanted a way to get the timestamp of messages we would do that differently.
> 
> I don't understand what you mean.  Is there an existing way of getting the time when a message was added?  I do agree that we should probably be calculating the timestamp when the `WI.ConsoleMessage` is constructed, but I don't see why we wouldn't want to set that as a tooltip.

There isn't an existing way to get the timestamps today. I was suggesting that if something did exist it should be consistent everywhere (such as a date line in the console per each message, or tooltip on the output icon). In any case I would not expect to see a tooltip on the image, particularly this one which doesn't have useful content.

>>> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:948
>>> +        let image = this._messageBodyElement.querySelector("img");
>> 
>> This seems a little risky. What if `console.trace`, or ObjectTreeView have an <img> somewhere. Maybe they aren't in the body element, but maybe something could. It might be better to include a class name on the <img> we create for Console Images. `.console-image` seems appropriate.
> 
> This shouldn't be an issue because the "contextmenu" listener is only added for `WI.ConsoleMessage.MessageType.Image`.  I'll still change it though.

Ahh good point. This does bring to mind that maybe we want another output indicate instead of the same as `console.log` / `console.trace`. The 10x10 icons are tough though. Jon Davis made the existing suite. Maybe a [o] like icon (box with a circle in the middle sorta like a "frame" around an image) might work for screenshot.
Comment 37 Devin Rousso 2019-03-14 02:05:40 PDT
Created attachment 364646 [details]
Patch
Comment 38 Devin Rousso 2019-03-14 02:06:21 PDT
Created attachment 364647 [details]
[Image] After Patch is applied
Comment 39 Build Bot 2019-03-14 04:13:26 PDT Comment hidden (obsolete)
Comment 40 Build Bot 2019-03-14 04:13:39 PDT Comment hidden (obsolete)
Comment 41 Joseph Pecoraro 2019-03-14 22:30:35 PDT
Comment on attachment 364646 [details]
Patch

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

r=me!

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:292
> +                let image = element.appendChild(document.createElement("img"));

Style: I think we normally use `img` to match the element. Up to you.

> Source/WebInspectorUI/UserInterface/Views/ConsoleMessageView.js:294
> +                image.src = this._message.messageText;

Maybe we could use srcset to avoid having to do the logic in a "load" event? But as is seems fine.

> LayoutTests/inspector/console/console-screenshot.html:30
> +            InspectorTest.evaluateInPage(`console.screenshot(document.querySelector("#test1"))`)

We should have some more tests:

    • Node that is not attached to the DOM `let e = document.createElement('a'); e.textContent = "test"; console.screenshot(e)`
    • Multiple nodes `console.screenshot(a, b)`
    • Screenshot with no arguments produces an expected width/height (I think 800x600 or something is the LayoutTest size
Comment 42 Devin Rousso 2019-03-14 23:16:35 PDT
Created attachment 364776 [details]
Patch
Comment 43 WebKit Commit Bot 2019-03-15 01:12:27 PDT
Comment on attachment 364776 [details]
Patch

Clearing flags on attachment: 364776

Committed r242992: <https://trac.webkit.org/changeset/242992>
Comment 44 WebKit Commit Bot 2019-03-15 01:12:30 PDT
All reviewed patches have been landed.  Closing bug.