Closed Bug 1446944 Opened 2 years ago Closed 2 years ago

Provide on-boarding tooltip for 3-pane feature

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: mbalfanz, Assigned: gl)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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
Assignee: nobody → gl
Status: NEW → ASSIGNED
I should note dt failures in this try is caused by other patches and not this particular one.
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.
Attachment #8972144 - Flags: review?(jdescottes) → review+
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.
Attachment #8972144 - Flags: review+ → review?(jdescottes)
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 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 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
Attachment #8972144 - Flags: review+
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!
Attachment #8972144 - Flags: review?(francesco.lodolo)
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.
(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.
Flags: needinfo?(gl)
Attached image tooltip-screenshot.png
Flags: needinfo?(gl)
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
Attachment #8972144 - Flags: review?(francesco.lodolo) → review-
Attachment #8972144 - Flags: review?(francesco.lodolo)
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
```
Attachment #8972144 - Flags: review?(francesco.lodolo) → review-
Attachment #8972144 - Flags: review+ → review?(jdescottes)
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 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.
Attachment #8972144 - Flags: review?(francesco.lodolo) → review+
Attachment #8972144 - Flags: review?(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
Attachment #8972144 - Flags: review?(jdescottes) → review+
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18937f7c5c2f
Provide onboarding tooltip for the 3 pane inspector feature. r=jdescottes, flod
https://hg.mozilla.org/mozilla-central/rev/18937f7c5c2f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Depends on: 1459538
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.