409748 - No statusbar-text for bookmarks toolbar sub-menu items

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Michael Schonfeld]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2007122404 Minefield/3.0b3pre
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2007122404 Minefield/3.0b3pre

Follow up to bug #213613.  While hovering over items inside sub menus within the bookmarks toolbar, the URI does not appear in the status bar.

Reproducible: Always

Steps to Reproduce:
1. Click on any menu on the bookmarks toolbar (Smart Bookmarks, Places, ...)
2. Hover over any item
Actual Results:  
Nothing changes/appears in the status bar

Expected Results:  
While hovering, the URI of the item should appear in the status bar

Comment 1

[Michael Schonfeld]

Oops.. I meant to write - Follow up to bug #213163.

Comment 2

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

confirming.

I'd wanted to say that this is a duplicate, but I couldn't find it with quick query.

Comment 3

[bomfog]

I put another "k" in boomarks in the summary, for searching convenience.

Comment 4

[Ria Klaassen (not reading all bugmail)]

Also bookmarks under the overflow chevron don't show up in the status bar.

Comment 6

[Michael Schonfeld]

Attachment: v1

Works for the chevron & other menu popups.

Comment 7

[Michael Schonfeld]

Attachment: v2

Toolbar.xml is responsible for displaying the URL of any toolbar item whether its just a bookmark, or a folder containing bookmarks.

Menu.xml is responsible for displaying the URL of any chevron item, including both folders and bookmarks.

Comment 8

[Asaf Romano (gone)]

Comment on attachment 303987 [details] [diff] [review]
v2

We should look into moving this code over to the new shard binding for places popups.

Comment 9

[Michael Schonfeld]

Attachment: v3

Mano: I'm not entirely sure as to what you meant by taking out the code into a shared place, because the placesMenuBindings binding doesn't handle the toolbar menu items.  Right now, toolbar.xml handles only the toolbars menu items (not folders!) and the menu.xml handles everything else, which include all folders, chevron menu items & chevron folders.

So, please let me know if I got it wrong...

Comment 10

[Marco Bonardo [:mak]]

actually places-popup-base handles all menus and submenus
while places-menupopup handles bookmarks menu (first level, the view)
toolbar.xml handles the toolbar while the chevron has it's own view (and the binding is still places-menupopup)

since places-menupopup extends places-popup-base using the latter will ensure that it will be fired on all menupopups. the toolbar is not a menupopup so it has to be served by its own

Comment 11

[Michael Schonfeld]

So in other words, I got it right?

Comment 12

[Asaf Romano (gone)]

Comment on attachment 306079 [details] [diff] [review]
v3

So, few concerns:
1. Use DOMMenuItemActive/Inactive events
2 . This probably makes the popuphidden unnecessary.
3. This _should_ make the statustext attribute set in createMenuItemForNode unnecessary as well.

Comment 13

[Asaf Romano (gone)]

And, "button" is a pretty odd variable-name for a menuitem ;)

Comment 14

[Michael Schonfeld]

Attachment: v3.2

Changing to DOMMenuActive/Inactive handlers also causes the Mac native menu bar in question (History & Bookmarks in this case) to display status bar text. This would be a great add-on, if not for another bug which prevents DOMMenuInactive from being fired for those native menus.  So, holding out for a review until resolution of bug 420033.

Comment 15

[Michael Schonfeld]

Attachment: v3.3

Same deal, better if-statements. (Trying to counteract some of Manos foreseeable nit comments!)

Comment 16

[Marco Bonardo [:mak]]

mmh, removing statustext attribute will break those themes that are using that to show different icons for bookmarklets (See bug 397263), so what about doing something like

      <handler event="mouseover"><![CDATA[
        var target = event.target;
        if (target.parentNode != this)
          return;
        var statustext = target.getAttribute("statustext");
        if (statustext)
          window.XULBrowserWindow.setOverLink(statustext, null);
      ]]></handler>

in toolbar.xml

Also in domMenuItemActive/inactive you could use the attribute, it exists if the node is an uri node, so we don't need to check again if that node is an uri. plus we don't break anything. If you want you can merge in the patch in Bug 397263 to add the attribute to toolbar buttons, so everything lives is in a single patch.

Comment 17

[Asaf Romano (gone)]

Marco: the statustext attribute is pretty buggy as far as i can tell, and slows the creation of very large bookmarks menus, I'm fine with removing it for now.

That said, if you want to add some css *class* for bookmarklets, that can happen in bug 397263.

Comment 18

[Michael Schonfeld]

Attachment: v3.4

Same as before, but hacked so it wouldn't show the status bar text for the History & Bookmarks menu bars in Mac OS, because chances are Bug 420033 won't be included in Firefox 3.

Comment 19

[Asaf Romano (gone)]

Comment on attachment 308602 [details] [diff] [review]
v3.4

r=mano

Comment 20

[Asaf Romano (gone)]

Comment on attachment 308602 [details] [diff] [review]
v3.4

hrm, sorry, i didn't realize that the this.id check wouldn't filter out items under submenus of the main bookmarks menu.

Comment 21

[Marco Bonardo [:mak]]

(In reply to comment #17)
> Marco: the statustext attribute is pretty buggy as far as i can tell, and slows
> the creation of very large bookmarks menus, I'm fine with removing it for now.

well, that was working fine however, but np we can use both ways

Comment 22

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

This will not block the final release of Firefox 3.

Comment 23

[Michael Schonfeld]

Attachment: v3.5

Same deal, different hack...

Comment 24

[Michael Schonfeld]

Attachment: v3.6

Comment 25

[Michael Schonfeld]

Attachment: v3.7

Comment 26

[Asaf Romano (gone)]

Comment on attachment 308688 [details] [diff] [review]
v3.7

Without the changes to toolbar.xml, r=mano. Thanks.

Comment 27

[Damon Sicore (:damons)]

Comment on attachment 308688 [details] [diff] [review]
v3.7

a1.9+=damons

Comment 28

[Marco Bonardo [:mak]]

this cannot be checkin-needed, you still need to address comment #26 (remove toolbar.xml changes)

Comment 29

[Michael Schonfeld]

Attachment: for checkin

Marco: Asaf had told to me that you can simply omit the changes to toolbar.xml during the checkin itself, thus there is no need for me to upload another patch.

But nevertheless, here is the updated patch.

Comment 30

[Marco Bonardo [:mak]]

still is better having the checked-in patch in the bug before resolving. Sorry Michael didn't want to hurt your work :)

Comment 31

[Reed Loden [:reed]]

Marco is right. Please always attach a final patch for check-in. It makes the checkin-needed people (such as myself) much happier.

Checking in browser/components/places/content/menu.xml;
/cvsroot/mozilla/browser/components/places/content/menu.xml,v  <--  menu.xml
new revision: 1.119; previous revision: 1.118
done
Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.120; previous revision: 1.119
done

Comment 32

[Michael Schonfeld]

Understandable.. Will do from now on!  Thanks guys...

Comment 33

[Reed Loden [:reed]]

Note that this patch missed the Mac nightly but made the Windows and Linux nightlies.

Comment 34

[Marco Bonardo [:mak]]

from my preliminary tests this seems to have caused Bug 422645

Comment 35

[Daniel.S]

Could this be responsible for Bug 423083 as well? It's at least in the regression range:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1205298240&maxdate=1205324099

Comment 36

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b5pre) Gecko/2008031704 Minefield/3.0b5pre and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008031704 Minefield/3.0b5pre

Comment 37

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv