Steps to reproduce: 1. Start Firefox newly created profile 2. Open the Bookmarks widget and close 3. Narrow browser width so that a Bookmarks widget moves to a toolbar chevron 4. Wider browser again so that the Bookmarks widget appears in toolbar 5. Open the Bookmarks widget and right click on a bookmark Actual Results: All places context menuitems are grayed out Expected Results: Some of places context menuitems should be available and enabled

So the cause of the issue reported in comment 0 is that the controller gets lost as the button moves into the overflow popup. That's not bad then (we don't have a need for a places view in there, nevermind a controller for the context menu). However, when it moves back (if we'd added one, it'd now get lost again), we do need one, and it's gone, so the places command controller is never queried for whether the commands should be enabled, which means they all end up disabled. The same issue happens when you use the new context menu items to move the item to the panel (without entering customize mode). I've fixed that case as well, and switched the BookmarkingUI object to the new listeners instead of the old ones. This leads to slightly more code, but also slightly more performant code as we don't run as much code whenever some other widget changes place. Finally, I noticed that if you stay on the same page and move the bookmarks button to the panel and add a bookmark while it's there and then move the widget back, we don't update the star. That's also fixed by this patch. Marco, what do you think? :-)

Comment on attachment 8376280 [details] [diff] [review] Australis - places controller gets lost if bookmarks button is moved to overflow or moved outside of customize mode, Review of attachment 8376280 [details] [diff] [review]: ----------------------------------------------------------------- overall I think the approach is fine, though I have some questions ::: browser/base/content/browser-customization.js @@ -40,5 @@ > > CombinedStopReload.uninit(); > CombinedBackForward.uninit(); > PlacesToolbarHelper.customizeStart(); > - BookmarkingUI.customizeStart(); out of curiosity, when you say new/old listeners, means all of these consumers should ideally be migrated to the new form, or just that the new form is preferred but both are expected? ::: browser/base/content/browser-places.js @@ +1168,3 @@ > let usedToUpdateStarState = this._shouldUpdateStarState(); > this._updateCustomizationState(); > if (usedToUpdateStarState != this._shouldUpdateStarState()) { I guess it may be more correct as if (!usedToUpdateStarStare && this._shouldUpdate... is there a reason we want to update the star state if we don't care anymore about it (the opposite condition)? @@ +1168,4 @@ > let usedToUpdateStarState = this._shouldUpdateStarState(); > this._updateCustomizationState(); > if (usedToUpdateStarState != this._shouldUpdateStarState()) { > + this.updateStarState(true); instead of adding a "force" parameter I'd prefer if you'd make updateStarState always being executed, add an onLocationChange handler, and replace this call http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#3609 with the new handler. Then move the (this._uri && gBrowser.currentURI.equals(this._uri)) check inside the new onLocationChange. That way updateStarState will only be invoked when needed, that makes more sense. @@ +1517,5 @@ > > // If the button hasn't been in the overflow panel before, we may ignore > // this event. > if (!this._starButtonLabel) > return; I know it's not due to this patch, though, may this really happen? how do we get an onWidgetUnderflow without an onWidgetOverflow? I'm asking mostly to understand if the uninitView you added may need to be moved above this. ::: browser/components/places/content/browserPlacesViews.js @@ +724,5 @@ > + // Removing the controller will fail if it is already no longer there > + // (for instance, by someone else moving it) > + try { > + this._viewElt.controllers.removeController(this._controller); > + } catch (ex) {} this explanation looks a little bit too generic, especially "someone else". Whoever moves the view causing the binding/controller to be detached should call uninit on the old view and create a new view, we should not try to mask errors caused by consumers not doing that Though, it's possible you have found a case where the uninit/newview procedure doesn't work, or the controller is destroyed before we call uninit, if so I'd like that to be explained and eventually reported in the comment, maybe even with a todo pointing to a bug (if something may be done about it...) Do you have a stack of how we arrive here and cause the exception?

(In reply to Marco Bonardo [:mak] from comment #2) > Comment on attachment 8376280 [details] [diff] [review] > Australis - places controller gets lost if bookmarks button is moved to > overflow or moved outside of customize mode, > > Review of attachment 8376280 [details] [diff] [review]: > ----------------------------------------------------------------- > > overall I think the approach is fine, though I have some questions > > ::: browser/base/content/browser-customization.js > @@ -40,5 @@ > > > > CombinedStopReload.uninit(); > > CombinedBackForward.uninit(); > > PlacesToolbarHelper.customizeStart(); > > - BookmarkingUI.customizeStart(); > > out of curiosity, when you say new/old listeners, means all of these > consumers should ideally be migrated to the new form, or just that the new > form is preferred but both are expected? New form is preferred, but I don't expect add-ons to be migrating en masse as long as we keep the old form. I wrote a patch to fix the old form to be a bit better in bug 972267. > ::: browser/base/content/browser-places.js > @@ +1168,3 @@ > > let usedToUpdateStarState = this._shouldUpdateStarState(); > > this._updateCustomizationState(); > > if (usedToUpdateStarState != this._shouldUpdateStarState()) { > > I guess it may be more correct as > if (!usedToUpdateStarStare && this._shouldUpdate... > > is there a reason we want to update the star state if we don't care anymore > about it (the opposite condition)? This code was already there, and I believe I had a reason in the bug that added this (bug 964887) but I don't recall offhand. Considering that we need to uplift this I'd prefer not to change it unless we were sure it wouldn't break anything, in particular in terms of keeping track of bookmarked pages (which is what the call would enable). > > @@ +1168,4 @@ > > let usedToUpdateStarState = this._shouldUpdateStarState(); > > this._updateCustomizationState(); > > if (usedToUpdateStarState != this._shouldUpdateStarState()) { > > + this.updateStarState(true); > > instead of adding a "force" parameter I'd prefer if you'd make > updateStarState always being executed, add an onLocationChange handler, and > replace this call > http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser. > js#3609 with the new handler. > Then move the (this._uri && gBrowser.currentURI.equals(this._uri)) check > inside the new onLocationChange. > That way updateStarState will only be invoked when needed, that makes more > sense. I'll give this a shot. > @@ +1517,5 @@ > > > > // If the button hasn't been in the overflow panel before, we may ignore > > // this event. > > if (!this._starButtonLabel) > > return; > > I know it's not due to this patch, though, may this really happen? how do we > get an onWidgetUnderflow without an onWidgetOverflow? > I'm asking mostly to understand if the uninitView you added may need to be > moved above this. Not sure. Mike de Boer added this. I would have thought it could happen when new windows are created or such, but it looks like it's only ever sent for items that are in the overflow panel already. Mike? > ::: browser/components/places/content/browserPlacesViews.js > @@ +724,5 @@ > > + // Removing the controller will fail if it is already no longer there > > + // (for instance, by someone else moving it) > > + try { > > + this._viewElt.controllers.removeController(this._controller); > > + } catch (ex) {} > > this explanation looks a little bit too generic, especially "someone else". > Whoever moves the view causing the binding/controller to be detached should > call uninit on the old view and create a new view, we should not try to mask > errors caused by consumers not doing that > Though, it's possible you have found a case where the uninit/newview > procedure doesn't work, or the controller is destroyed before we call > uninit, if so I'd like that to be explained and eventually reported in the > comment, maybe even with a todo pointing to a bug (if something may be done > about it...) Whenever the node is moved elsewhere for any reason, the controllers object will be reset by XUL/XBL/native code. I don't think it makes sense to try to catch any and every reason that this may have already happened by the time we get called. At least one of these cases is the one where the node is overflowed. At that point, there is no point re-creating the view. We could still do that, but it seems like work for no reason. getControllerById apparently throws, too, if the controller has been removed, so it seems like there's no non-exception-generating manner of checking if this has happened. :-( A possible alternative would be for the controllerForCommand checks that are used in onpopupshowing to check getControllerId(controller) to see if the controller was removed, and if so, to re-add it dynamically. I don't know if that will be sufficient to re-generate the view if necessary, however...

(In reply to :Gijs Kruitbosch from comment #3) > > I guess it may be more correct as > > if (!usedToUpdateStarStare && this._shouldUpdate... > > > > is there a reason we want to update the star state if we don't care anymore > > about it (the opposite condition)? > > This code was already there, and I believe I had a reason in the bug that > added this (bug 964887) but I don't recall offhand. Considering that we need > to uplift this I'd prefer not to change it unless we were sure it wouldn't > break anything, in particular in terms of keeping track of bookmarked pages > (which is what the call would enable). I can understand, though this is not just a graphical change, it causes IO, so we should try to avoid it as far as possible. I'd also be fine to have a nightly only patch for this and leave it unchanged on Aurora. > Whenever the node is moved elsewhere for any reason, the controllers object > will be reset by XUL/XBL/native code. I don't think it makes sense to try to > catch any and every reason that this may have already happened by the time > we get called. At least one of these cases is the one where the node is > overflowed. At that point, there is no point re-creating the view. We could > still do that, but it seems like work for no reason. So, basically the controller is destroyed before we get the notification (the operation already happened at this point). So, please just update the comment to make it clear we can't ensure the controller is still there cause we may be notified after the fact.

Alright, here's an updated patch with an automated test.

Comment on attachment 8377520 [details] [diff] [review] Australis - places controller gets lost if bookmarks button is moved to overflow or moved outside of customize mode, Review of attachment 8377520 [details] [diff] [review]: ----------------------------------------------------------------- there a couple mistakes, though I'm sure the next one will be fine ::: browser/base/content/browser-places.js @@ +1172,5 @@ > + } else if (usedToUpdateStarState && !this._shouldUpdateStarState()) { > + this._updateStar(); > + } > + // If we're moved outside of customize mode, we need to uninit > + // our view so it gets reconstructed: nit: s/:/./ @@ +1184,5 @@ > + if (aWidgetId != "bookmarks-menu-button") { > + return; > + } > + // If we're moved outside of customize mode, we need to uninit > + // our view so it gets reconstructed: nit: s/:/./ @@ +1223,5 @@ > delete this._pendingStmt; > } > }, > > + onLocationChange: function BUI_onLocationChange() { nice but you haven't changed the call in browser.js to point to this new helper... @@ +1229,3 @@ > return; > } > + this.updateStarState(); there's something wrong with the if condition, why did you change it? I'd say to either revert it or change it to if (!this._uri || ! gBrowser.currentURI.equals(this._uri)) this.update... @@ +1520,5 @@ > if (aNode.id != "bookmarks-menu-button" || win != window) > return; > > + // The view gets broken by being removed and reinserted, so uninit > + // here so popupshowing will generate a new one: nit: "so ... so", I'd remove the first one ::: browser/components/places/tests/browser/browser_toolbarbutton_menu_context.js @@ +5,5 @@ > +let newBookmarkItem = document.getElementById("placesContext_new:bookmark"); > + > +let ChromeUtils = {}; > +let scriptLoader = Cc["@mozilla.org/moz/jssubscript-loader;1"].getService(Ci.mozIJSSubScriptLoader); > +scriptLoader.loadSubScript("chrome://mochikit/content/tests/SimpleTest/ChromeUtils.js", ChromeUtils); I can't see any use of this, I only see uses of EventUtils... @@ +16,5 @@ > + CustomizableUI.addWidgetToArea("bookmarks-menu-button", CustomizableUI.AREA_NAVBAR, pos); > + yield checkPopupContextMenu(); > +}); > + > +function checkPopupContextMenu() { use function* so it's clear it's a generator @@ +19,5 @@ > + > +function checkPopupContextMenu() { > + let dropmarker = document.getAnonymousElementByAttribute(bookmarksMenuButton, "anonid", "dropmarker"); > + let popupShownPromise = onPopup(BMB_menuPopup, "shown"); > + EventUtils.synthesizeMouse(dropmarker, 2, 2, {}); you may use synthesizeMouseAtCenter @@ +23,5 @@ > + EventUtils.synthesizeMouse(dropmarker, 2, 2, {}); > + dropmarker.click(); > + yield popupShownPromise; > + let contextMenuShownPromise = onPopup(contextMenu, "shown"); > + EventUtils.synthesizeMouse(BMB_showAllBookmarks, 2, 2, {type: "contextmenu", button: 2 }); ditto @@ +30,5 @@ > + let contextMenuHiddenPromise = onPopup(contextMenu, "hidden"); > + contextMenu.hidePopup(); > + yield contextMenuHiddenPromise; > + let popupHiddenPromise = onPopup(BMB_menuPopup, "hidden"); > + EventUtils.synthesizeMouse(dropmarker, 2, 2, {}); ditto @@ +31,5 @@ > + contextMenu.hidePopup(); > + yield contextMenuHiddenPromise; > + let popupHiddenPromise = onPopup(BMB_menuPopup, "hidden"); > + EventUtils.synthesizeMouse(dropmarker, 2, 2, {}); > + yield popupHiddenPromise; please add some info("doing this and waiting that") in the middle, so that if the test timeouts it's easier to figure at which point it did. It will also make the test self-documenting.

One of the synthesizeMouse couldn't be converted, so I've commented there instead.

Comment on attachment 8377582 [details] [diff] [review] Australis - places controller gets lost if bookmarks button is moved to overflow or moved outside of customize mode, Review of attachment 8377582 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +3605,5 @@ > if (gURLBar) { > URLBarSetURI(aLocationURI); > > // Update starring UI > + BookmarkingUI.onLocationChange(); nit: the comment above may probably die, it doesn't add anything useful to BookmarkingUI self documentation ::: browser/components/places/content/browserPlacesViews.js @@ +731,1 @@ > this._controller = null; please put the null assignment into a finally ::: browser/components/places/tests/browser/browser_toolbarbutton_menu_context.js @@ +36,5 @@ > + info("Waiting for bookmarks menu to be hidden."); > + yield popupHiddenPromise; > +} > + > +function onPopup(popup, evt) { nit: I'd rename to onPopupEvent

w/ nits remote: https://hg.mozilla.org/integration/fx-team/rev/4e44d3d8917a

Comment on attachment 8377582 [details] [diff] [review] Australis - places controller gets lost if bookmarks button is moved to overflow or moved outside of customize mode, [Approval Request Comment] Bug caused by (feature/regressing bug #): Australis User impact if declined: bookmarks context menu and other bookmark button functions stop functioning when button is overflowed Testing completed (on m-c, etc.): on m-c, has automated test Risk to taking this patch (and alternatives if risky): relatively low String or IDL/UUID changes made by this patch: none

remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/f879ee1dea6b

This issue is still present in latest Aurora (Build ID:20140314004001) and Nightly (Build ID: 20140314030202) on Windows 7 64 bit: after performing steps from comment 0, the context menu items are grayed out (please see the attachment). If I restart Firefox - the context menu items are clickable, but after executing STR, they are grayed out again. Should we reopen this bug or log another one?

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #13) > Created attachment 8391186 [details] > Image showing this issue > > This issue is still present in latest Aurora (Build ID:20140314004001) and > Nightly (Build ID: 20140314030202) on Windows 7 64 bit: after performing > steps from comment 0, the context menu items are grayed out (please see the > attachment). If I restart Firefox - the context menu items are clickable, > but after executing STR, they are grayed out again. Should we reopen this > bug or log another one? Please file a new bug, mark as blocking both this and the 'australis-cust' bug, and CC me. Thank you.

Please ensure that is not a dupe of bug 625778

(In reply to Marco Bonardo [:mak] from comment #15) > Please ensure that is not a dupe of bug 625778 I don't consider the issue described in comment 13 the same as bug 625778, that one is about enter/exit Customize mode. Please correct me if I'm wrong.