Closed Bug 345257 Opened 18 years ago Closed 18 years ago

add tooltip to "all tabs" menu button

Categories

(Firefox :: Tabbed Browser, defect)

x86
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 2 beta2

People

(Reporter: moco, Assigned: ventnor.bugzilla)

References

Details

(Keywords: fixed1.8.1, late-l10n)

Attachments

(1 file, 9 obsolete files)

add tooltip to "all tabs" menu button

spun off from bug #343251

beltzner writes:

I'd suggest "List all tabs" or "Show all tabs". The latter is what Safari does,
but it seems odd to me since you're not actually showing the tabs, you're just
showing the names of the tabs (which I think of as "listing").

asaf writes:

<Mano> i guess the tooltip thing can't make the branch
<sspitzer> the search one cant, but for all tabs, we just want a static tooltip
<sspitzer> "List all tabs"
<sspitzer> the search one has a problem, since it dpends on the selected item, right?
<sspitzer> or did I misunderstand?
<Mano> i think if you set it on the button we'll see it on the menuitems as well :-/
<Mano> but please test
<sspitzer> I'll test.
Attached patch Initial patch (obsolete) — Splinter Review
Tooltip appears on button but not on menu items. Yes, I know I'm using a non-standard attribute "tooltiplabel". However this was the only design I could think of at the time to keep easy localization. "tooltiptext" would have caused it to appear on the items as well. If someone can suggest a better way to store the localization variable, I'm all ears.
Attached patch Patch w/o made-up attribute (obsolete) — Splinter Review
Ah, but I just discovered that you CAN pass the localization string as a parameter after all. *Slaps forehead* Anyway, no more made up attribute!
Attachment #229950 - Attachment is obsolete: true
Comment on attachment 229951 [details] [diff] [review]
Patch w/o made-up attribute

No scripts in the <content> part of this binding. Add an <handler> and check the event target instead.

I would put the label in tabbrowser.properties and cache it in a field.
Attachment #229951 - Flags: review-
(In reply to comment #3)
> (From update of attachment 229951 [details] [diff] [review] [edit])
> No scripts in the <content> part of this binding. Add an <handler> and check
> the event target instead.
> 
> I would put the label in tabbrowser.properties and cache it in a field.
> 

I tried to use a handler but the event always referred to the root <binding> node. No matter what I tried, I couldn't get it to refer to the button itself until I used document.tooltipNode. So I can only either make a handler or check the event.target, I can't do both.
What do you mean by "refer to the binding"?
(In reply to comment #5)
> What do you mean by "refer to the binding"?
> 

Add this line to the <binding> containing the List all tabs button:

<handler event="popupshowing" action="alert(event.target.getAttribute('class'));"/>

I expect a popup saying "tabs-alltabs-button", the class of the button, to tell me that the event is indeed pointing to the button. But instead I get "tabbrowser-tabs". A <binding> has this class, so the event has got it seriously wrong.

I have made a handler which works, but it MUST use document.tooltipNode. Unless there is a way to get event to point to the button, I can't use both that and a handler.
Attached patch Patch with handler (obsolete) — Splinter Review
After further testing, I found out that event.target points to the <xul:tabs> on line ~100, even when on the button and menu lines. I have made a handler, and put the string into tabbrowser.properties, but that is as far as I can go. I cannot check the event unless someone else gives me some pointers.

(yes, I tried all the handler phases and currentTarget too)
Attachment #229951 - Attachment is obsolete: true
Attachment #230086 - Flags: review?(bugs.mano)
Button should def have a tooltip for 2.0 release so nominating for blocking 2.0
Flags: blocking-firefox2?
Comment on attachment 230086 [details] [diff] [review]
Patch with handler

(In reply to comment #7)
> Created an attachment (id=230086) [edit]
> Patch with handler
> 
> After further testing, I found out that event.target points to the <xul:tabs>
> on line ~100, even when on the button and menu lines. I have made a handler,
> and put the string into tabbrowser.properties, but that is as far as I can go.
> I cannot check the event unless someone else gives me some pointers.
> 
> (yes, I tried all the handler phases and currentTarget too)
> 

Probably bacuase this is an anonymous node, did you try event.originalTarget?

Another question, maybe we can set the label attribute "statically" on the tooltip element, i.e
 <tooltip label="&&listAllTabs.label"/>

and just do |event.stopPropagation();| in the popupshowing handler if event.originalTarget.parentNode == this.mAllTabsPopup (attribute checks are usually bad because they break themes which try to override the binding content part only).
Attachment #230086 - Flags: review?(bugs.mano) → review-
Attached patch Patch trunk candidate (obsolete) — Splinter Review
Thank you so much! originalTarget made me one step closer. Although it didn't refer to the button, but the tooltip node itself. Nonetheless, after a long while of waiting for that "eureka" moment, I finally made this patch. I hope it satisfies you this time, please try not to nitpick. ;)
Attachment #230086 - Attachment is obsolete: true
Attachment #230135 - Flags: review?(bugs.mano)
Comment on attachment 230135 [details] [diff] [review]
Patch trunk candidate

Well, this _is_ progress :)

>Index: toolkit/content/widgets/tabbrowser.xml
>===================================================================

>+      <method name="createAllTabsTooltip">

get rid of this method and put its contents in the handler itself.


>+          // controls whether the "list all tabs" tooltip should show
>+          // suppress it if the popup list is open AND the event refers to the tooltip node
>+          return !(this.mAllTabsPopup.hasChildNodes() && event.originalTarget == this.mAllTabsTooltip);

The following would be much better
 if (event.originalTarget == this.mAllTabsTooltip &&
     document.tooltipNode.parentNode == this.mAllTabsPopup) {
    event.stopPropagation();
 }
Attachment #230135 - Flags: review?(bugs.mano) → review-
(In reply to comment #11)
> (From update of attachment 230135 [details] [diff] [review] [edit])

> >+          // controls whether the "list all tabs" tooltip should show
> >+          // suppress it if the popup list is open AND the event refers to the tooltip node
> >+          return !(this.mAllTabsPopup.hasChildNodes() && event.originalTarget == this.mAllTabsTooltip);
> 
> The following would be much better
>  if (event.originalTarget == this.mAllTabsTooltip &&
>      document.tooltipNode.parentNode == this.mAllTabsPopup) {
>     event.stopPropagation();
>  }
> 

That doesn't work. Tooltip is shown on menu items as well.
Attached patch Patch #5 (obsolete) — Splinter Review
Replacing "stopPropagation" with "return false;" on the other hand...
Now please check this in before more of my hair falls out. :(
Attachment #230135 - Attachment is obsolete: true
Attachment #230149 - Flags: review?(bugs.mano)
Why did you replace it? We generally use preventDefault and stopPropagation in chrome code rather then returning false...
Oh, Er, Yeah - I guess you need preventDefault rather than stopPropagation considering the way popupshowing is fired.
Not going to block on this, but will consider a safe patch.
Flags: blocking-firefox2? → blocking-firefox2-
Attached patch Patch preventDefault() (obsolete) — Splinter Review
preventDefault works. This patch now uses that.
Attachment #230149 - Attachment is obsolete: true
Attachment #230216 - Flags: review?(bugs.mano)
Attachment #230149 - Flags: review?(bugs.mano)
Comment on attachment 230216 [details] [diff] [review]
Patch preventDefault()

>+      <handler event="popupshowing">
>+        <![CDATA[
>+        if (event.originalTarget == this.mAllTabsTooltip &&
>+            document.tooltipNode.parentNode == this.mAllTabsPopup) {
>+          event.preventDefault();
>+        }
>+        ]]>
>+      </handler>

nit: make this:

>+      <handler event="popupshowing"><![CDATA[
>+        if (event.originalTarget == this.mAllTabsTooltip &&
>+            document.tooltipNode.parentNode == this.mAllTabsPopup)
>+          event.preventDefault();     
>+      ]]></handler>
>     </handlers>
>   </binding>


r=mano.
Attachment #230216 - Flags: review?(bugs.mano) → review+
Assignee: sspitzer → ventnors_dogs234
Target Milestone: --- → Firefox 2 beta2
Comment on attachment 230216 [details] [diff] [review]
Patch preventDefault()

Oh, you also need to do the content-part changes to the pinstripe binding.

http://lxr.mozilla.org/seamonkey/source/toolkit/themes/pinstripe/global/globalBindings.xml#49
Nits are fixed and tooltip added to pinstripe.
Attachment #230216 - Attachment is obsolete: true
Attachment #230294 - Flags: review?(bugs.mano)
Comment on attachment 230294 [details] [diff] [review]
Patch with nits fixed and pinstripe support

r=mano.
Attachment #230294 - Flags: review?(bugs.mano) → review+
Checked in on trunk:

mozilla/toolkit/content/widgets/tabbrowser.xml 1.172
mozilla/toolkit/locales/en-US/chrome/global/tabbrowser.dtd 1.3
mozilla/toolkit/themes/pinstripe/global/globalBindings.xml 1.11
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Attachment #230294 - Flags: approval1.8.1?
Attachment #230216 - Flags: review+
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060723 Minefield/3.0a1 ID:2006072304 [cairo]

landed ?
where is "List all tabs"
screenshot: http://img77.imageshack.us/img77/3417/alltabsfq5.jpg
The "List all tabs" tooltip is on the tabs menubutton, on the far right of the tabbar.
(In reply to comment #24)
> The "List all tabs" tooltip is on the tabs menubutton, on the far right of the
> tabbar.
> 

I found it, thanks.
Comment on attachment 230294 [details] [diff] [review]
Patch with nits fixed and pinstripe support

a=drivers. Please go ahead and land this on the branch.
Attachment #230294 - Flags: approval1.8.1? → approval1.8.1+
1.8 branch:
mozilla/toolkit/content/widgets/tabbrowser.xml 1.103.2.58
mozilla/toolkit/locales/en-US/chrome/global/tabbrowser.dtd 1.2.18.1
mozilla/toolkit/themes/pinstripe/global/globalBindings.xml 1.3.12.5
Keywords: fixed1.8.1
This patch appears to crash trunk builds and since today, also branch: 
@ nsFrameManager::CaptureFrameStateFor 
On branch, the tooltip needs an extra click to disappear.
Depends on: 345659
I backed this out (except the dtd change) from the branch due to the crash reported in bug 345659:
Checking in toolkit/content/widgets/tabbrowser.xml;
/cvsroot/mozilla/toolkit/content/widgets/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.103.2.63; previous revision: 1.103.2.62
done
Checking in toolkit/themes/pinstripe/global/globalBindings.xml;
/cvsroot/mozilla/toolkit/themes/pinstripe/global/globalBindings.xml,v  <--  globalBindings.xml
new revision: 1.3.12.6; previous revision: 1.3.12.5
done

and from the trunk:
Checking in toolkit/content/widgets/tabbrowser.xml;
/cvsroot/mozilla/toolkit/content/widgets/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.179; previous revision: 1.178
done
Checking in toolkit/themes/pinstripe/global/globalBindings.xml;
/cvsroot/mozilla/toolkit/themes/pinstripe/global/globalBindings.xml,v  <--  globalBindings.xml
new revision: 1.12; previous revision: 1.11
done
Status: RESOLVED → REOPENED
Keywords: fixed1.8.1
Resolution: FIXED → ---
Depends on: 336662
No longer depends on: 345659
OK, I could not get it working using our old method no matter what I tried. So instead I used the tooltiptext attribute, which is remarkably more stable than a tooltip element. This patch uses a mouseover handler, and mutates the tooltiptext attribute of the button accordingly.
Attachment #230294 - Attachment is obsolete: true
Attachment #230708 - Flags: review?(bugs.mano)
PS, could someone backout my dtd changes? This new method uses tabbrowser.properties instead.
Status: REOPENED → ASSIGNED
No longer depends on: 336662
Attachment #230708 - Flags: review?(bugs.mano) → review?(beltzner)
Comment on attachment 230708 [details] [diff] [review]
Patch using tooltiptext mutations

Michael, beltzner can't review this.
Attachment #230708 - Flags: review?(beltzner)
(In reply to comment #32)
> (From update of attachment 230708 [details] [diff] [review] [edit])
> Michael, beltzner can't review this.
> 

Can you recommend someone?
Me, but be patient ;)
Attachment #230708 - Flags: review?(bugs.mano)
(In reply to comment #34)
> Me, but be patient ;)
> 

Of all times you could've showed up, you show up now. :P
OK, I've set it to you.
You really should mention bug 345659 or this bug in the code if this approach is accepted.
Attachment #230708 - Attachment description: Patch using "tooltiptext" mutations → Patch using tooltiptext mutations
Comment on attachment 230708 [details] [diff] [review]
Patch using tooltiptext mutations

>Index: toolkit/content/widgets/tabbrowser.xml
>===================================================================

It would be better to use something like a tooltiplabel attribute instead of a stringbundle so we don't force another late-l10n change.

>     <handlers>
>       <handler event="TabSelect" action="this._handleTabSelect();"/>
>+      <handler event="mouseover"><![CDATA[
>+        if (event.originalTarget.parentNode == this.mAllTabsBox) {
>+          var tooltipText = this.mStringBundle.getString("tabs.listAllTabs");
>+          event.originalTarget.setAttribute("tooltiptext", tooltipText);
>+        } else {
>+          this.mAllTabsBox.childNodes[0].removeAttribute("tooltiptext");

On trunk (and on branch rsn) the toolbar button has its own annonid and corresponding field, please use it.

nit: get rid of breaces around single line blocks.
Attachment #230708 - Flags: review?(bugs.mano) → review-
Attached patch Patch with nits fixed (obsolete) — Splinter Review
>On trunk (and on branch rsn) the toolbar button has its own annonid and 
>corresponding field, please use it.

Anonid yes, but no field. But I've given it one anyway. This uses the dtd changes you've kept in there with nits fixed.
Attachment #230708 - Attachment is obsolete: true
Attachment #232101 - Flags: review?(bugs.mano)
Comment on attachment 232101 [details] [diff] [review]
Patch with nits fixed

This does not work, for two reasons:

 * You didn't add the attribute in the Pinstripe tabbrowser-tabs binding.
 * originalTarget refers to child nodes of the all tabs buttons (the dropmarker or the button usually). AFAICT event.target is what you're looking for, please test.
Attachment #232101 - Flags: review?(bugs.mano) → review-
(In reply to comment #39)
> (From update of attachment 232101 [details] [diff] [review] [edit])
> This does not work, for two reasons:
> 
>  * You didn't add the attribute in the Pinstripe tabbrowser-tabs binding.
>  * originalTarget refers to child nodes of the all tabs buttons (the dropmarker
> or the button usually). AFAICT event.target is what you're looking for, please
> test.
> 

No, actually. I have tested this by having Fx alert() to me the class attribute of whatever I mouseover, and I can safely tell you that event.originalTarget points only to the button and is definitely 110% without a shadow of a doubt what I am looking for. event.target actually points to the tabbrowser-tabs binding itself, so there is no way for me to differentiate what element the user is mousing over.

I have tested this patch and it works. It just works perfectly, so I can see absolutely no reason now for this to not go on the trunk. However I did put in Pinstripe support like you asked. :)
Attachment #232101 - Attachment is obsolete: true
Attachment #232317 - Flags: review?(bugs.mano)
It doesn't work for me, I'll test again tomorrow though,
Comment on attachment 232317 [details] [diff] [review]
Patch with pinstripe support

OK, sorry for false-alarm, this didn't work here only due the missing Pinstripe update in the last patch.

r=mano.
Attachment #232317 - Flags: review?(bugs.mano) → review+
mozilla/toolkit/content/widgets/tabbrowser.xml 1.190
mozilla/toolkit/themes/pinstripe/global/globalBindings.xml 1.15
Status: ASSIGNED → RESOLVED
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
Comment on attachment 232317 [details] [diff] [review]
Patch with pinstripe support

Drivers: the l10n change for this was checked in earlier in the beta cycle.
Attachment #232317 - Flags: approval1.8.1?
Comment on attachment 232317 [details] [diff] [review]
Patch with pinstripe support

a=drivers for checkin on the 181 branch.
Attachment #232317 - Flags: approval1.8.1? → approval1.8.1+
1.8 branch:
mozilla/toolkit/content/widgets/tabbrowser.xml 1.103.2.82
mozilla/toolkit/themes/pinstripe/global/globalBindings.xml 1.3.12.11
Keywords: fixed1.8.1
in-litmus+: http://litmus.mozilla.org/show_test.cgi?id=4523

Verified FIXED using:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a7pre) Gecko/2007080208
Minefield/3.0a7pre

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a7pre) Gecko/2007080204
Minefield/3.0a7pre

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a7pre) Gecko/2007080204
Minefield/3.0a7pre
Status: RESOLVED → VERIFIED
Flags: in-litmus+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: