Closed Bug 575896 Opened 13 years ago Closed 13 years ago

Bookmark Toolbar context menu does not work after customize

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 4.0b2
Tracking Status
blocking2.0 --- final+

People

(Reporter: BijuMailList, Assigned: mak)

References

Details

(Keywords: relnote, Whiteboard: [4b1])

Attachments

(1 file, 5 obsolete files)

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
(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.
Assignee: nobody → mak77
Summary: Unable to create new items on "Bookmark Toolbar" → Bookmark Toolbar context menu does not work when toolbar is visible
Whiteboard: [4b1]
(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.
(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?
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.
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.
actually, it's probably a bug we have from some time, and the patch in bug 388880 could fix it.
Attached patch patch v1.0 (obsolete) — Splinter Review
simple fix for the beta, reset the view during customize.
Bug 388880 can provide better caching code on top of this if needed.
Attachment #455120 - Flags: review?(dao)
Summary: Bookmark Toolbar context menu does not work when toolbar is visible → Bookmark Toolbar context menu does not work after customize
Attached patch patch v1.1 (obsolete) — Splinter Review
toolbar elt can be removed, as well!
Attachment #455120 - Attachment is obsolete: true
Attachment #455120 - Flags: review?(dao)
Attachment #455122 - Flags: review?(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...
blocking2.0: beta1+ → ?
Component: Widget: Win32 → Bookmarks & History
Product: Core → Firefox
QA Contact: win32 → bookmarks
OS: Windows XP → All
Hardware: x86 → All
Yup, agree - blocks final, and if we can get it in before we do a second build of beta1, we should.
blocking2.0: ? → final+
Adding relnote just in case we don't get this in time for beta - hopefully we do!
Keywords: relnote
looks like relnote is pointing to the wrong bug number?
See https://bugzilla.mozilla.org/show_bug.cgi?id=575932#c5
Not fixed in Beta 1.

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

All I wanted is to rename a bookmark.
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>?
Also, did you mean to say that the patch in bug 388880 fixes this? If so, what value does your patch add?
(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
Attached patch patch v1.2 (obsolete) — Splinter Review
Attachment #455122 - Attachment is obsolete: true
Attachment #457576 - Flags: review?(dao)
Attachment #455122 - Flags: review?(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.
(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.
Attached patch patch v1.3 (obsolete) — Splinter Review
Attachment #457576 - Attachment is obsolete: true
Attachment #457585 - Flags: review?(dao)
Attachment #457576 - Flags: review?(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.
Attachment #457585 - Flags: review?(dao) → review+
Attached patch patch v1.4 (obsolete) — Splinter Review
bah, I'm distracted lately :(
Attachment #457585 - Attachment is obsolete: true
Comment on attachment 457641 [details] [diff] [review]
patch v1.4

my repository corrupted on a qrefresh! =:\
Attachment #457641 - Attachment is obsolete: true
Attached patch patch v1.5Splinter Review
http://hg.mozilla.org/mozilla-central/rev/ce439bd24549
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b2
You need to log in before you can comment on or make changes to this bug.