604913 - Tooltip for icon should be provided in Panorama

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, enhancement, P3)

Description

[Alice0775 White]

Build Identifier: 
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101016
Firefox/4.0b8pre ID:20101016041245

Tooltip for Icon and Searchbox should be provided in Panorama

Reproducible: Always

Steps to Reproduce:
1. Start Minefield with new profile
2. Enter Panorama Ctri+E
3. Mouse hiver icon

Actual Results:
 No tooltip indicated.

Comment 1

[Alice0775 White]

Attachment: screenshot-1

Comment 2

[Alice0775 White]

Attachment: screenshot-2

Comment 3

[Kevin Hanes]

We're punting all our accessibility bugs to milestone "future". Though these are first on our list of stretch-goals.

Comment 4

[Michael Yoshitaka Erlewine [:mitcho]]

Involves strings so unfortunately it's too late for fx4. Punting.

Comment 5

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 555346 [details] [diff] [review]
v1

aHTMLTooltip is there for content documents and needs a rewrite for e10s, please don't use it.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 555346 [details] [diff] [review]
v1

Review of attachment 555346 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for tackling this!

::: browser/base/content/browser-tabview.js
@@ +189,5 @@
>      // ___ create the frame
>      this._iframe = document.createElement("iframe");
>      this._iframe.id = "tab-view";
>      this._iframe.setAttribute("transparent", "true");
> +    this._iframe.setAttribute("tooltip", "aHTMLTooltip");

Please address Dão's comment. We don't need to be e10s safe because the Panorama iframe won't run in a separate process but we'd need our own implementation of "aHTMLTooltip". Maybe we don't need the "title" attributes either with our own solution.

::: browser/base/content/tabview/groupitems.js
@@ +198,5 @@
>        e.stopPropagation();
>      })
>      .keypress(handleKeyPress)
> +    .keyup(handleKeyUp)
> +    .attr("title", tabviewString("groupItem.defaultName"));

I agree, that the titleShield should have a tooltip, but should the input field have one, too?

::: browser/base/content/tabview/tabitems.js
@@ +913,3 @@
>          tabItem.url = tabUrl;
>          tabItem.save();
> +        tabItem.$canvas.attr("title", tabUrl);

Could we maybe set a "title" for the whole tabItem and combine it like "$label - $url"? IMHO it's not that useful to differentiate between the title and the url.

::: browser/locales/en-US/chrome/browser/tabview.properties
@@ +1,1 @@
> +tabview.button.searchTabs=Search tabs...

That should be "Search tab groups"

@@ +1,2 @@
> +tabview.button.searchTabs=Search tabs...
> +tabview.button.exitTabGroups=Exit Tabs Groups

IMHO this should be "Exit Panorama". https://www.mozilla.com/en-US/firefox/6.0/whatsnew/ refers to us as "Panorama".

@@ +1,3 @@
> +tabview.button.searchTabs=Search tabs...
> +tabview.button.exitTabGroups=Exit Tabs Groups
> +tabview.groupItem.defaultName=Name this tab group...

You accidentally replaced "…" (…) with "..." (three dots).

@@ +5,2 @@
>  tabview.groupItem.undoCloseGroup=Undo Close Group
> +tabview.groupItem.restoreClosedGroup=Restore closed group

This should rather be "Discard closed group". The close icon does not restore but remove the hidden group.

@@ +5,3 @@
>  tabview.groupItem.undoCloseGroup=Undo Close Group
> +tabview.groupItem.restoreClosedGroup=Restore closed group
> +tabview.tabItem.closeTab=Close tab

We should re-use "tabs.closeTab" here.

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: v2

> Please address Dão's comment. We don't need to be e10s safe because the
> Panorama iframe won't run in a separate process but we'd need our own
> implementation of "aHTMLTooltip". Maybe we don't need the "title" attributes
> either with our own solution.

Implemented our own fillInTooltip method.

> 
> Could we maybe set a "title" for the whole tabItem and combine it like
> "$label - $url"? IMHO it's not that useful to differentiate between the
> title and the url.

The whole tabItem has a two line tooltip: first line is label and second line is url.

Fixed other things in the properties file

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 555429 [details] [diff] [review]
v2

>+    document.documentElement.appendChild(tooltip);

Can you find a more sensible place for this? mainPopupSet?

Comment 10

[Raymond Lee [:raymondlee]]

Attachment: v3

> Can you find a more sensible place for this? mainPopupSet?

Fixed

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 555616 [details] [diff] [review]
v3

Review of attachment 555616 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I forgot that from an accessibility point of view it could make sense to have a title attribute for the group title's input field. Can you re-include that, please? :)

Anyway, looks good! I like that newline solution for the tab tooltips, too. r=me with those two little fixes.

I'd like to have UX weigh in. Can you please push that patch to try and and ask the UX team for review and point to your try builds? That would be great, thanks.

::: browser/locales/en-US/chrome/browser/tabview.properties
@@ +3,2 @@
>  tabview.groupItem.defaultName=Name this tab group…
> +tabview.groupItem.closeGroup=Close Group

Please make that a lowercase "group".

Comment 12

[Raymond Lee [:raymondlee]]

Attachment: v4

(In reply to Tim Taubert [:ttaubert] from comment #11) 
> Sorry, I forgot that from an accessibility point of view it could make sense
> to have a title attribute for the group title's input field. Can you
> re-include that, please? :)

I didn't remove it in this patch.

> ::: browser/locales/en-US/chrome/browser/tabview.properties
> @@ +3,2 @@
> >  tabview.groupItem.defaultName=Name this tab group…
> > +tabview.groupItem.closeGroup=Close Group
> 
> Please make that a lowercase "group".

Fixed


Could UX weight in this please?  You can download the builds using the below link 
http://ftp.mozilla.org/pub/mozilla.org/mobile/try-builds/[e-mail]-4720bf706bd1/

Comment 14

[Raymond Lee [:raymondlee]]

Attachment: v5

unrotted

Passed Try:
https://tbpl.mozilla.org/?tree=Try&rev=d8f36b3841ff

Builds available in the below address 
http://ftp.mozilla.org/pub/mozilla.org/mobile/try-builds/[e-mail]-d8f36b3841ff/

Alex: could you weight in please? Thanks!

Comment 15

[Dietrich Ayala (:dietrich)]

Does this add a tooltip of tab title to the tab thumbnail itself?

Comment 16

[Dietrich Ayala (:dietrich)]

(In reply to Dietrich Ayala (:dietrich) from comment #15)
> Does this add a tooltip of tab title to the tab thumbnail itself?

Nm, that's bug 587301.

Comment 17

[Raymond Lee [:raymondlee]]

(In reply to Dietrich Ayala (:dietrich) from comment #15)
> Does this add a tooltip of tab title to the tab thumbnail itself?

Yes, this does add a tooltip of tab title to the tab thumbnail.

Comment 18

[Raymond Lee [:raymondlee]]

Attachment: Video with the patch applied

Comment 19

[Raymond Lee [:raymondlee]]

Attachment: v6

Updated the patch after updating the patch.

limi: please see the video which shows how the tooltips look like 
https://bugzilla.mozilla.org/attachment.cgi?id=569914

Comment 20

[Raymond Lee [:raymondlee]]

Comment on attachment 569915 [details] [diff] [review]
v6

Passed Try!
https://tbpl.mozilla.org/?tree=Try&rev=0cf0e38a1cf9

Comment 21

[Jennifer Morrow [:Boriss] (UX)]

Two quick things re tooltip strings:

1. I see in Raymond's video in Comment 18 that the tooltip for exiting says "Exit Panorama."  As far as I know we don't use the name Panorama in product any longer, so this should just be "Exit tab groups."

2. The text field tooltip for naming a tab group has "..." after it.  We don't do this for other Firefox text fields, so let's shorten the strong to just "Name this tab group"

Comment 22

[RC]

Video looks great! Can't wait to see this slipped into Nightly!

Comment 23

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 569915 [details] [diff] [review]
v6

Sorry about not seeing this earlier, ui-r+ with the changes Boriss pointed out.

Minor nit: Don't include the punctuation mark in any of the tooltips, tooltips never have an ending period unless there are multiple sentences (in which case we're probably doing it wrong :)

(Using this bug to show Zhenshuo and Yuan how Bugzilla reviews work, didn't mean to steal from you. :)

Comment 24

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

(In reply to Jennifer Morrow [:Boriss] (Firefox UX) from comment #21)
> Two quick things re tooltip strings:
> 
> 1. I see in Raymond's video in Comment 18 that the tooltip for exiting says
> "Exit Panorama."  As far as I know we don't use the name Panorama in product
> any longer, so this should just be "Exit tab groups."
> 
Updated

> 2. The text field tooltip for naming a tab group has "..." after it.  We
> don't do this for other Firefox text fields, so let's shorten the strong to
> just "Name this tab group"

Fixed

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=dc3660b678e8

Comment 25

[Raymond Lee [:raymondlee]]

(In reply to Raymond Lee [:raymondlee] from comment #24) 
> Pushed to try and waiting for results
> https://tbpl.mozilla.org/?tree=Try&rev=dc3660b678e8

Passed Try

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/3ac4bc759655

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/3ac4bc759655