Closed Bug 562998 Opened 14 years ago Closed 13 years ago

Selecting commands from a bookmarks context menu while the window isn't active does nothing

Categories

(Firefox :: Bookmarks & History, defect)

x86
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 4.0b11
Tracking Status
blocking2.0 --- final+

People

(Reporter: enndeakin, Assigned: asaf)

References

(Depends on 1 open bug)

Details

(Keywords: regression, Whiteboard: [softblocker])

Attachments

(1 file, 8 obsolete files)

Steps:

1. Put a Firefox window into the background
2. Context-click on a bookmark. The context menu should appear but the window won't come to the front.
3. Select a command, such as Open in New Tab but nothing happens.

Console error:

JavaScript error: , line 0: uncaught exception: [Exception... "'[JavaScript Error: "this._getCurrentActiveWin() is null" {file: "file:///builds/moz2/working/output/dist/MinefieldDebug.app/Contents/MacOS/modules/PlacesUIUtils.jsm" line: 925}]' when calling method: [nsIController::doCommand]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://browser/content/places/controller.js :: goDoPlacesCommand :: line 1659"  data: yes]

This bug only applies to Mac I assume. You may need an external mouse to open the menu in the background.
yeah, we should probably fallback to the top browser window if there is active window
Assignee: nobody → mak77
i there is NO active window
Blocks: 560198
Why do we need to rely on getting the active window? At least for the context menu, seems like we should be using the popupNode's window...
i was thinking to that just after having posted the comment, we have to change some method to pass the current window in, it should be feasible. This code is there as a temp stub after the UIUtils change
Attached patch patch v1.0 (obsolete) — Splinter Review
This should solve it, i'm carrying on the window from the controller, or if I have an event I use the new getGlobalObjectFor() magic.

Since I had to bring the window for a bunch of stupid showAddMonkeyUI methods, I've decided to finish work i started time ago with bookmarksProperties, and remove all of that crap. BookmarksProperties input is much more readable than the crazy stuff we had. So I ended up with a single ShowBookmarkDialog util.
Also found 2 unused controller methods (sigh).

Will ask review after some manual testing.
Attached patch patch v1.1 (obsolete) — Splinter Review
Attachment #443788 - Attachment is obsolete: true
Attached patch patch v1.2 (obsolete) — Splinter Review
Added back PUIU helpers for add-ons compatibility reasons (Weave is also using a couple of them). We will evaluate a shim PlacesUIUtils in future for compatibility.

Made bookmarks dialogs "dependent", it is amazing on Windows, unfortunatly i have no idea if this is going to work on Mac since I was told there is some longstanding bug there. So on Mac I could be forced to fallback to modal.
Attachment #443859 - Attachment is obsolete: true
to clarify why i'm doing this compare these two:

- performed = PlacesUIUtils.showAddBookmarkUI(null, null, null, ip);

+    PlacesUIUtils.showBookmarkDialog({ action: "add"
+                                     , type: aType
+                                     , defaultInsertionPoint: ip
+                                     , hiddenRows: [ "folderPicker" ]
+                                     , placesView: this._view
+                                     }, window, true);

can you guess what result you will get from the first call?
and we also have nice cases like:
showMinimalAddBookmarkUI(uri, aTitle, null, null, true, true);
There is a nice drawback making all dialogs dependent, practically I open a dialog, then I can open another one since the first one is not modal. Now i make a change in one of the 2 dialogs and confirm. Then i cancel the second dialog, and puff, my changes in the first dialog are gone. This is by the fact the transactions manager is one.
I think i could use the wm to check if there is already a properties dialog open and don't allow opening a new one.
Possibilities:
1. If an option to open a new dialog is choosen, just ignore it. Easy to do.
2. If a properties dialog is already open, disable commands that could open another one. Nice but will make commands updating slower (will alwasy have to ask wm at each update).
3. reinit the open panel to the new required element, canceling the previous one. Could still lose changes, but the users will quickly get used to it.
4. Just ignore the problem and expect users getting used to not change bookmarks while a properties dialog is open.

Actually there is also another problematic path, a Cancel rollbacks all changes from when the dialog has been opened, thus also if i do changes to the toolbar or the menu with the dialog open and i click Cancel, those are rollbacked.
Instead of using the global Places transaction manager I could create a new instance per dialog, If the user clicks OK, then I should register the same transactions to the global Places tm instance. This should work.
Attached patch patch v1.3 (obsolete) — Splinter Review
let's say this is a proof of concept.
Actually it is using dependent on Win and Lin, modal on Mac, dependent does not work at all on Mac, Neil suggested to convert the dialog window to a noautohide panel. But this could and should probably be a followup since it's barely related.

I added a transaction manager to each dialog, and allowed editBookmarkOverlay to receive an optional tm and use it. This way I can avoid an undo in this window to pollute the global places transaction manager.

The only remaining problem is that once i close the dialog i cannot undo the change through the global transaction manager, since I used a new instance. IF there would be a way to transplant/migrate transactions from one manager to another one would be AWESOME (so far i've added a listener in the dialog to collect transactions). The only workaround I could find so far is to undo in the window manager, add all transactions to the global manager and do them again, that would suck for performances and flickering though.

Asking feedback to Mano, he could have some clever suggestion!
Attachment #443883 - Attachment is obsolete: true
Attachment #443958 - Flags: feedback?(mano)
actually what i'm thinking is that this could be good as it is, without the transactions listener and with a followup to convert the dialog to a noautohide panel.
I say so because:
- this is a panel that acts on a single bookmark
- the panel has a Cancel button
For these reasons i think the possibilities of doing something bad are really limited.  If I wrongly create a bookmark (I could have pressed Cancel) I can just delete it, but I don't have dataloss. If i wrongly edit a bookmark, again i could have cenceled (while i confirmed) and i can just edit it again. Sure it's not perfect but adding a transplant functionality to the transactions manager sounds like a lot of work for a really small benefit.
Let's talk on IRC.
(In reply to comment #14)
> Let's talk on IRC.

Looks like it is hard these days, can you port your concerns here or send me a mail? So we can move this on.
Status: NEW → ASSIGNED
Blocks: 539283
this should block final at least, it's a focus problem and also causing some orange.
blocking2.0: --- → ?
Clarifying the blocking request, if user tries to open a bookmarks context menu on a background window everything will break there.
blocking2.0: ? → final+
Comment on attachment 443958 [details] [diff] [review]
patch v1.3

clearing request, I have to revert UI changes because I don't want them at this stage, I just want the code clarification and the bug fix.
Attachment #443958 - Flags: feedback?(mano)
As discussed few days ago, this bug is stolen.
Assignee: mak77 → mano
I think there should not be big changes to do apart removing my transaction manager hacks and setting back the dialog  mode to whatever it was before. If you can, please preserve the unified showBookmarkDialog API for all internal calls that is much more readable (old strange named methods must stay there for addons compatibility though). That method should already have code to detect minimalUI to restore the old behavior.
Mano, any news?
This needs an active owner - Mano, is that still you?
Yes.
Whiteboard: [needs update]
Whiteboard: [needs update] → [eta: 15/12]
Patch coming in a bit.  However, to fix it nicely we'll need to move utilityOverlay.js to a module - I've a patch for that, but it's trunk material.
yes please, we should get a smaller fix for this, changing utilityOverlay to a module does not look like a feasible goal so late even if it's extremely valuable for a dot release.
Depends on: 619398
Actually, this depends on bug 	619398 being fixed now. Waiting for Dietrich's review there.
Whiteboard: [eta: 15/12] → [eta: 17/12]
Blocks: 603646
Attached patch patch v1.3.5 (obsolete) — Splinter Review
I'm not so happy about this patch as a whole.  But first, let me summarize what it does and what it doesn't:
1. It makes some of the method in placesutils take a places-view parameter.  This is a very desired change regardless of this bug, IMO, as it would less us move more generic work into placesutils.
2. showBookmarkDialog is public now, as Marco did in his patch, and the use-case methods are deprecated.
3. When using a deprecated method, we warn.
4. When a parameter is missing, we warn. The methods will still work though.
5. showBookmarkDialog cannot take a view parameter unfortunately, because of the usage in nsSidebar. There are three possible right fixes:
  * Remove support for addPanel - we should really consider that at some point.  This SeaMonkey API isn't used much, and if someone really needs it, he could provide an extension.
  * Fake a view in nsSidebar
  * When there is no view, don't modal the dialog. This actually makes sense...

For now, the methods just takes a window argument.
Attachment #443958 - Attachment is obsolete: true
Attachment #498748 - Flags: review?(mak77)
Oh, and I forgot to note about the utilityOverlay.js mess.  First, I don't know who reviewed that, it might have just been me, but we should have never force all windows that use placesuiutils to include utilityOverlay.js.  This lead to the weird situation where browser.xul gets utilityOverlay.js from... placesOverlay.xul.

The patch here won't fix that.  I filed and patched bug 619398 for making utilityOverlay a module on trunk. Once it's done, placesutils won't require its clients to use utilityOverlay.
Comment on attachment 498748 [details] [diff] [review]
patch v1.3.5

>diff -r 3b4f3b897999 browser/base/content/browser-places.js

>@@ -708,17 +708,17 @@
>    * Handler for click event for an item in the bookmarks toolbar or menu.
>    * Menus and submenus from the folder buttons bubble up to this handler.
>    * Left-click is handled in the onCommand function.
>    * When items are middle-clicked (or clicked with modifier), open in tabs.
>    * If the click came through a menu, close the menu.
>    * @param aEvent
>    *        DOMEvent for the click
>    */
>-  onClick: function BEH_onClick(aEvent) {
>+  onClick: function BEH_onClick(aEvent, aView) {

update javadoc please, specify if optional and what happens if missing.

>   /**
>    * Handler for command event for an item in the bookmarks toolbar.
>    * Menus and submenus from the folder buttons bubble up to this handler.
>    * Opens the item.
>    * @param aEvent 
>    *        DOMEvent for the command
>    */
>-  onCommand: function BEH_onCommand(aEvent) {
>+  onCommand: function BEH_onCommand(aEvent, aView) {

ditto

>diff -r 3b4f3b897999 browser/components/places/content/controller.js

>   newItem: function PC_newItem(aType) {
>-    var ip = this._view.insertionPoint;
>+    let ip = this._view.insertionPoint;
>     if (!ip)
>       throw Cr.NS_ERROR_NOT_AVAILABLE;
> 

>+    var performed =
>+      PlacesUIUtils.showBookmarkDialog({ action: "add"

nit: since you're changing to let, this should be let, not var.

>+                                       , type: aType
>+                                       , defaultInsertionPoint: ip
>+                                       , hiddenRows: [ "folderPicker" ]
>+                                       }, window);
>     if (performed) {
>       // select the new item
>       var insertedNodeId = PlacesUtils.bookmarks
>                                       .getIdForItemAt(ip.itemId, ip.index);

nit: same here

>   newSeparator: function PC_newSeparator() {
>     var ip = this._view.insertionPoint;
>     if (!ip)

can we forward this to newItem as a special case as well and deprecate it?
do you think it's not worth it?

>diff -r 3b4f3b897999 browser/components/places/src/PlacesUIUtils.jsm

>+  showBookmarkDialog:
>+  function PUIU_showBookmarkDialog(aInfo, aParentWindow) {
>+    if (!aParentWindow)
>+      aParentWindow = this._getWindow(null);

nit: based on global code style, let's brace single lines in new code please.

>+    // Preserve size attributes differently based on the fact the dialog has
>+    // a folder picker or not.
>+    let minimalUI = "hiddenRows" in aInfo &&
>+                    aInfo.hiddenRows.indexOf("folderPicker") != -1;
>+    let dialogURL = minimalUI ?
>                     "chrome://browser/content/places/bookmarkProperties2.xul" :
>                     "chrome://browser/content/places/bookmarkProperties.xul";
> 
>-    var features;
>-    if (aMinimalUI)
>+    let features;
>+    if (minimalUI)
>       features = "centerscreen,chrome,modal,resizable=yes";
>     else
>       features = "centerscreen,chrome,modal,resizable=no";

nit: ditto for bracing
I'd collapse to one-line:
let features = "centerscreen,chrome,modal,resizable=" + (minimalUI ? "yes" : "no");

>-  _openTabset: function PUIU__openTabset(aItemsToOpen, aEvent) {
>+  _openTabset: function PUIU__openTabset(aItemsToOpen, aEvent, aWindow) {

>-    var browserWindow = this._getTopBrowserWin();
>-    var where = browserWindow ?
>-                browserWindow.whereToOpenLink(aEvent, false, true) : "window";
>+    var where = aWindow.whereToOpenLink(aEvent, false, true);

nit: let

>     if (where == "window") {
>-      let win = this._getCurrentActiveWin();
>-      win.openDialog(win.getBrowserURL(), "_blank",
>-                     "chrome,all,dialog=no", urls.join("|"));
>+      aWindow.openDialog(win.getBrowserURL(), "_blank",
>+                             "chrome,all,dialog=no", urls.join("|"));

nit: bad indentation on arguments

>+  /**
>+   * Gets the window associated with the calls to some of our public methods.

I'd like better description here, looks like this is guessing the windows from a places view, you should briefly describe the process and the fallback.

>+   * Post Fx4, aView/aParentWindow won't be an optional!

this should be a @note, also, I don't see any aParentWindow argument.

>+   */
>+  _getWindow: function PUIU__getWindow(aView) {

maybe this should be names _getWindowForView
you miss a @param and a @return in the javadoc

>+    if (!aView) {
>+      let caller = arguments.callee.caller;
>+      

trailing spaces

>+      // If a view wasn't expected, the method should have got a window.
>+      if (aView === null) {
>+        Components.utils.reportError("The api has changed. A window should be \
>+                                      passed to " + caller.name + ".  Not \
>+                                      passing a window will throw in a future \
>+                                      release.");
>+      }
>+      else {
>+        Components.utils.reportError("The api has changed. A places view \
>+                                     should be passed to " + caller.name + ".  \
>+                                     Not passing a view will throw in a future \
>+                                     release.");
>+      }

which window? this code path is a bit cumbersome. All of this code to notify extensions looks a bit exagerated,
we should just report a single generic error and suggest to check the new apis javadocs, rather than trying to guess what the user called and which argument he missed.
that's documentation scope, not code scope.

>+      

trailing spaces

>+      // Try to find a window from the stack.
>+      let externalCaller = null;
>+      let externalCallerGloablObject = null;
>+      while (!externalCaller) {
>+        // It would never be null.
>+        let callerGlobalObject = Cu.getGlobalForObject(caller);
>+        if (callerGlobalObject != this)
>+          externalCaller = callerGlobalObject;
>+
>+        caller = caller.caller;
>+      }
>+
>+      return externalCaller;

What happens if the externalCaller is not a browser window?
Could we instead just use getTopBrowserWindow and stop trying to be worried by extreme compatibility? Since we notify about new apis, making all of this sounds painful.
All of this code is here just for compatibility with old add-ons, right? we were already partially broken, using getTopBrowserWindow will allow things to still work partially fine.
The point is, let's make things just work for old code and make them work correctly for new code.

>+    if (aView instanceof Components.interfaces.nsIDOMNode)
>+      return aView.ownerDocument.defaultView;
>+
>+    return Cu.getGlobalForObject(aView);

should we check if this is a instance of a browser window and if not use getTopBrowserWindow?

>diff -r 3b4f3b897999 browser/components/sidebar/src/nsSidebar.js

> nsSidebar.prototype.addPanelInternal =
> function (aTitle, aContentURL, aCustomizeURL, aPersist)
> {
>     var WINMEDSVC = Components.classes['@mozilla.org/appshell/window-mediator;1']
>                               .getService(Components.interfaces.nsIWindowMediator);
>+
>+    // XXX: We shouldn't do this anymore. Instead, we should find the global
>+    // object for our caller and use it.
>     var win = WINMEDSVC.getMostRecentWindow( "navigator:browser" );

file a bug (maybe you already did) and put bug number in the comment please

globally looks fine, but I'd like to do a second pass once comments are addresses/answered and do some testing on the final patch before marking it, should not take a lot of time.
Attachment #498748 - Flags: review?(mak77)
Whiteboard: [eta: 17/12] → [eta: 17/12][needs new patch]
Blocks: 616919
Whiteboard: [eta: 17/12][needs new patch] → [eta: 1/4][needs new patch]
Attached patch v1.3.5.1.0000 (obsolete) — Splinter Review
Not-so-happy about this, but I see your point.

About aView being optional for onClick/onCommand: well, it's not, and they will see the warning.

About newSeparator/newItem: I don't think it's worth it. I don't think it's right. Unlike newItem, it doesn't show any UI.
Attachment #498748 - Attachment is obsolete: true
Attachment #502461 - Flags: review?
Attachment #502461 - Flags: review? → review?(mak77)
Whiteboard: [eta: 1/4][needs new patch]
Comment on attachment 502461 [details] [diff] [review]
v1.3.5.1.0000

>diff -r 4a3866321a14 browser/base/content/browser-menubar.inc
>diff -r 4a3866321a14 browser/base/content/browser-places.js
>--- a/browser/base/content/browser-places.js	Sat Jan 08 20:06:29 2011 -0800
>+++ b/browser/base/content/browser-places.js	Mon Jan 10 14:09:32 2011 +0200
>@@ -707,18 +707,20 @@
>   /**
>    * Handler for click event for an item in the bookmarks toolbar or menu.
>    * Menus and submenus from the folder buttons bubble up to this handler.
>    * Left-click is handled in the onCommand function.
>    * When items are middle-clicked (or clicked with modifier), open in tabs.
>    * If the click came through a menu, close the menu.
>    * @param aEvent
>    *        DOMEvent for the click
>+   * @param aView
>+   *        The places view with with which aEvent should be associated.

typo: "with with", probably you meant "The Places view which aEvent should be associated with."


>   /**
>    * Handler for command event for an item in the bookmarks toolbar.
>    * Menus and submenus from the folder buttons bubble up to this handler.
>    * Opens the item.
>    * @param aEvent 
>    *        DOMEvent for the command
>+   * @param aView
>+   *        The places view with with which aEvent should be associated.

same typo


>diff -r 4a3866321a14 browser/components/places/content/sidebarUtils.js

>   handleTreeKeyPress: function SU_handleTreeKeyPress(aEvent) {
>+    // XXXmano: Post Fx4, this method should take a tree parameter

file a bug with the reasoning and annotate bug number in the comment please.


diff -r 4a3866321a14 browser/base/content/browser.xul

@@ -741,18 +741,18 @@
              toolbarname="&personalbarCmd.label;" accesskey="&personalbarCmd.accesskey;"
              collapsed="true"
              customizable="true">
       <toolbaritem flex="1" id="personal-bookmarks" title="&bookmarksItem.title;"
                    removable="true">
         <hbox flex="1"
               id="PlacesToolbar"
               context="placesContext"
-              onclick="BookmarksEventHandler.onClick(event);"
-              oncommand="BookmarksEventHandler.onCommand(event);"
+              onclick="BookmarksEventHandler.onClick(event, this.parentNode._placesView);"
+              oncommand="BookmarksEventHandler.onCommand(event, this._placesView);"

important: why are these different (one to parentNode, one not), onclick should use this._placesView

indeed, this causes a warning each time i click on the toolbar
JavaScript strict warning: chrome://browser/content/browser.xul, line 1: reference to undefined property this.parentNode._placesView


>diff -r 4a3866321a14 browser/components/places/src/PlacesUIUtils.jsm

>+  showBookmarkDialog:
>+  function PUIU_showBookmarkDialog(aInfo, aParentWindow) {
...
>-    var features;
>-    if (aMinimalUI)
>+    let features;
>+    if (minimalUI)
>       features = "centerscreen,chrome,modal,resizable=yes";
>     else
>       features = "centerscreen,chrome,modal,resizable=no";

please one-line these 5 lines.
let features = "centerscreen,chrome,modal,resizable=" + (minimalUI ? "yes" :
"no");


>+  /**
>+   * Gets the window associated with the calls to some of our public methods.
>+   * Post Fx4, aView/aParentWindow won't be an optional!
>+   */
>+  _getWindow: function PUIU__getWindow(aView) {

the documentation is still unclear "calls to some of our public methods" does not mean anything... which methods? what do they do? why do they need a window?
this need to be a bit more descriptive please.
And still I don't see any aParentWindow param as stated in the comment.
Attachment #502461 - Flags: review?(mak77) → review+
Whiteboard: [softblocker]
Blocks: 612480
Blocks: 627901
Attached patch as checked in (obsolete) — Splinter Review
http://hg.mozilla.org/mozilla-central/rev/dd398fdba56f
Attachment #502461 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Looks like this checkin accidentally included parts of bug 610682, so I fixed that:

https://hg.mozilla.org/mozilla-central/rev/4dc906ad8b6d
Blocks: 629370
Blocks: 629371
Attached patch mano's wip (obsolete) — Splinter Review
I'm running this wip through tryserver, in the meanwhile here are comments. Attaching just in case the irc discussion gets lost.

diff --git a/browser/components/places/content/places.js b/browser/components/places/content/places.js
 
   handleTreeKeyPress: function SU_handleTreeKeyPress(aEvent) {
+    // XXX Bug 627901: Post Fx4, this method should take a tree parameter.
+    let node = aEvent.target.selectedNode;
+    let view = PlacesUIUtils.getViewForNode(node);
     if (aEvent.keyCode == KeyEvent.DOM_VK_RETURN)
-      PlacesUIUtils.openNodeWithEvent(aEvent.target.selectedNode, aEvent);
+      PlacesUIUtils.openNodeWithEvent(node, aEvent, view);
   },

aEvent.target.selectedNode can arguably be null in some case, and we should not
pass it to getViewForNode in such a case

diff --git a/browser/components/places/src/PlacesUIUtils.jsm b/browser/components/places/src/PlacesUIUtils.jsm

@@ -846,37 +772,75 @@ var PlacesUIUtils = {
       urls.push(item.uri);
     }
 
-    var browserWindow = this._getTopBrowserWin();
-    var where = browserWindow ?
-                browserWindow.whereToOpenLink(aEvent, false, true) : "window";
+    var where = aWindow.whereToOpenLink(aEvent, false, true);
     if (where == "window") {
-      let win = this._getCurrentActiveWin();
-      win.openDialog(win.getBrowserURL(), "_blank",
-                     "chrome,all,dialog=no", urls.join("|"));
+      aWindow.openDialog(win.getBrowserURL(), "_blank",
+                             "chrome,all,dialog=no", urls.join("|"));

bad indentation here
Attachment #506006 - Attachment is obsolete: true
the wip patch passed all tests on tryserver.
unbitrotted against Bug 588011, fixed my comments, should be ready to land.
Attachment #507460 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/5e576c6e3d75
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b11
the changes in this bug completely broke PlacesUIUtils._openTabset for the case the we try to open openTabset when there is no browser window.

> var where = aWindow.whereToOpenLink(aEvent, false, true);
whereToOpenLink function was not design to return "window" when there is no browser window!!!

also there is typo
- aWindow.openDialog(win.getBrowserURL(), "_blank",
-                          "chrome,all,dialog=no", urls.join("|"));
+ aWindow.openDialog(aWindow.getBrowserURL(), "_blank",
+                          "chrome,all,dialog=no", urls.join("|"));
(In reply to comment #39)
> the changes in this bug completely broke PlacesUIUtils._openTabset for the case
> the we try to open openTabset when there is no browser window.

file a followup please, feel free to assign to Mano, then we'll figure it out.
filed bug 629889 - open all in tabs is broken from Library when no browser window is open.
Depends on: 629889
QA verified working. [fx4-fixed-bugday]
Status: RESOLVED → VERIFIED
Blocks: FF2SM
Version: unspecified → Trunk
Blocks: 553116
No longer blocks: FF2SM
Attachment #507875 - Attachment description: patch v1.4 → patch v1.4 [Checked in: Comment 38]
You need to log in before you can comment on or make changes to this bug.