Last Comment Bug 604913 - Tooltip for icon should be provided in Panorama
: Tooltip for icon should be provided in Panorama
Status: RESOLVED FIXED
[strings]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: P3 enhancement
: Firefox 11
Assigned To: Raymond Lee [:raymondlee]
:
:
Mentors:
: 627086 (view as bug list)
Depends on:
Blocks: 587301 592204 623649
  Show dependency treegraph
 
Reported: 2010-10-16 13:13 PDT by Alice0775 White
Modified: 2016-04-12 14:00 PDT (History)
9 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
screenshot-1 (83.08 KB, image/png)
2010-10-16 13:13 PDT, Alice0775 White
no flags Details
screenshot-2 (10.09 KB, image/png)
2010-10-16 13:15 PDT, Alice0775 White
no flags Details
v1 (8.16 KB, patch)
2011-08-24 02:18 PDT, Raymond Lee [:raymondlee]
dao+bmo: review-
Details | Diff | Splinter Review
v2 (10.72 KB, patch)
2011-08-24 09:02 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v3 (10.74 KB, patch)
2011-08-24 18:41 PDT, Raymond Lee [:raymondlee]
ttaubert: review+
Details | Diff | Splinter Review
v4 (10.74 KB, patch)
2011-08-25 08:35 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v5 (11.08 KB, text/plain)
2011-10-11 22:35 PDT, Raymond Lee [:raymondlee]
no flags Details
Video with the patch applied (582.50 KB, video/quicktime)
2011-10-27 01:57 PDT, Raymond Lee [:raymondlee]
no flags Details
v6 (11.04 KB, patch)
2011-10-27 01:59 PDT, Raymond Lee [:raymondlee]
limi: ui‑review+
Details | Diff | Splinter Review
Patch for checkin (11.14 KB, patch)
2011-11-30 22:00 PST, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description Alice0775 White 2010-10-16 13:13:12 PDT
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 2010-10-16 13:13:49 PDT
Created attachment 483758 [details]
screenshot-1
Comment 2 Alice0775 White 2010-10-16 13:15:35 PDT
Created attachment 483759 [details]
screenshot-2
Comment 3 Kevin Hanes 2010-10-18 15:43:34 PDT
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] 2011-01-20 15:18:55 PST
Involves strings so unfortunately it's too late for fx4. Punting.
Comment 5 Raymond Lee [:raymondlee] 2011-08-24 02:18:30 PDT
Created attachment 555346 [details] [diff] [review]
v1
Comment 6 Dão Gottwald [:dao] 2011-08-24 03:25:26 PDT
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] 2011-08-24 03:43:50 PDT
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] 2011-08-24 09:02:39 PDT
Created attachment 555429 [details] [diff] [review]
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] 2011-08-24 11:36:47 PDT
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] 2011-08-24 18:41:13 PDT
Created attachment 555616 [details] [diff] [review]
v3

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

Fixed
Comment 11 Tim Taubert [:ttaubert] 2011-08-25 02:14:23 PDT
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] 2011-08-25 08:35:09 PDT
Created attachment 555743 [details] [diff] [review]
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/raymond@raysquare.com-4720bf706bd1/
Comment 13 Raymond Lee [:raymondlee] 2011-09-16 00:03:42 PDT
*** Bug 627086 has been marked as a duplicate of this bug. ***
Comment 14 Raymond Lee [:raymondlee] 2011-10-11 22:35:27 PDT
Created attachment 566438 [details]
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/raymond@raysquare.com-d8f36b3841ff/

Alex: could you weight in please? Thanks!
Comment 15 Dietrich Ayala (:dietrich) 2011-10-12 15:35:23 PDT
Does this add a tooltip of tab title to the tab thumbnail itself?
Comment 16 Dietrich Ayala (:dietrich) 2011-10-12 15:49:15 PDT
(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] 2011-10-12 17:46:34 PDT
(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] 2011-10-27 01:57:37 PDT
Created attachment 569914 [details]
Video with the patch applied
Comment 19 Raymond Lee [:raymondlee] 2011-10-27 01:59:23 PDT
Created attachment 569915 [details] [diff] [review]
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] 2011-10-27 09:05:10 PDT
Comment on attachment 569915 [details] [diff] [review]
v6

Passed Try!
https://tbpl.mozilla.org/?tree=Try&rev=0cf0e38a1cf9
Comment 21 Jennifer Morrow [:Boriss] (UX) 2011-11-30 13:05:32 PST
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 2011-11-30 13:31:26 PST
Video looks great! Can't wait to see this slipped into Nightly!
Comment 23 Alex Limi (:limi) — Firefox UX Team 2011-11-30 14:01:55 PST
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] 2011-11-30 22:00:01 PST
Created attachment 578177 [details] [diff] [review]
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] 2011-12-01 01:40:05 PST
(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] 2011-12-01 01:46:32 PST
https://hg.mozilla.org/integration/fx-team/rev/3ac4bc759655
Comment 27 Tim Taubert [:ttaubert] 2011-12-02 05:41:04 PST
https://hg.mozilla.org/mozilla-central/rev/3ac4bc759655

Note You need to log in before you can comment on or make changes to this bug.