1446944 - Provide on-boarding tooltip for 3-pane feature

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

[Martin Balfanz [:mbalfanz]]

To help users to discover the split rule view, we should add an on-boarding tooltip.

The tooltip should show up once and announce the new feature. If the user dismisses it (by clicking the X), it should not show up again.

The copy and the destination of the link are still to be defined.

Mockups: https://mozilla.invisionapp.com/share/Z3F7OGCTK#/screens/284159829

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 1446944 - Provide onboarding tooltip for the 3 pane inspector feature.

Review commit: https://reviewboard.mozilla.org/r/240834/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240834/

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8972144 [details]
Bug 1446944 - Provide onboarding tooltip for the 3 pane inspector feature.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240834/diff/1-2/

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8972144 [details]
Bug 1446944 - Provide onboarding tooltip for the 3 pane inspector feature.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240834/diff/2-3/

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=79e6d60b8a6875dcf9e48b1bd666885009a422e6

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

I should note dt failures in this try is caused by other patches and not this particular one.

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8972144 [details]
Bug 1446944 - Provide onboarding tooltip for the 3 pane inspector feature.

https://reviewboard.mozilla.org/r/240834/#review246786

Thanks, some comments but overall looks good. 
R+ with a green try + comments addressed.

::: devtools/client/inspector/inspector.js:154
(Diff revision 3)
>    this.onSidebarSelect = this.onSidebarSelect.bind(this);
>    this.onSidebarShown = this.onSidebarShown.bind(this);
>    this.onSidebarToggle = this.onSidebarToggle.bind(this);
>  
>    this._target.on("will-navigate", this._onBeforeNavigate);
> +  this.prefsObserver.on(SHOW_THREE_PANE_ONBOARDING_PREF,

I am not sure to understand why we listen to preference changes here. We only ever read the variable during the inspector initialization.

Could we just read the value once and remove the pref listener here?

If you decide to keep it, we should remove the listener in the destroy (prefsObserver.destroy() does not take care of this).

(on a sidenote, this prefsObserver looks like a leftover no longer used by anything else in this class)

::: devtools/client/inspector/shared/three-pane-onboarding-tooltip.js:55
(Diff revision 3)
> +    content.append(message);
> +
> +    const learnMoreLink = doc.createElementNS(XHTML_NS, "a");
> +    learnMoreLink.className = "theme-link";
> +    learnMoreLink.href = "#";
> +    content.append(learnMoreLink);

This looks like it's missing some text?
Probably inspector.threePaneOnboarding.learnMore ?

::: devtools/client/locales/en-US/inspector.properties:468
(Diff revision 3)
> +inspector.threePaneOnboarding.new=New:
> +inspector.threePaneOnboarding.content=3-pane mode lets you see both CSS rules and Layout tools. Click this button to toggle.
> +inspector.threePaneOnboarding.learnMore=Learn more

They all should have localization notes. Could you flag :flod for review after you have added the notes?

::: devtools/client/themes/tooltips.css:489
(Diff revision 3)
> +/* Tooltip: 3 Pane Inspecot Onboarding Tooltip */
> +
> +.three-pane-onboarding-container {
> +  align-items: center;
> +  background-color: var(--theme-toolbar-background);
> +  border-bottom: 1px solid var(--theme-splitter-color);

We should add `color: var(--theme-body-color);` as well otherwise the text will be too dark in the dark theme.

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8972144 [details]
Bug 1446944 - Provide onboarding tooltip for the 3 pane inspector feature.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240834/diff/3-4/

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8972144 [details]
Bug 1446944 - Provide onboarding tooltip for the 3 pane inspector feature.

Gonna ask for a re-review since I finished the learn more link.

Comment 9

[Code Review Bot [:reviewbot]]

Comment on attachment 8972144 [details]
Bug 1446944 - Provide onboarding tooltip for the 3 pane inspector feature.

https://reviewboard.mozilla.org/r/240834/#review247008


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/inspector.js:16
(Diff revision 4)
>  const Services = require("Services");
>  const promise = require("promise");
>  const EventEmitter = require("devtools/shared/event-emitter");
>  const {executeSoon} = require("devtools/shared/DevToolsUtils");
>  const {Toolbox} = require("devtools/client/framework/toolbox");
>  const {PrefObserver} = require("devtools/client/shared/prefs");

Error: 'prefobserver' is assigned a value but never used. [eslint: no-unused-vars]

::: devtools/client/inspector/shared/three-pane-onboarding-tooltip.js:59
(Diff revision 4)
> +    content.append(message);
> +
> +    this.learnMoreLink = doc.createElementNS(XHTML_NS, "a");
> +    this.learnMoreLink.className = "theme-link";
> +    this.learnMoreLink.href = "#";
> +    this.learnMoreLink.textContent = L10N.getStr("inspector.threePaneOnboarding.learnMore");

Error: Line 59 exceeds the maximum line length of 90. [eslint: max-len]

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8972144 [details]
Bug 1446944 - Provide onboarding tooltip for the 3 pane inspector feature.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240834/diff/4-5/

Comment 11

[Code Review Bot [:reviewbot]]

Comment on attachment 8972144 [details]
Bug 1446944 - Provide onboarding tooltip for the 3 pane inspector feature.

https://reviewboard.mozilla.org/r/240834/#review247012


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/shared/three-pane-onboarding-tooltip.js:59
(Diff revision 5)
> +    content.append(message);
> +
> +    this.learnMoreLink = doc.createElementNS(XHTML_NS, "a");
> +    this.learnMoreLink.className = "theme-link";
> +    this.learnMoreLink.href = "#";
> +    this.learnMoreLink.textContent = 

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

Comment 12

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8972144 [details]
Bug 1446944 - Provide onboarding tooltip for the 3 pane inspector feature.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240834/diff/5-6/

Comment 13

[Julian Descottes [:jdescottes]]

Comment on attachment 8972144 [details]
Bug 1446944 - Provide onboarding tooltip for the 3 pane inspector feature.

https://reviewboard.mozilla.org/r/240834/#review247040

Looks good, some comments to fix before landing.

::: devtools/client/inspector/inspector.js
(Diff revision 6)
>  const Services = require("Services");
>  const promise = require("promise");
>  const EventEmitter = require("devtools/shared/event-emitter");
>  const {executeSoon} = require("devtools/shared/DevToolsUtils");
>  const {Toolbox} = require("devtools/client/framework/toolbox");
> -const {PrefObserver} = require("devtools/client/shared/prefs");

thanks for the cleanup!

::: devtools/client/inspector/shared/three-pane-onboarding-tooltip.js:18
(Diff revision 6)
> +
> +const SHOW_THREE_PANE_ONBOARDING_PREF = "devtools.inspector.show-three-pane-tooltip";
> +
> +const XHTML_NS = "http://www.w3.org/1999/xhtml";
> +const CONTAINER_WIDTH = 300;
> +const LEARN_MORE_LINK = "https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/Use_the_3-pane_inspector";

Should we have campaign parameters for analytics? To check w/ Harald?

::: devtools/client/inspector/shared/three-pane-onboarding-tooltip.js:30
(Diff revision 6)
> +  constructor(toolbox, doc) {
> +    this.toolbox = toolbox;
> +    this.doc = doc;
> +    this.tooltip = new HTMLTooltip(this.toolbox.doc, {
> +      type: "arrow",
> +      useXulWrapper: true,

(╯°□°）╯︵ ┻━┻

::: devtools/client/inspector/shared/three-pane-onboarding-tooltip.js:71
(Diff revision 6)
> +
> +    this.closeButton.addEventListener("click", this.onCloseButtonClick);
> +    this.learnMoreLink.addEventListener("click", this.onLearnMoreLinkClick);
> +
> +    this.tooltip.setContent(container, {
> +      position: "top",

setContent doesn't take a position argument. It will favor the anchor with the most available space. If you want to enforce top, you must pass {position: "top"} to show():

  .show(anchorEl, {position: "top"})

::: devtools/client/inspector/shared/three-pane-onboarding-tooltip.js:106
(Diff revision 6)
> +   * and opens the link to the mdn page in a new tab.
> +   */
> +  onLearnMoreLinkClick() {
> +    Services.prefs.setBoolPref(SHOW_THREE_PANE_ONBOARDING_PREF, false);
> +    this.tooltip.hide();
> +    openWebLink(LEARN_MORE_LINK, this.toolbox);

Looks like our new helper doesn't work correctly if DevTools are undocked. Logged https://bugzilla.mozilla.org/show_bug.cgi?id=1458591

::: devtools/client/themes/images/fox-smiling.svg:1
(Diff revision 6)
> +<svg version="1.0" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"

License header missing

Comment 14

[Julian Descottes [:jdescottes]]

Comment on attachment 8972144 [details]
Bug 1446944 - Provide onboarding tooltip for the 3 pane inspector feature.

Francesco, could you check the properties file update here? Thanks!

Comment 15

[Francesco Lodolo [:flod]]

Comment on attachment 8972144 [details]
Bug 1446944 - Provide onboarding tooltip for the 3 pane inspector feature.

https://reviewboard.mozilla.org/r/240834/#review247042

Can you provide a screenshot of how the tooltip looks like? I have the feeling you're concatenating strings in a non obvious way, both from the strings with trailing spaces and code.

::: devtools/client/locales/en-US/inspector.properties:472
(Diff revision 6)
> +
> +# LOCALIZATION NOTE (inspector.threePaneOnboarding.new, 
> +# inspector.threePaneOnboarding.content, inspector.threePaneOnboarding.learnMore):
> +# This is the content shown in the 3 pane inspector onboarding tooltip that is displayed
> +# on top of the 3 pane inspector toggle button.
> +inspector.threePaneOnboarding.new=New: 

Please remove trailing spaces from the file, and in particular from strings.

Technically, they're relavant, meaning that "New: " is different from "New:", and the next developer touching the file will likely strip them away.

Comment 16

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #15)
> Can you provide a screenshot of how the tooltip looks like? I have the
> feeling you're concatenating strings in a non obvious way, both from the
> strings with trailing spaces and code.

Comment 17

[Julian Descottes [:jdescottes]]

Attachment: tooltip-screenshot.png

Comment 18

[Francesco Lodolo [:flod]]

Comment on attachment 8972144 [details]
Bug 1446944 - Provide onboarding tooltip for the 3 pane inspector feature.

https://reviewboard.mozilla.org/r/240834/#review247050

I completely missed the Invision link.

Sorry, concatenating strings in .properties files is a no-go. My suggestion would be to drop the bold stiling for "New:" to simplify the code

inspector.threePaneOnboarding.content=New: 3-pane mode lets you see both CSS rules and Layout tools. Click this button to toggle. %S
inspector.threePaneOnboarding.learnMore=Learn more

Then replace the %S with the Learn more link. Or make the title stand-alone (on a separate line)

<b>New:</b>
3-pane mode lets you see both CSS rules and Layout tools. Click this button to toggle. Learn more

Comment 19

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8972144 [details]
Bug 1446944 - Provide onboarding tooltip for the 3 pane inspector feature.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240834/diff/6-7/

Comment 20

[Francesco Lodolo [:flod]]

Comment on attachment 8972144 [details]
Bug 1446944 - Provide onboarding tooltip for the 3 pane inspector feature.

https://reviewboard.mozilla.org/r/240834/#review247252

Code is still concatenating strings, just replacing trailing spaces with padding.

If the final result needs to be:

```
<b>New:</b> 3-pane mode lets you see both CSS rules and Layout tools. Click this button to toggle. <a>Learn more</a>
```

Then you need something like this

```
# LOCALIZATION NOTE (inspector.threePaneOnboarding.content):
# This is the content shown in the 3 pane inspector onboarding tooltip that is displayed
# on top of the 3 pane inspector toggle button.
# %1$S is replaced by inspector.threePaneOnboarding.new, displayed with bold font
# %2$S is replaced by a link, using inspector.threePaneOnboarding.learnMore as text
inspector.threePaneOnboarding.new=New:
inspector.threePaneOnboarding.content=%1$S 3-pane mode lets you see both CSS rules and Layout tools. Click this button to toggle. %2$S
inspector.threePaneOnboarding.learnMore=Learn more
```

That's quite ugly, and the associated might get as ugly. That's why I suggested to at least simplify the first part, putting "New:" on its own line (and it might not need a colon at that point).

```
# LOCALIZATION NOTE (inspector.threePaneOnboarding.content):
# String is displayed as a title for the onboarding snippet
inspector.threePaneOnboarding.new=New:

# LOCALIZATION NOTE (inspector.threePaneOnboarding.content):
# This is the content shown in the 3 pane inspector onboarding tooltip that is displayed
# on top of the 3 pane inspector toggle button.
# %S is replaced by a link, using inspector.threePaneOnboarding.learnMore as text
inspector.threePaneOnboarding.content=3-pane mode lets you see both CSS rules and Layout tools. Click this button to toggle. %S
inspector.threePaneOnboarding.learnMore=Learn more
```

Comment 21

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8972144 [details]
Bug 1446944 - Provide onboarding tooltip for the 3 pane inspector feature.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240834/diff/7-8/

Comment 22

[Code Review Bot [:reviewbot]]

Comment on attachment 8972144 [details]
Bug 1446944 - Provide onboarding tooltip for the 3 pane inspector feature.

https://reviewboard.mozilla.org/r/240834/#review247270


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/shared/three-pane-onboarding-tooltip.js:59
(Diff revision 8)
> +    let messageString = L10N.getFormatStr("inspector.threePaneOnboarding.content",
> +      learnMoreString);
> +    messageString = messageString.replace(learnMoreString,
> +      `<a href="#" class="three-pane-onboarding-learn-more-link theme-link">
> +      ${learnMoreString}</a>`);
> +    message.innerHTML = messageString;

Error: Unsafe assignment to innerhtml [eslint: no-unsanitized/property]

Comment 23

[Francesco Lodolo [:flod]]

Comment on attachment 8972144 [details]
Bug 1446944 - Provide onboarding tooltip for the 3 pane inspector feature.

https://reviewboard.mozilla.org/r/240834/#review247280

Strings-wise, this looks good. Given efforts like bug 1444394, you might need to get rid of .innerHTML.

In case you need to split the string, keep in mind that there could be content after the %S placeholder.

Comment 24

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8972144 [details]
Bug 1446944 - Provide onboarding tooltip for the 3 pane inspector feature.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240834/diff/8-9/

Comment 25

[Julian Descottes [:jdescottes]]

Comment on attachment 8972144 [details]
Bug 1446944 - Provide onboarding tooltip for the 3 pane inspector feature.

https://reviewboard.mozilla.org/r/240834/#review247292

LGTM, 2 issues to fix + you mentioned you wanted to update the style of the links? Any plans here?

::: devtools/client/inspector/shared/three-pane-onboarding-tooltip.js:52
(Diff revision 9)
> +    const message = doc.createElementNS(XHTML_NS, "div");
> +    const learnMoreString = L10N.getStr("inspector.threePaneOnboarding.learnMoreLink");
> +    const messageString = L10N.getFormatStr("inspector.threePaneOnboarding.content",
> +      learnMoreString);
> +    const learnMoreStartIndex = messageString.indexOf(learnMoreString);
> +
> +    message.append(messageString.substring(0, learnMoreStartIndex));
> +
> +    this.learnMoreLink = doc.createElementNS(XHTML_NS, "a");
> +    this.learnMoreLink.className = "theme-link";
> +    this.learnMoreLink.href = "#";
> +    this.learnMoreLink.textContent = learnMoreString;
> +
> +    message.append(this.learnMoreLink);
> +    message.append(messageString.substring(learnMoreStartIndex + learnMoreString.length));
> +    content.append(message);

Given the complexity of the solution I think innerHTML + disabling eslint for this line would have been fine. 

We should file a follow up to clean this, up :freddyb can probably advise on what should be the preferred solution in this particular case.

::: devtools/client/inspector/shared/three-pane-onboarding-tooltip.js:111
(Diff revision 9)
> +   * and opens the link to the mdn page in a new tab.
> +   */
> +  onLearnMoreLinkClick() {
> +    Services.prefs.setBoolPref(SHOW_THREE_PANE_ONBOARDING_PREF, false);
> +    this.tooltip.hide();
> +    openWebLink(LEARN_MORE_LINK, this.toolbox);

Since https://bugzilla.mozilla.org/show_bug.cgi?id=1458591 already landed, we need to drop the toolbox argument here.

::: devtools/client/locales/en-US/inspector.properties:473
(Diff revision 9)
> +# LOCALIZATION NOTE (inspector.threePaneOnboarding.new): This string is displayed as
> +# a title for the 3 pane inspector onboarding tooltip.
> +inspector.threePaneOnboarding.new=New
> +
> +# LOCALIZATION NOTE (inspector.threePaneOnboarding.content,
> +# inspector.threePaneOnboarding.learnMore): This is the content shown in the 3 pane

This should be learnMoreLink and not learnMore

Comment 26

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8972144 [details]
Bug 1446944 - Provide onboarding tooltip for the 3 pane inspector feature.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240834/diff/9-10/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18937f7c5c2f
Provide onboarding tooltip for the 3 pane inspector feature. r=jdescottes, flod

Comment 28

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/18937f7c5c2f