345261 - show status text when mousing over all tabs menu items

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P2)

Description

[(not reading, please use [e-mail] instead)]

show status text when mousing over all tabs menu items

from tabbrowser.xml

+            // XXX todo
+            // statustext not working yet, since I don't have a menubar
+            // reuse the menubar statustext logic

from irc:

<sspitzer> about the status text, I'd like to re-factor the menubar statusbar code in a way that I can use it here, too.
<sspitzer> but that will be a spin off bug.
<Mano> note: you cannot assume there's a statusbar in the window
show status text when mousing over all tabs menu items
<sspitzer> it could be hidden, but the menubar statusbar code should already handle this.
<Mano> you can't even force it if you find out that there's a statusbar
<Mano> becuase, well, this code lives in toolkit for some reason
<Mano> (tabbrowser)
<sspitzer> I can't force what?
<sspitzer> setting the status text?
<Mano> yeah
<Mano> though i guess you can add a statusbar property to tabbrowser

Comment 1

[Peter Kasting]

Darin had some crazy branch interface that made it possible to goof around with status text more.  I wish I could remember which bug that was.

Comment 2

[Darin Fisher]

Trunk only:
http://lxr.mozilla.org/mozilla/source/xpfe/appshell/public/nsIXULBrowserWindow.idl#63

Ugly branch version is in bug 324642.  I never checked it in.

Comment 3

[(not reading, please use [e-mail] instead)]

note to self, once this works, we need to make sure we update statustext when the uri changes.

see bug #345260

Comment 4

[Asaf Romano (gone)]

Taking, but i'm going to question the cons vs pros here. As in:
  1. Activating a menuitem in the menu while a tab is loading will replace the current stauts text with the activated item URL.
  2. While a menuitem is active, the statustext might be changed once the loading status of a tab has changed.

Comment 5

[Peter Kasting]

(In reply to comment #4)
>   1. Activating a menuitem in the menu while a tab is loading will replace the
> current stauts text with the activated item URL.

That seems like a pro to me.  Doesn't the same thing happen when you hover a link in a page?

>   2. While a menuitem is active, the statustext might be changed once the
> loading status of a tab has changed.

Due to redirects, etc.?  Yeah, that might look kinda unexpected, but perhaps not illogical.  I'd certainly like to see it in action.

Comment 6

[Asaf Romano (gone)]

(In reply to comment #5)
   2. While a menuitem is active, the statustext might be changed once the
> > loading status of a tab has changed.
> 
> Due to redirects, etc.?  Yeah, that might look kinda unexpected, but perhaps
> not illogical.  I'd certainly like to see it in action.
> 

Much more common case: the tab loading state is changed ("Connecting", "Waiting", "Transfering data" etc).

Comment 7

[Peter Kasting]

(In reply to comment #6)
> Much more common case: the tab loading state is changed ("Connecting",
> "Waiting", "Transfering data" etc).

I thought the tooltip would show the URL of the tab in question, not a title/status/whatever.  I agree that seeing the tab's status would be totally bizarre and weird.  I just want to see the URL, that's all.  (The status in many cases would just be "done" anyway...)

Comment 8

[Asaf Romano (gone)]

You didn't parse this right: say you activate a menuitem while the current tab is "Connecting...", the statusbar text would change from "Connecting..." to the url of the activated item. Now, while the item is activated, the current tab state is changed to "Waiting...", the statusbar text will be replaced from the url of the activated item to "Waiting...".

Comment 9

[Peter Kasting]

(In reply to comment #8)
> You didn't parse this right: say you activate a menuitem while the current tab
> is "Connecting...", the statusbar text would change from "Connecting..." to the
> url of the activated item. Now, while the item is activated, the current tab
> state is changed to "Waiting...", the statusbar text will be replaced from the
> url of the activated item to "Waiting...".

Ah, you're right, I was mistaken.

So, once again, what happens currently in the case of hovering a link on a page that hasn't finished loading, or something like that?  If we're broken there then this would be no worse than that, and one fix could solve both issues at the same time.

(Is the solution here some way to "lock the statusbar"?)

Comment 10

[Asaf Romano (gone)]

*** Bug 347603 has been marked as a duplicate of this bug. ***

Comment 11

[Asaf Romano (gone)]

Attachment: patch

This takes care of the issues I pointed out earlier in this bug, i.e. the browser implementation of updateStatusField gives higher priority to hovered links.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 233437 [details] [diff] [review]
patch

Yup, this looks like the right behaviour to me.

Comment 13

[Asaf Romano (gone)]

Attachment: updated

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 239069 [details] [diff] [review]
updated

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

>+      <method name="_onMenuItemActive">
>+        <parameter name="aEvent"/>
>+        <body><![CDATA[
>+          var tab = aEvent.target.tab;
>+          if (tab) {
>+            var statusText = tab.linkedBrowser.currentURI.spec;
>+            if (statusText == "about:blank") {
>+              // XXXhack: Passing a space here (and not "")
>+              // to make sure the the browser implementation would
>+              // still consider it a hovered link.
>+              statusText = " ";
>+            }
>+
>+            window.XULBrowserWindow.setOverLink(url, null);
>+          }
>+        ]]></body>
>+      </method>
>+
>+      <method name="_onMenuItemInactive">
>+        <parameter name="aEvent"/>
>+        <body><![CDATA[
>+          window.XULBrowserWindow.setOverLink("", null);
>+        ]]></body>
>+      </method>

Any reason you didn't inline these in the <handler>?

I don't think it's a good idea to be relying on XULBrowserWindow being defined in the global scope, that's only true for our browser. Could we instead do something like what menubar does, and have a statusbar attribute on the tabbrowser? At the very least, you should fail gracefully if it isn't defined. None of this is really a problem if tabbrowser moves out of toolkit, but until it does...

Comment 15

[Asaf Romano (gone)]

Attachment: patch

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 239421 [details] [diff] [review]
patch

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

>+      <constructor><![CDATA[
>+        // We cannot cache the XULBrowserWindow object itself since it might
>+        // be set after this binding is constructed.
>+        try {
>+          this._xulWindow = 
>+            window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                  .getInterface(Components.interfaces.nsIWebNavigation)
>+                  .QueryInterface(Components.interfaces.nsIDocShellTreeItem)
>+                  .treeOwner
>+                  .QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+                  .getInterface(Components.interfaces.nsIXULWindow);
>+        }
>+        catch(ex) { }
>+      ]]></constructor>

I wonder if perhaps the XULBrowserWindow should be gotten lazily (i.e. have it's own readonly property that does caching itself), to minimize startup cost, and avoid having to cache the xulWindow (and simplify the checks in the handlers). I guess it probably doesn't matter.

>+      <method name="_onMenuItemActive">
>+        <parameter name="aEvent"/>
>+        <body><![CDATA[
>+          if (!this._xulWindow || !this._xulWindow.XULBrowserWindow)
>+            return;
>+
>+          var tab = aEvent.target.tab;
>+          if (tab) {
>+            var statusText = tab.linkedBrowser.currentURI.spec;
>+            if (statusText == "about:blank") {
>+              // XXXhack: Passing a space here (and not "")
>+              // to make sure the the browser implementation would
>+              // still consider it a hovered link.
>+              statusText = " ";

File a bug on this? nsBrowserStatusHandler should probably default overLink to null and explicitly compare to null in updateStatusField instead of ignoring "" values.

>-      <handler event="popupshowing"><![CDATA[
>-        if (event.target == this)
>-          this._onShowingAllTabsPopup();
>-      ]]></handler>
>-      <handler event="popuphiding"><![CDATA[
>-        if (event.target == this)
>-          this._onHidingAllTabsPopup();
>-      ]]></handler>
>+      <handler event="popupshowing" action="this._onShowingAllTabsPopup();"/>
>+      <handler event="popuphiding" action="this._onHidingAllTabsPopup();"/>
>+      <handler event="DOMMenuItemActive" action="this._onMenuItemActive(event);"/>
>+      <handler event="DOMMenuItemInactive" action="this._onMenuItemInactive(event);"/>
>     </handlers>
>   </binding>

I still prefer having the handlers inline, like all the other handlers in this file.

Comment 17

[Asaf Romano (gone)]

(In reply to comment #16)

> I wonder if perhaps the XULBrowserWindow should be gotten lazily (i.e. have
> it's own readonly property that does caching itself), to minimize startup cost,
> and avoid having to cache the xulWindow (and simplify the checks in the
> handlers). I guess it probably doesn't matter.

I don't think this would be right, esp. for the case where XULBrowserWindow is changed ;).

> >+      <method name="_onMenuItemActive">
> >+        <parameter name="aEvent"/>
> >+        <body><![CDATA[
> >+          if (!this._xulWindow || !this._xulWindow.XULBrowserWindow)
> >+            return;
> >+
> >+          var tab = aEvent.target.tab;
> >+          if (tab) {
> >+            var statusText = tab.linkedBrowser.currentURI.spec;
> >+            if (statusText == "about:blank") {
> >+              // XXXhack: Passing a space here (and not "")
> >+              // to make sure the the browser implementation would
> >+              // still consider it a hovered link.
> >+              statusText = " ";
> 
> File a bug on this? nsBrowserStatusHandler should probably default overLink to
> null and explicitly compare to null in updateStatusField instead of ignoring ""
> values.

will do.

> 
> >-      <handler event="popupshowing"><![CDATA[
> >-        if (event.target == this)
> >-          this._onShowingAllTabsPopup();
> >-      ]]></handler>
> >-      <handler event="popuphiding"><![CDATA[
> >-        if (event.target == this)
> >-          this._onHidingAllTabsPopup();
> >-      ]]></handler>
> >+      <handler event="popupshowing" action="this._onShowingAllTabsPopup();"/>
> >+      <handler event="popuphiding" action="this._onHidingAllTabsPopup();"/>
> >+      <handler event="DOMMenuItemActive" action="this._onMenuItemActive(event);"/>
> >+      <handler event="DOMMenuItemInactive" action="this._onMenuItemInactive(event);"/>
> >     </handlers>
> >   </binding>
> 
> I still prefer having the handlers inline, like all the other handlers in this
> file.
> 

I'll clean this up in a separate bug.

Comment 18

[Asaf Romano (gone)]

mozilla/toolkit/content/widgets/tabbrowser.xml 1.202