439312 - Site Navigation Bar misses its tooltip when minimized

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Serge Gautherie (:sgautherie)]

(Found while testing bug 401552)

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a5pre) Gecko/20070515 SeaMonkey/1.5a] (nightly) (W2Ksp4)
[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008061402 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Other minimized (tool)bars have a tooltip displaying their name;
S.N.B. has none.

Comment 1

[Tony Mechelynck [:tonymec]]

Mozilla/5.0 (X11; Linux x86_64; rv:2.0.2pre) Gecko/20110518 Firefox/4.0.2pre SeaMonkey/2.1.1pre

Happens on Linux too => Platform to "All".
Tentatively moving to UI Design. Please move to Themes if it doesn't happen in Modern theme (I tested only with Classic).

Comment 2

[therube]

I cannot seem to reproduce?
WinXP x86, (Windows) Classic Theme


Mozilla/5.0 (Windows NT 5.1; rv:2.0.1) Gecko/20110511 Firefox/4.0.1 SeaMonkey/2.1

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.19) Gecko/20110420 SeaMonkey/2.0.14

Comment 3

[therube]

Changing to (Windows) XP Theme made no difference.
I still see the tooltips?

Comment 4

[Philip Chee]

The Link toolbar (website navigation toolbar) just needs a "grippytooltiptext" attribute set e.g.
grippytooltiptext="Website Navigation Bar"

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to therube from comment #2)
> I cannot seem to reproduce?

(In reply to therube from comment #3)
> I still see the tooltips?

Unexpected. What does the S.N.B. tooltip read? (Maybe attach a screenshot.)

*****

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1.19) Gecko/20110420 SeaMonkey/2.0.14] (release)
[Mozilla/5.0 (Windows NT 5.0; rv:13.0a1) Gecko/20120201 Firefox/13.0a1 SeaMonkey/2.10a1] (nightly, 2012-02-01-00-30-09-comm-central-trunk)

Bug still there (with Classic theme).

Comment 6

[Philip Chee]

> Changing to (Windows) XP Theme made no difference.
> I still see the tooltips?

STR:
1. View -> Show/Hide->Website Navigation Bar -> Show always.
2. Collapse the WNB (links) toolbar.
3. Hover your mouse over the collapsed grippy.

Comment 7

[Philip Chee]

> The Link toolbar (website navigation toolbar) just needs a "grippytooltiptext"
> attribute set e.g. |grippytooltiptext="Website Navigation Bar"|

The link toolbar is here in case anyone wants to fix this:
http://hg.mozilla.org/comm-central/annotate/4d1465a48f5a/suite/browser/linkToolbarOverlay.xul#l56

Comment 8

[Tony Mechelynck [:tonymec]]

Attachment: patch v0

Comment 9

[Tony Mechelynck [:tonymec]]

Comment on attachment 645301 [details] [diff] [review]
patch v0

:-? If it was so simple, why didn't anybody do it? Everything was in the bug comments.

...And BTW I can still see the bug in the following build:
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Firefox/17.0 SeaMonkey/2.14a1 ID:20120723003021 CSet: 433268decbc6 m-c: 82b6c5885345

Comment 10

[Tony Mechelynck [:tonymec]]

Since this fix includes a new localizable string, it won't be ported back to versions earlier than current trunk.

Comment 11

[Tony Mechelynck [:tonymec]]

Attachment: patch v0.1

This version uses an existing string.

Comment 12

[[e-mail]]

Comment on attachment 645303 [details] [diff] [review]
patch v0.1

It's not quite that simple, you have to use the name of the string (the bit after <!ENTITY in the .dtd file) wrapped in & and ; characters. (This assumes that the .dtd file that you're using is already listed in the <!DOCTYPE> declaration.)

Comment 13

[Tony Mechelynck [:tonymec]]

(In reply to [e-mail] from comment #12)
> Comment on attachment 645303 [details] [diff] [review]
> patch v0.1
> 
> It's not quite that simple, you have to use the name of the string (the bit
> after <!ENTITY in the .dtd file) wrapped in & and ; characters. (This
> assumes that the .dtd file that you're using is already listed in the
> <!DOCTYPE> declaration.)

I thought there was a hitch. OK let's go find it.

Comment 14

[Tony Mechelynck [:tonymec]]

http://mxr.mozilla.org/comm-central/source/suite/locales/en-US/chrome/browser/linkToolbar.dtd#6

and it is listed at http://hg.mozilla.org/comm-central/annotate/4d1465a48f5a/suite/browser/linkToolbarOverlay.xul#l8

Comment 15

[Tony Mechelynck [:tonymec]]

Attachment: patch v0.2 (Aurora and earlier)

Comment 16

[[e-mail]]

Comment on attachment 645324 [details] [diff] [review]
patch v0.2 (Aurora and earlier)

This seems acceptable as a branch hack (although you'd still need approval).

However I notice that all of our other grippy tooltips use the form
<toolbar id="someToolbar" grippytooltiptext="&someToolbar.tooltip;"> so I'll request KaiRo's feedback as to what localisers would expect us to do.

Comment 17

[Tony Mechelynck [:tonymec]]

Hm, "Menu Bar" is not present in the View→Show/Hide menu, while "Bookmarks Toolbar" and "Navigation Toolbar" use (at least in en-US) the same string for menuitem and tooltip. It would be possible (for 2.14 and later) to modify the DTD (creating a second entity with the same text) but wouldn't it be, er, in French we would say "se chatouiller pour se faire rire" (tickling oneself to make oneself laugh)? Reusing the same string ensures consistency.

Comment 18

[Philip Chee]

> However I notice that all of our other grippy tooltips use the form
> <toolbar id="someToolbar" grippytooltiptext="&someToolbar.tooltip;">

For branch we should take this. For trunk we should create a new entity and use that.

> with the same text) but wouldn't it be, er, in French we would say "se chatouiller pour
> se faire rire" (tickling oneself to make oneself laugh)? Reusing the same string
> ensures consistency.
It may not be the same string in *all* languages. In fact it's almost a certainty that some language or other has some peculiar rule or special case.

Comment 19

[Tony Mechelynck [:tonymec]]

Attachment: patch v1.0 (comm-central ONLY)

OK, here's the patch for trunk, with distinct strings for label and tooltip.

Comment 20

[[e-mail]]

Comment on attachment 645660 [details] [diff] [review]
patch v1.0 (comm-central ONLY)

> <!-- Link Toolbar Title -->
> <!ENTITY linkToolbar.label "Website Navigation Bar">
>+<!ENTITY linkToolbar.tooltip "Website Navigation Bar">
> <!ENTITY linkToolbar.accesskey "a">
linkToolbar.label and linkToolbar.accesskey go on the same DOM element, so they should not be split apart by the new entity. r=me with that fixed.

Comment 21

[Tony Mechelynck [:tonymec]]

Attachment: patch v1.1 (comm-central ONLY) r=neil

Comment 22

[Robert Kaiser]

Comment on attachment 645675 [details] [diff] [review]
patch v1.1 (comm-central ONLY) r=neil

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

::: suite/locales/en-US/chrome/browser/linkToolbar.dtd
@@ +7,3 @@
>  <!ENTITY linkToolbar.label "Website Navigation Bar">
>  <!ENTITY linkToolbar.accesskey "a">
>  

Style nit: I'd put the .tooltip entry after the other two, just for logic.

Comment 23

[Tony Mechelynck [:tonymec]]

(In reply to Robert Kaiser (:[e-mail]) from comment #22)
> Comment on attachment 645675 [details] [diff] [review]
> patch v1.1 (comm-central ONLY) r=neil
> 
> Review of attachment 645675 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: suite/locales/en-US/chrome/browser/linkToolbar.dtd
> @@ +7,3 @@
> >  <!ENTITY linkToolbar.label "Website Navigation Bar">
> >  <!ENTITY linkToolbar.accesskey "a">
> >  
> 
> Style nit: I'd put the .tooltip entry after the other two, just for logic.

I thought the two occurrences of the string belonged next to each other, and Neil wanted title & accesskey together; but no prob, I'll make the change.

Comment 24

[Tony Mechelynck [:tonymec]]

Attachment: patch v1.2 (comm-central ONLY) r+=neil f+=kairo

carrying flags forward from v1.1

Comment 25

[Tony Mechelynck [:tonymec]]

setting checkin-needed, please push only patch v1 (attachment #646434 [details] [diff] [review]) and only to comm-central.

If no problems and after approval we'll take care of the branches.

Comment 26

[Tony Mechelynck [:tonymec]]

Comment on attachment 645324 [details] [diff] [review]
patch v0.2 (Aurora and earlier)

[Approval Request Comment]
Regression caused by (bug #): unknown.
    Problem existed in SeaMonkey 2.0a1pre (2008-06-14) and possibly earlier.
User impact if declined: trivial: missing tooltip on a grippy
Testing completed (on m-c, etc.):
    checkin-needed shall not happen before VERIFIED on trunk.
Risk to taking this patch (and alternatives if risky): minimal (IMHO none)
String changes made by this patch: none

This is the "simplified" version (with no new strings) of the patch now ready for trunk (which has one new string, albeit identical in en-US to an existing string).

Comment 27

[Tony Mechelynck [:tonymec]]

Callek and/or ewong: I'm not sure how far back the fix can be ported. Feel free to deny any "exaggerated" approval request.

Comment 28

[Robert Kaiser]

Comment on attachment 645324 [details] [diff] [review]
patch v0.2 (Aurora and earlier)

approval‑seamonkey2.* should actually be retired, we don't care about those old branches any more.
This also doesn't warrant landing in a potential chemspill, so - for release.
We are early in the beta cycle and this is basically zero risk, so I'll grant it there, but if we'd be later in the cycle, this wouldn't be enough win to risk any change at all there.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/d23b0fba5ed6

Hi Tony, to make life easier for those checking in on your behalf, please make sure your future patches include all the needed metadata. Thanks!
https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in

Comment 30

[Tony Mechelynck [:tonymec]]

(In reply to Ryan VanderMeulen from comment #29)
> https://hg.mozilla.org/comm-central/rev/d23b0fba5ed6
> 
> Hi Tony, to make life easier for those checking in on your behalf, please
> make sure your future patches include all the needed metadata. Thanks!
> https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in

ah, I forgot the -m argument to qrefresh. Thanks for reminding me.

Comment 31

[Tony Mechelynck [:tonymec]]

Attachment: patch v0.2.1 (Aurora and earlier) r=Neil (patch with enhanced metadata)

Carrying flags over from patch v0.2
No changes in the code
Added #Author and patch title in what "man patch" calls the leading garbage.

Do Not Yet Push.

Comment 32

[Tony Mechelynck [:tonymec]]

Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Firefox/17.0 SeaMonkey/2.14a1 ID:20120726184449 c-c: d23b0fba5ed6 m-c: f528e021ceb1

Testing the first hourly build with the fix using the STR in comment #6.

It has the tooltip (the latest nightly didn't). Otherwise it works just as well AFAICT.

In the meantime I have cloned comm-aurora and comm-beta and verified that attachment #646452 [details] [diff] [review] (patch v0.2.1) applies cleanly on their respective "default" branch tips.

Please push attachment #646452 [details] [diff] [review] (or attachment #645324 [details] [diff] [review]) to comm-aurora and comm-beta.

Note: As anyone can verify by clicking "Details" on them, both these attachments have identical code, but one of them has the approvals while the other has the metadata.

Comment 33

[[e-mail]]

Comment on attachment 646452 [details] [diff] [review]
patch v0.2.1 (Aurora and earlier) r=Neil (patch with enhanced metadata)

[Triage Comment]
Transferring KaiRo's approvals.

Comment 34

[Philip Chee]

Pushed:
http://hg.mozilla.org/releases/comm-aurora/rev/780fa44fb8fd
http://hg.mozilla.org/releases/comm-beta/rev/1a9d3dfb1c97