1272466 - adding which container is being used in the tooltip when hovering over tabs

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Kamil Jozwiak [:kjozwiak]]

Attachment: example.png

We should add which container is being used in the tooltip that the user receives when they hover over tabs. Something similar to what e10s is currently doing. When a user hovers over a tab that's using e10s, they'll see a "e10s" at the end of the tooltip (see attached example).

* Default Container - we shouldn't do anything (leave as is)
* Personal Container - append "Personal" at the end of the tooltip
* Work Container - append "Work" at the end of the tooltip
* Banking Container - append "Banking" at the end of the tooltip
* Shopping Container - append "Shopping" at the end of the tooltip

Once we implement bug # 1267920, users should see the names that they've selected under the tooltip rather than the hardcoded values (Personal, Work etc..) that we're currently using.

Comment 1

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1272466 - Adding in container name to the tab title tooltip

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

Comment 2

[:Gijs (out for now; he/him)]

Comment on attachment 8814189 [details]
Bug 1272466 - Adding in container name to the tab title tooltip

https://reviewboard.mozilla.org/r/95446/#review95668

I expect we should use a separate string with 2 replacement vars for this. Flod, am I overthinking this?

Comment 3

[Francesco Lodolo [:flod]]

Comment on attachment 8814189 [details]
Bug 1272466 - Adding in container name to the tab title tooltip

https://reviewboard.mozilla.org/r/95446/#review95674

I suggest to have a different string ID, with placeholders and a clear localization comment, to manage this properly. For example: 

tabs.container.tooltip = #1 - #2

Or you could use

tabs.container.tooltip = \u0020- #2

But the risk is to be confusing, since you need an explicit whitespace.

Personally I would argue that the container might be the first information displayed, but that's a UX decision.

Comment 4

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #3)
> tabs.container.tooltip = #1 - #2
> tabs.container.tooltip = \u0020- #2

Better examples
tabs.container.tooltip = %1$S - %2$S
tabs.container.tooltip = \u0020- %S

Comment 5

[Jonathan Kingston [:jkt] he/him]

:flod, Should there be a similar one added for e10s too with the same usecase for consistency?

Comment 6

[Francesco Lodolo [:flod]]

(In reply to Jonathan Kingston [:jkt] from comment #5)
> :flod, Should there be a similar one added for e10s too with the same
> usecase for consistency?

Ideally yes, but we have e10s strings hard-coded in other places, including one fairly visible in the context menu. When I filed bug 1198920, the reply was that this was temporary, so I wouldn't bother touching a e10s specific strings.

Comment 7

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8814189 [details]
Bug 1272466 - Adding in container name to the tab title tooltip

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

Comment 8

[Francesco Lodolo [:flod]]

Comment on attachment 8814189 [details]
Bug 1272466 - Adding in container name to the tab title tooltip

https://reviewboard.mozilla.org/r/95446/#review95710

::: browser/locales/en-US/chrome/browser/tabbrowser.properties:53
(Diff revision 2)
>  
>  # LOCALIZATION NOTE (tabs.allowTabFocusByPromptForSite):
>  # %S is the hostname of the site where dialogs are allowed to switch tabs
>  tabs.allowTabFocusByPromptForSite=Allow dialogs from %S to take you to their tab
> +
> +tabs.containers.tooltip=%1$S - %2$S

Please add a localization comment explaining what the variables are.

# LOCALIZATION NOTE (tabs.containers.tooltip
# Displayed as a tooltip on container tabs
# %1$S is the URL of the current tab
# %1$S is the name of the current container

Comment 9

[Francesco Lodolo [:flod]]

Comment on attachment 8814189 [details]
Bug 1272466 - Adding in container name to the tab title tooltip

String looks good with the note added.

I suggest to request a proper review from a peer, since I can't actually test the code, and mozreview wouldn't land the code with my review anyway.

Comment 10

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #8)

> # LOCALIZATION NOTE (tabs.containers.tooltip
> # Displayed as a tooltip on container tabs
> # %1$S is the URL of the current tab
> # %1$S is the name of the current container

That's one poor example :-\

# LOCALIZATION NOTE (tabs.containers.tooltip):
# Displayed as a tooltip on container tabs
# %1$S is the URL of the current tab
# %2$S is the name of the current container

Comment 11

[Jonathan Kingston [:jkt] he/him]

Thanks Flod appreciate you looking at this, sorry forgot about the note :)

Comment 12

[Jonathan Kingston [:jkt] he/him]

Ah except it's not a URL either I'm changing that line to:
# %1$S is the title of the current tab

Comment 13

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8814189 [details]
Bug 1272466 - Adding in container name to the tab title tooltip

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

Comment 14

[:Gijs (out for now; he/him)]

Comment on attachment 8814189 [details]
Bug 1272466 - Adding in container name to the tab title tooltip

LGTM, but Flod should OK, I guess.

Comment 15

[Francesco Lodolo [:flod]]

Comment on attachment 8814189 [details]
Bug 1272466 - Adding in container name to the tab title tooltip

https://reviewboard.mozilla.org/r/95446/#review95912

Thanks Gijs, I feel safer when a real developer looks at code, as trivial as it can be.
The problem is that he's probably not going to be able to land with my r+ (but I'll ask).

Comment 16

[Francesco Lodolo [:flod]]

A sheriff will need to r+ this again to let this land.

Comment 17

[:Gijs (out for now; he/him)]

Comment on attachment 8814189 [details]
Bug 1272466 - Adding in container name to the tab title tooltip

https://reviewboard.mozilla.org/r/95446/#review96454

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b25f222b8636
Adding in container name to the tab title tooltip r=flod,Gijs

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b25f222b8636