Closed
Bug 345257
Opened 18 years ago
Closed 18 years ago
add tooltip to "all tabs" menu button
Categories
(Firefox :: Tabbed Browser, defect)
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)
4.54 KB,
patch
|
asaf
:
review+
beltzner
:
approval1.8.1+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•18 years ago
|
||
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.
Assignee | ||
Comment 2•18 years ago
|
||
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 3•18 years ago
|
||
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-
Assignee | ||
Comment 4•18 years ago
|
||
(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.
Comment 5•18 years ago
|
||
What do you mean by "refer to the binding"?
Assignee | ||
Comment 6•18 years ago
|
||
(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.
Assignee | ||
Comment 7•18 years ago
|
||
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 9•18 years ago
|
||
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-
Assignee | ||
Comment 10•18 years ago
|
||
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 11•18 years ago
|
||
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-
Assignee | ||
Comment 12•18 years ago
|
||
(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.
Assignee | ||
Comment 13•18 years ago
|
||
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)
Comment 14•18 years ago
|
||
Why did you replace it? We generally use preventDefault and stopPropagation in chrome code rather then returning false...
Comment 15•18 years ago
|
||
Oh, Er, Yeah - I guess you need preventDefault rather than stopPropagation considering the way popupshowing is fired.
Comment 16•18 years ago
|
||
Not going to block on this, but will consider a safe patch.
Flags: blocking-firefox2? → blocking-firefox2-
Assignee | ||
Comment 17•18 years ago
|
||
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 18•18 years ago
|
||
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+
Updated•18 years ago
|
Assignee: sspitzer → ventnors_dogs234
Target Milestone: --- → Firefox 2 beta2
Comment 19•18 years ago
|
||
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
Assignee | ||
Comment 20•18 years ago
|
||
Nits are fixed and tooltip added to pinstripe.
Attachment #230216 -
Attachment is obsolete: true
Attachment #230294 -
Flags: review?(bugs.mano)
Comment 21•18 years ago
|
||
Comment on attachment 230294 [details] [diff] [review] Patch with nits fixed and pinstripe support r=mano.
Attachment #230294 -
Flags: review?(bugs.mano) → review+
Comment 22•18 years ago
|
||
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
Updated•18 years ago
|
Attachment #230294 -
Flags: approval1.8.1?
Updated•18 years ago
|
Attachment #230216 -
Flags: review+
Comment 23•18 years ago
|
||
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
Comment 24•18 years ago
|
||
The "List all tabs" tooltip is on the tabs menubutton, on the far right of the tabbar.
Comment 25•18 years ago
|
||
(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 26•18 years ago
|
||
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+
Keywords: late-l10n
Comment 27•18 years ago
|
||
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
Comment 28•18 years ago
|
||
This patch appears to crash trunk builds and since today, also branch: @ nsFrameManager::CaptureFrameStateFor On branch, the tooltip needs an extra click to disappear.
Comment 29•18 years ago
|
||
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
Updated•18 years ago
|
Assignee | ||
Comment 30•18 years ago
|
||
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)
Assignee | ||
Comment 31•18 years ago
|
||
PS, could someone backout my dtd changes? This new method uses tabbrowser.properties instead.
Status: REOPENED → ASSIGNED
Assignee | ||
Updated•18 years ago
|
Attachment #230708 -
Flags: review?(bugs.mano) → review?(beltzner)
Comment 32•18 years ago
|
||
Comment on attachment 230708 [details] [diff] [review] Patch using tooltiptext mutations Michael, beltzner can't review this.
Attachment #230708 -
Flags: review?(beltzner)
Assignee | ||
Comment 33•18 years ago
|
||
(In reply to comment #32) > (From update of attachment 230708 [details] [diff] [review] [edit]) > Michael, beltzner can't review this. > Can you recommend someone?
Comment 34•18 years ago
|
||
Me, but be patient ;)
Assignee | ||
Updated•18 years ago
|
Attachment #230708 -
Flags: review?(bugs.mano)
Assignee | ||
Comment 35•18 years ago
|
||
(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.
Comment 36•18 years ago
|
||
You really should mention bug 345659 or this bug in the code if this approach is accepted.
Assignee | ||
Updated•18 years ago
|
Attachment #230708 -
Attachment description: Patch using "tooltiptext" mutations → Patch using tooltiptext mutations
Comment 37•18 years ago
|
||
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-
Assignee | ||
Comment 38•18 years ago
|
||
>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 39•18 years ago
|
||
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-
Assignee | ||
Comment 40•18 years ago
|
||
(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)
Comment 41•18 years ago
|
||
It doesn't work for me, I'll test again tomorrow though,
Comment 42•18 years ago
|
||
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+
Comment 43•18 years ago
|
||
mozilla/toolkit/content/widgets/tabbrowser.xml 1.190 mozilla/toolkit/themes/pinstripe/global/globalBindings.xml 1.15
Status: ASSIGNED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
Comment 44•18 years ago
|
||
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 45•18 years ago
|
||
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+
Comment 46•18 years ago
|
||
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.
Description
•