588817 - Using "Bookmark all tabs" initially mislabels new folder menuitem in Bookmarks menu

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Marcia Knous [:marcia]]

Attachment: Screenshot of Bookmarks menu after trying to Bookmark All Tabs with App Tabs

Seen while running Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b4) Gecko/20100818 Firefox/4.0b4

STR:
1. Have a mixture of App Tabs and regular Tabs
2. Go to the tab context menu or the Bookmarks widget and select "Bookmark All Tabs"
3. See attached screenshot - you lose the ability to name the folder.

In Bug 587295, Alex states that Bookmark All tabs will remain, so this issue will need to fixed.

Comment 1

[Marcia Knous [:marcia]]

To clarify, the "Test Folder" in the screenshot was done when there were no App Tabs. As soon as I added an App Tab and tried the same thing, you can see the last entry is named "Folder Name."

Comment 2

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

Hm. Seems to me that App Tabs should be exempt from bookmarking all tabs, yes?

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #2)
> Hm. Seems to me that App Tabs should be exempt from bookmarking all tabs, yes?

I would think, yes.

Comment 4

[Dão Gottwald [:dao]]

I can't reproduce this bug.

Comment 5

[Pim Schellart]

I can reproduce the bug (also on OSX with Firefox 4.0 beta 6), the name of the folder changes from [Folder Name] to the name given only after a restart of Firefox.
If app tabs are exempt from bookmark all tabs (which I think they should be) it would be nice to have something like "bookmark all app-tabs" or create a default app-tabs bookmarks folder automatically to easily recreate app-tabs that were close accidentally.

Comment 6

[Pim Schellart]

Oh, I just noticed that the [Folder Name] is only unchanged in the Bookmarks menu. In the menu called when clicking the bookmarks button (the one next to the feedback button) the new name does appear correctly.

Comment 7

[:Margaret Leibovic]

(In reply to comment #6)
> Oh, I just noticed that the [Folder Name] is only unchanged in the Bookmarks
> menu. In the menu called when clicking the bookmarks button (the one next to
> the feedback button) the new name does appear correctly.

I agree with this. However, I don't think the [Folder Name] issue is a problem with app tabs, because I can reproduce it by bookmarking all tabs, even when I don't have any app tabs open.

Comment 8

[:Margaret Leibovic]

I'm updating the summary to describe the actual problem.

I found that when I opened a new window, the menuitem was correctly named, so the problem just seems to involve the label for the new folder menuitem when it's inserted into the existing menupopup.

Comment 9

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

So we're actually creating the items before the dialog is even dismissed. You can see this happen by leaving the library window open and/or looking at the bookmarks menu in a different menu. Though 3.6 does this too, so perhaps this is just a red herring.

Marco, do you know of anything that might have changed here?

If not... My other guess is that something changed in cocoa menu code so we're not updating menu items after they're added.

Comment 10

[Marco Bonardo [:mak]]

I can guess this is a regression from bug 528884, we had code in place before that change that was forcing binding application on os x native menubar. Mano removed it because arguably it's not needed to force a binding that does not exist. But looks like the issue is more complicated than that.

The fact we create stuff before dismissing the dialog is usual, this is an instant-apply dialog.

the titles changes are handled in browserPlacesViews.js nodeTitleChanged, the menu is created in browser-menubar.inc. There is some special handling for Mac native menubar (see this._nativeView), it's possible it has to be used in other points.

Comment 11

[Marco Bonardo [:mak]]

interesting, nodeTitleChanged is called and elt.label is set to the right name, but looks like it's a no-op (and before setting it elt.label is undefined rather than "[New Folder]").
As the old bindings issue we had, the menu binding is applied lazily on mac native menus, thus we should not use label property, but label attribute.
Using the attribute works fine, looks like we already fixed that in other points for our views. IIRC this issue was on the plate for xbl2 or something like that.

Comment 12

[Marco Bonardo [:mak]]

also, we have a test for this (browser_views_liveupdate.js), but looks like the way used to open the bookmarks menu popup (synthesizing popupshowing to it) workarounds the native menubar binding issue.
Testing the native menubar is hard, testing menus is hard globally since prone to random failures (menus closing or not opening due to focus changes)

So Margaret if you wish to fix it you could just s/.label/.setAttribute("label" in browserPlacesViews.js. If you find a way to test changes in the native menubar you get bonus points. I can review the change.
Otherwise I can make the same patch, doubt I can investigate another test (I spent time doing that when writing the above b-c test and did not find anything really working out of the box)

Comment 13

[:Margaret Leibovic]

Attachment: patch

Yes, this change fixes the problem. I didn't look into testing the native menubars because I have a bunch of other bugs on my plate right now, but maybe I will look into it later if I have free time.

Comment 14

[Marco Bonardo [:mak]]

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

Sounds good, thanks!

Comment 15

[:Margaret Leibovic]

Attachment: test update

There was a test failure when I pushed to try, but updating this test fixes the problem.

Comment 16

[:Margaret Leibovic]

http://hg.mozilla.org/mozilla-central/rev/e614fa32b2e2
http://hg.mozilla.org/mozilla-central/rev/813f49e7a76e