Tooltip for icon should be provided in Panorama

RESOLVED FIXED in Firefox 11

Status

Firefox Graveyard
Panorama
P3
enhancement
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: Alice0775 White, Assigned: raymondlee)

Tracking

Trunk
Firefox 11
Dependency tree / graph

Details

(Whiteboard: [strings])

Attachments

(4 attachments, 6 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
Created attachment 483758 [details]
screenshot-1
(Reporter)

Comment 2

7 years ago
Created attachment 483759 [details]
screenshot-2

Updated

7 years ago
Blocks: 592204

Comment 3

7 years ago
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]
(Assignee)

Comment 5

6 years ago
Created attachment 555346 [details] [diff] [review]
v1
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Attachment #555346 - Flags: review?(tim.taubert)
(Assignee)

Updated

6 years ago
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)
(Assignee)

Comment 8

6 years ago
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
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?
(Assignee)

Comment 10

6 years ago
Created attachment 555616 [details] [diff] [review]
v3

> 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+
(Assignee)

Comment 12

6 years ago
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/
Attachment #555616 - Attachment is obsolete: true
Attachment #555743 - Flags: ui-review?(limi)
(Assignee)

Updated

6 years ago
Blocks: 623649
(Assignee)

Updated

6 years ago
Duplicate of this bug: 627086
(Assignee)

Comment 14

6 years ago
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!
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.
(Assignee)

Comment 17

6 years ago
(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.
(Assignee)

Comment 18

6 years ago
Created attachment 569914 [details]
Video with the patch applied
(Assignee)

Comment 19

6 years ago
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
Attachment #566438 - Attachment is obsolete: true
Attachment #569915 - Flags: ui-review?(limi)
Attachment #566438 - Flags: ui-review?(limi)
(Assignee)

Comment 20

6 years ago
Comment on attachment 569915 [details] [diff] [review]
v6

Passed Try!
https://tbpl.mozilla.org/?tree=Try&rev=0cf0e38a1cf9
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)

Comment 22

6 years ago
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+
(Assignee)

Comment 24

6 years ago
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
Attachment #569915 - Attachment is obsolete: true
(Assignee)

Comment 25

6 years ago
(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
Last Resolved: 6 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.