512148 - Port Bug 88541 - Show URI in status bar onmouseover of Back/Forward menu items

Status: NEW

(SeaMonkey :: UI Design, enhancement)

Description

[Jens Hatlak (:InvisibleSmiley)]

Attachment: proposed patch

Comment 1

[[e-mail]]

Comment on attachment 396142 [details] [diff] [review]
proposed patch

>+    // Lazily add the hover listeners on first showing and never remove them
The problem here is that this is called from the Go menu, and if we're going to show status text for those items then it should be done using the toolkit menu status text functions for menubars.

>+        // Only the current page should have the checked attribute, so skip it
The current code doesn't actually add the uri to the current page, since that's handled by createRadioMenuItem, which you don't patch.

>+    menuitem.setAttribute( "uri", aURI );
Nit: I'd prefer you to use the .statusText property.

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2

(In reply to comment #1)
> (From update of attachment 396142 [details] [diff] [review])
> >+    // Lazily add the hover listeners on first showing and never remove them
> The problem here is that this is called from the Go menu, and if we're going to
> show status text for those items then it should be done using the toolkit menu
> status text functions for menubars.

Sorry, don't know what you mean. All I got was that I need to fix the Go menu as well (valid point, it only took me some time to understand that we're not talking about the Go button ;-)) which I did now.

Comment 3

[[e-mail]]

(In reply to comment #2)
> (In reply to comment #1)
> > The problem here is that this is called from the Go menu, and if we're going to
> > show status text for those items then it should be done using the toolkit menu
> > status text functions for menubars.
> Sorry, don't know what you mean. All I got was that I need to fix the Go menu
> as well (valid point, it only took me some time to understand that we're not
> talking about the Go button ;-)) which I did now.
I've never tried it, but apparently toolkit has built-in status text support for menubars, which we should try to use, rather than possibly conflicting with it.

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #3)
> I've never tried it, but apparently toolkit has built-in status text support
> for menubars, which we should try to use, rather than possibly conflicting with
> it.

After some checking: You are right, it's as simple as adding statusbar="statusbar-display" to the menubar element in navigator.xul (in addition to setting menuitem.statusText = aURI or menuitem.setAttribute("statusText",aURI)).

BUT this only works for menubars (it's in Toolkit's toolbar.xml BTW). We are trying to do it for both the Go menu (works that way) and the back/forward dropdowns (doesn't work that way).

If you like I can add the attribute statusbar="statusbar-display" in navigator.xul and add a comment in sessionHistoryUI.js's FillHistoryMenu() that we're only adding the listener there because of the back/forward buttons dropdowns. Or we stick with the existing patch.

Comment 5

[[e-mail]]

Well, it would be better if we could do adapt the toolkit menubar code for use with the back/forward buttons, but I guess we could do it this way instead.

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v3

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

BTW: I just noticed the navigator.xul change also makes it work for bookmarks in the Bookmarks menu, without any further change. :-)

Comment 8

[[e-mail]]

Comment on attachment 397926 [details] [diff] [review]
patch v3

>+    // Note: The Go menu doesn't need this
But you're adding it anyway? I think it would be better if the back/forward popups had separate calls to enable/disable status tracking.

>+        // Only the current page should have the checked attribute, so skip it
But this only applies to the Go menu, which uses the toolkit handler, so you'll have to change createRadioMenuItem or its caller.

>+        if (!aEvent.target.hasAttribute("checked"))
>+          XULBrowserWindow.setOverLink(aEvent.target.statusText);
>+      }, false);
>+      aParent.addEventListener("DOMMenuItemInactive", function() {
>+        XULBrowserWindow.setOverLink("");
I don't like this being inconsistent with the toolkit menubar way of doing things. You might have to adapt it slightly of course, because it uses DOMMenuBarActive/Inactive instead of popupshowing/hiding, and put the original status text in to a global. Ideally it would then be able to work with the Bookmarks button, and maybe even the whole Bookmarks toolbar!

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #7)
> BTW: I just noticed the navigator.xul change also makes it work for bookmarks
> in the Bookmarks menu, without any further change. :-)

As I wrote in bug 23460, that part seems to have been fixed as part of the transition to Places-based bookmarks.

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 397926 [details] [diff] [review]
patch v3

Effectively, the last review result was r-. Updating attachment details to reflect that, and unassigning myself since I'm not actively working on this.