213163 - show bookmark URL over mouseover of bookmark toolbar item in status bar

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, enhancement, P4)

Description

[Chris Neale]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5a cdn) Gecko/20030718 Mozilla Firebird/0.6
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.5a cdn) Gecko/20030718 Mozilla Firebird/0.6

Mozilla Firebird implementation of bug 23460 : Bookmarks URL should show in
status bar at mouseover of menuitem

Reproducible: Always

Steps to Reproduce:
1. Move mouse over a bookmark on the Personal toolbar
2.
3.

Actual Results:  
see Status text does not change to URI of bookmark

Expected Results:  
URI of bookmark shown in statusbar

Comment 1

[Chris Neale]

Attachment: Patch for browser.xul / bookmarksMenu.js to show URI of bookmarks in statusbar

based on Tim Powell's patch for Seamonkey :
http://worldtimzone.com/mozilla/bugs/bug23460/

Comment 2

[Mike Connor [:mconnor]]

Confirming, patch looks good.

Comment 3

[Mike Connor [:mconnor]]

taking QA contact, sorry about the bugspam

Comment 4

[Pham]

I saw this fixed in Bonsai about weeks ago but finally got a build. Mr QA please
verify since this should've been marked a while ago and I don't understand why
it's not marked.

Comment 5

[Mike Connor [:mconnor]]

thanks, I saw this fixed, but in my little "clean up after hyatt" rounds I
missed some :)

Comment 6

[Chris Neale]

did Bonsai report that 

mozilla/browser/base/content/browser.xul or
mozilla/browser/components/bookmarks/content/bookmarksMenu.js had been altered ?

afaict this particular bug hasn't been touched

those checkins must have been for Seamonkey

Comment 7

[Chris Neale]

Comment on attachment 128069 [details] [diff] [review]
Patch for browser.xul / bookmarksMenu.js to show URI of bookmarks in statusbar

browser.xul change in bookmarksMenu.js change not in afaict

Comment 8

[Chris Neale]

Attachment: patch for bookmarksMenu.js

Comment 9

[Mike Connor [:mconnor]]

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/browser/base/content&command=DIFF_FRAMESET&file=browser-menubar.inc&rev1=1.1&rev2=1.2&root=/cvsroot
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/browser/base/content&command=DIFF_FRAMESET&file=browser.xul&rev1=1.201&rev2=1.202&root=/cvsroot
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/toolkit/content/widgets&command=DIFF_FRAMESET&file=toolbar.xml&rev1=1.26&rev2=1.27&root=/cvsroot

Chris, hyatt checked in at 08/12/2003 18:55, however he did this in browser.xml
and toolbar.xml, which is why the files in question didn't change.

Re-resolving

This is actually working in current builds.

Comment 10

[Mike Connor [:mconnor]]

Okay, I see, you want all mouseovers of bookmarks in any menu to do this.  Hyatt
intended only for bookmarks in the Bookmarks menu to have this, which is what he
implemented.  The BTF bookmarks do not trigger the statusbar change.

Reopening for now.

Comment 11

[Chris Neale]

hyatt has a time machine ?

as for on other menuitems, I'm not sure

I missed the other implementation, only tried after you pointed it out

haven't followed bonsai for a bit, just been blindly building dailies

Comment 12

[Pham]

is comment 10 the only thing left for this bug?

Comment 13

[psellhorst]

This should also apply to bookmarks inside folders on the bookmarks toolbar.

Comment 14

[Tuukka Tolvanen (sp3000)]

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

Comment 15

[Mike Connor [:mconnor]]

not a 0.9 blocker, plussing for 1.0 to get on review radar

Comment 16

[Walter K.]

Bug 239429 is this bug's cousin in the Bookmarks sidebar

Comment 17

[Tim Powell]

Attachment: Patch for bookmarks toolbar menus and buttons

Following Hyatt's example, I created a fix that uses XUL/XBL. This extends
statusbar support to toolbarbuttons. This could be used to enhance the back and
forward button and their submenus so that they'd show the URL in the statusbar
(bug 88541 for Mozilla--is there one for Firefox?) much like the Go menu.

This was patched against the files in the chrome jars for Firefox 0.9. I
believe toolbarbutton.xml has a different location in the source tree.

Comment 18

[Chris Neale]

Attachment: reworking of attachment 151923 so that it alters files in AVIARY_1_0_20040515_BRANCH branch of cvs tree

Comment 19

[Chris Neale]

Attachment: diffed too soon, should have cvs upped first

Comment 20

[Tim Powell]

Thanks, Chris. The patch in comment #19 looks to be the right files and the same
changes as my original patch. What's next? Who can review this?

Comment 21

[Steffen Wilberg]

-> vlad. There's a patch in here.

Comment 22

[Asa Dotzler [:asa]]

p4 priority - not a blocker. if a fully reviewed patch materializes, please
nominate for aviary approval. 

Comment 23

[Phil Ringnalda (:philor)]

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

Comment 24

[Walter K.]

Patch needs update because of recent Livemarks changes. I've applied it manually
to FF PR, and it works as expected.

Comment 25

[Phil Ringnalda (:philor)]

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

Comment 26

[Chris Neale]

Attachment: Update of attachment 152063 - takes Livemarks into account

May be woolly

Comment 27

[Walter K.]

(In reply to comment #26)
> Created an attachment (id=167774) [edit]
> Update of attachment 152063 [details] [diff] [review] [edit] - takes Livemarks into account
> 
> May be woolly

Great job again! The patch applies to FF 1.0 with no problems. Can you set
review flag on the patch, so the patch can be reviewed and checked in?

Comment 28

[Andreas Wuest]

As of today (Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050523
Firefox/1.0+), the urls for bookmarks (top-level as well as bookmarks contained
in a folder, or sub-folder, etc.) are still not shown in the 'status bar'. At
least on arch: PC, OS: Linux.

Neither does the feature as mentioned in comment #17.

Comment 29

[sairuh (rarely reading bugmail)]

(In reply to comment #28)
> As of today (Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b2) Gecko/20050523
> Firefox/1.0+), the urls for bookmarks (top-level as well as bookmarks contained
> in a folder, or sub-folder, etc.) are still not shown in the 'status bar'. At
> least on arch: PC, OS: Linux.

hm, odd: using 2005052311-trunk (deer park alpha 1) on linux fc3 (gnome), I see
the urls for bookmarks in the bookmarks main menu (both toplevel and children of
bookmark folders).

however, I don't see the urls of bookmarks in either the bookmarks toolbar or
the bookmarks sidebar, when I do a mouseover.

Comment 30

[Andreas Wuest]

(In reply to comment #29)

> hm, odd: using 2005052311-trunk (deer park alpha 1) on linux fc3 (gnome), I see
> the urls for bookmarks in the bookmarks main menu (both toplevel and children of
> bookmark folders).
> 
> however, I don't see the urls of bookmarks in either the bookmarks toolbar or
> the bookmarks sidebar, when I do a mouseover.

Yes, that's exactly what I am talking about. URLs are not desplayed in the
'status bar' when hovering over bookmarks in the 'bookmarks toolbar'.

This is with a trunk checkout from about 5 hours ago.

Comment 31

[Mike Connor [:mconnor]]

Sairuh, this was established a long time ago.  Hyatt implemented for the menu
only, and never got the other bits done. :-/

Comment 32

[Andreas Wuest]

Ok, I should have a patch ready by the end of the week (say, monday evening,
just for the case...).

Comment 33

[Andreas Wuest]

(In reply to comment #32)

> Ok, I should have a patch ready by the end of the week (say, monday evening,
> just for the case...).

So, I already start talking to myself...

Sorry for the delay, but <rant>why is there no possibility to add multiple
bindings independent of each other?? It's simply a pain in the ass to always
track back every possible binding chain, and then insert the new binding into
this hierarchy. But even worse, this prevents code reuse, and renders binding
hierarchies basicly unreadable. It is simply not possible to add any orthogonal
functionality, which does not have to be linked into the existing binding
hierarchy. Of course I see the issues of multiple inheritance, but this could be
solved by linking the additional bindings transparently into the hierarchy. And
don't tell me this is what document.addBinding is for. This little bitch simply
crashes when called (ah, who said bug
https://bugzilla.mozilla.org/show_bug.cgi?id=51261 was fixed??)!</rant>

Now, I first have to workaround this bug, maybe by adding events manually. If
someone could point out a better way to add a binding, please speak up.

Comment 34

[Mike Connor [:mconnor]]

if 296375 materialized in time to get a low-risk patch before beta, we can take
a look, but this isn't a blocker.

Comment 35

[Frank Wein [:mcsmurf]]

BTW: Take a look at your review request again, it says "(BM, do not Cc or
Request) ", so you won't get any review from vlad this way (try requesting
review from :vlad).

Comment 36

[Phil Ringnalda (:philor)]

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

Comment 37

[Mike Connor [:mconnor]]

Not a blocker, this is a small polish fix at best.

Comment 38

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

Comment on attachment 167774 [details] [diff] [review]
Update of attachment 152063 [details] [diff] [review] - takes Livemarks into account

this patch is no longer applicable (needs to be written against Places)

Comment 39

[Walter K.]

Attachment: Updated patch against MOZILLA_1_8_BRANCH

I've updated the previous patch against MOZILLA_1_8_BRANCH. There is still time for it to make into FF2.0.

Comment 40

[Walter K.]

Attachment: Updated patch against latest MOZILLA_1_8_BRANCH only.

This is an updated patch for FF2. It has a minimal regression risk and addresses an annoying usability issue. Please consider before FF final ships out.

Comment 41

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

Comment on attachment 234523 [details] [diff] [review]
Updated patch against latest MOZILLA_1_8_BRANCH only.

I can't review this - you likely want to ask "mconnor@moz" or "bugs.mano@".

Comment 42

[Asaf Romano (gone)]

Comment on attachment 234523 [details] [diff] [review]
Updated patch against latest MOZILLA_1_8_BRANCH only.

We shouldn't change the  toolbarbutton binding in order to implement something like this (there are statusbar utils in browser.js, setOverLink in particular). Also, considering the way this is implemented, the statustext might be overwritten while you've hovering an item.

Using setOverLink should address these issues.

Comment 43

[Michael Schonfeld]

Attachment: patch

Please note: The patch will not show the URI of a menuitem due to a different bug. Will open a followup.

Comment 44

[Asaf Romano (gone)]

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

>Index: browser/components/places/content/toolbar.xml
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/toolbar.xml,v
>retrieving revision 1.112
>diff -u -p -8 -r1.112 toolbar.xml
>--- browser/components/places/content/toolbar.xml	14 Dec 2007 21:10:38 -0000	1.112
>+++ browser/components/places/content/toolbar.xml	25 Dec 2007 01:37:03 -0000
>@@ -992,16 +992,27 @@
>             parent = parent.parentNode;
>           }
>           return false;
>         ]]></body>
>       </method>
>     </implementation>
> 
>     <handlers>
>+      <handler event="mouseover"><![CDATA[
>+        var button = event.target;
>+        if (button.parentNode == this && button.node &&
>+            PlacesUtils.nodeIsURI(button.node))
>+          window.XULBrowserWindow.setOverLink(event.target.node.uri, null);
>+        else
>+          window.XULBrowserWindow.setOverLink("", null);

I think the mouseout handler covers the else case.

r=mano with that removed..

Comment 45

[Michael Schonfeld]

Attachment: v1.2

Thanks for the review, Mano!

Same deal, no {else}. 
r=mano.

Comment 46

[Michael Schonfeld]

Attachment: For checkin

Comment 47

[Asaf Romano (gone)]

mozilla/browser/components/places/content/toolbar.xml 1.120

Comment 48

[ojab]

I think that "Home" button URL should be shown too, but now I see "Done" in status bar when mouse over Home bookmark.

Comment 49

[William R. "Xer" Cope]

It's fixed for items directly on the bookmarks toolbar, but items within sub-folders (inc. Smart Bookmarks and feeds) still do not give status bar feedback.
I'm assuming this is what the note in comment #43 is referring to (as it almost certainly is), but, as it's not completely clear, I'll say it anyway.

Comment 50

[Michael Schonfeld]

William:  Yea, that's gonna be Bug 409748.

Comment 51

[Steffen Wilberg]

So the URL now displays in the bookmarks menu and bookmarks toolbar.
Are you going to implement this for the bookmarks sidebar as well (bug 239429)?

Comment 52

[Michael Schonfeld]

Actually, it only shows the URL for the bookmarks toolbar, not menu.  Maybe we should open a follow-up bug for the bookmarks menu.

Comment 53

[Michael Schonfeld]

Opened a follow-up bug regarding both the bookmarks menu, and the toolbars chevron.  Bug 417938.