Bug 179329 - Web Inspector: Canvas Tab initial user experience is poor when no canvases exist
Summary: Web Inspector: Canvas Tab initial user experience is poor when no canvases exist
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: Matt Baker
URL:
Keywords: InRadar
Depends on:
Blocks: WebInspectorCanvasTab
  Show dependency treegraph
 
Reported: 2017-11-06 10:26 PST by Brian Burg
Modified: 2017-12-06 12:28 PST (History)
7 users (show)

See Also:


Attachments
Screenshot (217.48 KB, image/png)
2017-11-06 10:26 PST, Brian Burg
no flags Details
[Image] New "button help" UI (247.16 KB, image/png)
2017-12-04 16:03 PST, Matt Baker
no flags Details
Patch (14.10 KB, patch)
2017-12-04 16:27 PST, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (14.21 KB, patch)
2017-12-04 16:32 PST, Matt Baker
no flags Details | Formatted Diff | Diff
Patch for landing (14.23 KB, patch)
2017-12-06 11:56 PST, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2017-11-06 10:26:31 PST
Created attachment 326131 [details]
Screenshot

Visit a page without any canvases, then open Canvas Tab.

It just says "No this.. No that..".

We should have a better experience, perhaps some text that explains that canvases appear in a grid as they are found, but this page doesn't have any. It should also mention that you can import previous recordings and provide an inline button to click that shows this.

Lastly, this might be a good place to add a link to our reference documentation or an article about Canvas Tab. That is out of scope for now.
Comment 1 Radar WebKit Bug Importer 2017-12-04 16:00:06 PST
<rdar://problem/35842036>
Comment 2 Matt Baker 2017-12-04 16:03:15 PST
Created attachment 328402 [details]
[Image] New "button help" UI

New descriptive content in the empty Canvas tab overview, which surfaces the Import button from the sidebar.

(the Import button style is new, and will be applied to the sidebar in https://webkit.org/b/178744)
Comment 3 Matt Baker 2017-12-04 16:27:10 PST
Created attachment 328405 [details]
Patch
Comment 4 Matt Baker 2017-12-04 16:28:30 PST
Chrome and Firefox have similar UI in various locations to surface toolbar buttons, and provide helpful hints when no content is shown.
Comment 5 Matt Baker 2017-12-04 16:32:02 PST
Created attachment 328407 [details]
Patch
Comment 6 Matt Baker 2017-12-04 16:34:51 PST
(In reply to Brian Burg from comment #0)
> Created attachment 326131 [details]
> Screenshot
> 
> Visit a page without any canvases, then open Canvas Tab.
> 
> It just says "No this.. No that..".
> 
> We should have a better experience, perhaps some text that explains that
> canvases appear in a grid as they are found, but this page doesn't have any.
> It should also mention that you can import previous recordings and provide
> an inline button to click that shows this.

This builds on https://webkit.org/b/179330, which added explanatory text to the content view. This addresses the part about mentioning the ability to import previous recordings, by surfacing the "Import" button from the sidebar.
Comment 7 Timothy Hatcher 2017-12-05 23:09:56 PST
Comment on attachment 328407 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:13
> +        This patch adds a new method, WI.createNavigationItemHelp, for creating command
> +        help to display in a message text view. The method accepts a format string and
> +        NavigationItem, and returns a DOM element. For example,
> +
> +            WI.createNavigationItemHelp("Press %s to do X.", navigationItem)

Neat!

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:703
> +localizedStrings["Press %s to load a recording from file."] = "Press %s to load a recording from file.";

This could be hard to localize because of the wording in the help button replacement, but I'm not sure what would make it better.

> Source/WebInspectorUI/UserInterface/Base/Main.js:2271
> +    String.format(formatString, [wrapperElement], String.standardFormatters, containerElement, append);

Clever.

> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.js:39
> +        let importNavigationItem = new WI.ButtonNavigationItem("import-recording", WI.UIString("Import"), "Images/Import.svg", 15, 15);
> +        importNavigationItem.buttonStyle = WI.ButtonNavigationItem.Style.ImageAndText;
> +        importNavigationItem.addEventListener(WI.ButtonNavigationItem.Event.Clicked, () => { WI.canvasManager.importRecording(); });

Anyway to reuse the real ButtonNavigationItem from RecordingNavigationSidebarPanel?

> Source/WebInspectorUI/UserInterface/Views/Main.css:204
> +.message-text-view .navigation-item-help .navigation-bar > .item {

Text size being the same larger size as the body of the message bothers me. I kind of expect this text to exactly match the navigation bar text.
Comment 8 Matt Baker 2017-12-06 11:48:14 PST
Comment on attachment 328407 [details]
Patch

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

>> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:703
>> +localizedStrings["Press %s to load a recording from file."] = "Press %s to load a recording from file.";
> 
> This could be hard to localize because of the wording in the help button replacement, but I'm not sure what would make it better.

It does mean that both the help string and any formatted navigation items may need to be considered together when localizing. This does add an additional burden, but how big a burden I couldn't say. I expect that even without this change, many UI strings cannot be localized outside of some context (other UI strings).

>> Source/WebInspectorUI/UserInterface/Base/Main.js:2271
>> +    String.format(formatString, [wrapperElement], String.standardFormatters, containerElement, append);
> 
> Clever.

I was surprised how easy this was!

>> Source/WebInspectorUI/UserInterface/Views/CanvasOverviewContentView.js:39
>> +        importNavigationItem.addEventListener(WI.ButtonNavigationItem.Event.Clicked, () => { WI.canvasManager.importRecording(); });
> 
> Anyway to reuse the real ButtonNavigationItem from RecordingNavigationSidebarPanel?

It would be awkward from a layering point of view to expose the button to the content view. The big issue is that if the Import button text/image/style changes, it now needs to be updated in multiple places. I've been thinking it would be useful to have a CommandController (accessible from AppController) that has entries for all frontend UI commands. If we ever wanted to add a command palette to the Inspector (Chrome, most editors) this would be a step in that direction.

>> Source/WebInspectorUI/UserInterface/Views/Main.css:204
>> +.message-text-view .navigation-item-help .navigation-bar > .item {
> 
> Text size being the same larger size as the body of the message bothers me. I kind of expect this text to exactly match the navigation bar text.

This was unintentional, good catch! A nice side effect is that fixing the font size makes the -1px adjustment unnecessary.
Comment 9 Matt Baker 2017-12-06 11:53:43 PST
Comment on attachment 328407 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/Main.css:204
>>> +.message-text-view .navigation-item-help .navigation-bar > .item {
>> 
>> Text size being the same larger size as the body of the message bothers me. I kind of expect this text to exactly match the navigation bar text.
> 
> This was unintentional, good catch! A nice side effect is that fixing the font size makes the -1px adjustment unnecessary.

I spoke too soon. Reducing the font to 11px requires 1px of vertical padding to make the button look good, which makes the -1px still necessary.
Comment 10 Matt Baker 2017-12-06 11:56:03 PST
Created attachment 328602 [details]
Patch for landing
Comment 11 WebKit Commit Bot 2017-12-06 12:28:30 PST
Comment on attachment 328602 [details]
Patch for landing

Clearing flags on attachment: 328602

Committed r225587: <https://trac.webkit.org/changeset/225587>
Comment 12 WebKit Commit Bot 2017-12-06 12:28:32 PST
All reviewed patches have been landed.  Closing bug.