Closed Bug 604913 Opened 11 years ago Closed 10 years ago

Tooltip for icon should be provided in Panorama

Categories

(Firefox Graveyard :: Panorama, enhancement, P3)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 11

People

(Reporter: alice0775, Assigned: raymondlee)

References

Details

(Whiteboard: [strings])

Attachments

(4 files, 6 obsolete files)

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.
Attached image screenshot-1
Attached image screenshot-2
Blocks: 592204
We're punting all our accessibility bugs to milestone "future". Though these are first on our list of stretch-goals.
Priority: -- → P3
Target Milestone: Firefox 4.0 → Future
Involves strings so unfortunately it's too late for fx4. Punting.
No longer blocks: 585689
Whiteboard: [strings]
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Attachment #555346 - Flags: review?(tim.taubert)
Blocks: 587301
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.
Attachment #555346 - Flags: review-
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.
Attachment #555346 - Flags: review?(tim.taubert)
Attached patch v2 (obsolete) — Splinter Review
> 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
Attachment #555346 - Attachment is obsolete: true
Attachment #555429 - Flags: review?(tim.taubert)
Comment on attachment 555429 [details] [diff] [review]
v2

>+    document.documentElement.appendChild(tooltip);

Can you find a more sensible place for this? mainPopupSet?
Attached patch v3 (obsolete) — Splinter Review
> Can you find a more sensible place for this? mainPopupSet?

Fixed
Attachment #555429 - Attachment is obsolete: true
Attachment #555616 - Flags: review?(tim.taubert)
Attachment #555429 - Flags: review?(tim.taubert)
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".
Attachment #555616 - Flags: review?(tim.taubert) → review+
Attached patch v4 (obsolete) — Splinter Review
(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/
Attachment #555616 - Attachment is obsolete: true
Attachment #555743 - Flags: ui-review?(limi)
Blocks: 623649
Duplicate of this bug: 627086
Attached file v5 (obsolete) —
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!
Attachment #555743 - Attachment is obsolete: true
Attachment #566438 - Flags: ui-review?(limi)
Attachment #555743 - Flags: ui-review?(limi)
Does this add a tooltip of tab title to the tab thumbnail itself?
(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.
(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.
Attached patch v6 (obsolete) — Splinter Review
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
Attachment #566438 - Attachment is obsolete: true
Attachment #569915 - Flags: ui-review?(limi)
Attachment #566438 - Flags: ui-review?(limi)
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"
Attachment #569915 - Flags: ui-review?(limi) → ui-review?(jboriss)
Video looks great! Can't wait to see this slipped into Nightly!
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. :)
Attachment #569915 - Flags: ui-review?(jboriss) → ui-review+
(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
Attachment #569915 - Attachment is obsolete: true
(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
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/3ac4bc759655
Keywords: checkin-needed
Whiteboard: [strings] → [fixed-in-fx-team][strings]
Target Milestone: Future → Firefox 11
https://hg.mozilla.org/mozilla-central/rev/3ac4bc759655
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][strings] → [strings]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.