575896 - Bookmark Toolbar context menu does not work after customize

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Biju]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a6pre)
Gecko/20100628 Minefield/3.7a6pre

Unable to create new items on "Bookmark Toolbar"

Steps:-
1. Start new Firefox profile
2. Enable "Bookmarks Toolbar"
3. Try to create a new folder, or bookmark, or separator 

Result:-
Unable create 

Expected:-
User should be able to create a new Item

Comment 1

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

(uhm, who set this as blocking1.9.3:beta1+? - it does, but not sure how someone did it)

This wfm on a 20100629 nightly, but is broken on beta 1. You can drag things to the bookmarks toolbar to create them, but the context menu doesn't seem to work.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #1)
> (uhm, who set this as blocking1.9.3:beta1+? - it does, but not sure how someone
> did it)

I think there is a bugzilla bug where if you clone a blocking bug it is marked as blocking. looks like serious.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #1)
> This wfm on a 20100629 nightly, but is broken on beta 1. You can drag things to
> the bookmarks toolbar to create them, but the context menu doesn't seem to
> work.

We did not push any additional change to the toolbar nor to related code after beta1 tag has been added. Could be that it just works fine after a restart?

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

So, I've create a new profile with beta1, from the bookmarks button I've choosen "View Bookmarks Toolbar", I've right clicked on the toolbar and choosen "New separator", the separator has been created. I can't reproduce, or I need better steps.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I can reproduce the issue this way:
right click on toolbars and choose Customize. Close customize dialog and choose View Bookmarks Toolbar. The context menu does not work anymore, restarting the browser fixes it. This would be a view bug due to the fact customize removes nodes from the document, but it does not look a blocker to me.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

actually, it's probably a bug we have from some time, and the patch in bug 388880 could fix it.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

simple fix for the beta, reset the view during customize.
Bug 388880 can provide better caching code on top of this if needed.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.1

toolbar elt can be removed, as well!

Comment 9

[Dão Gottwald [:dao]]

First of all, based on comment 5, this shouldn't block beta 1, so I'm resetting the flag.

Looking at the patch now...

Comment 10

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

Yup, agree - blocks final, and if we can get it in before we do a second build of beta1, we should.

Comment 11

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

Adding relnote just in case we don't get this in time for beta - hopefully we do!

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

looks like relnote is pointing to the wrong bug number?
See https://bugzilla.mozilla.org/show_bug.cgi?id=575932#c5

Comment 13

[Max Stepin]

Not fixed in Beta 1.

I was quite surprised that Bookmark->Properties dialog is gone.

All I wanted is to rename a bookmark.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 455122 [details] [diff] [review]
patch v1.1

>+  get _toolbar() {

Can you rename this to something that makes this not sound like we're dealing with a <toolbar>?

Comment 15

[Dão Gottwald [:dao]]

Also, did you mean to say that the patch in bug 388880 fixes this? If so, what value does your patch add?

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #15)
> Also, did you mean to say that the patch in bug 388880 fixes this? If so, what
> value does your patch add?

no, I meant it was touching same code, so it could have fixed the issue, it does not actually afaict

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.2

Comment 18

[Dão Gottwald [:dao]]

Unlike init, uninit doesn't seem to be called from the outside. Maybe rename it to something like "_uninitView"?
onBrowserWindowClose doesn't seem to be called at all.

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #18)
> onBrowserWindowClose doesn't seem to be called at all.

I was pretty sure it was already called in browser.js but now that I look at the original patch for toolbar binding removal, Mano added the method but not a call point. At this point I can avoid uninit completely.

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.3

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 457585 [details] [diff] [review]
patch v1.3

>+  get _toolbarViewElt() {
>+    return document.getElementById("PlacesToolbar");

nit: _viewElt would be sufficient, as the whole host object is dedicated to the "toolbar".

>+  customizeStart: function PTH_customizeStart() {
>+    if (this._toolbarViewElt && this._toolbarViewElt._placesView)
>+      this._toolbarViewElt._placesView.uninit();

nit: use a variable instead of accessing the _toolbarViewElt getter three times.

Comment 22

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.4

bah, I'm distracted lately :(

Comment 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 457641 [details] [diff] [review]
patch v1.4

my repository corrupted on a qrefresh! =:\

Comment 24

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.5

Comment 25

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/ce439bd24549