488846 - Places code assumes non-focusable elements can be focused

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Neil Deakin]

There are a number of points in the places code which tries to call focus() on non-focusable elements such as panels and menus.

One example is when opening a context menu on a bookmark menu where buildContextMenu in menu.xml tries to focus a menupopup.

Currently, focus() on a non-focusable xul element makes it sort of focusable. After bug 178324 is done, a non-focusable element will not be focusable at all.

These will need to be fixed to do whatever they are trying to do.

Comment 1

[Marco Bonardo [:mak]]

(In reply to comment #0)
> These will need to be fixed to do whatever they are trying to do.

That is probably activating the controller.

Comment 2

[Neil Deakin]

After a bit more debugging, the issue is that the context menu for bookmarks is expecting to focus the bookmarks menupopup (in menu.xml:buildContextMenu for example). The code in goUpdatePlacesCommands() is trying to get the controller for a command using getControllerForCommand, yet this method always uses the focused element, and since nothing is focused, it can't get one.

I'm going to attach a patch which changes this to use the popupNode if a controller cannot be retrieved the normal way.

Comment 3

[Neil Deakin]

Attachment: as described

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 375059 [details] [diff] [review]
as described

>diff --git a/browser/components/places/content/controller.js b/browser/components/places/content/controller.js
>--- a/browser/components/places/content/controller.js
>+++ b/browser/components/places/content/controller.js
>@@ -1544,23 +1544,18 @@
>     var flavourSet = new FlavourSet();
>     var acceptedDropFlavours = this.GENERIC_VIEW_DROP_TYPES;
>     acceptedDropFlavours.forEach(flavourSet.appendFlavour, flavourSet);
>     return this.flavourSet = flavourSet;
>   }
> };
> 
> function goUpdatePlacesCommands() {
>-  var placesController;
>-  try {
>-    // Or any other command...
>-    placesController = top.document.commandDispatcher
>-                          .getControllerForCommand("placesCmd_open");
>-  }
>-  catch(ex) { return; }
>+  // Get the controller for one of the places commands

end comments with . please

>+  var placesController = doGetPlacesControllerForCommand("placesCmd_open");
> 
>   function updatePlacesCommand(aCommand) {
>     var enabled = false;
>     if (placesController)
>       enabled = placesController.isCommandEnabled(aCommand);

previous code was early returning in case placesController was not set, i think it would make more sense to add an "if(!placesController) return;" after the call to doGetPlacesController and remove the useless if(placesController) check from the helper. Should save some work.

>@@ -1572,8 +1567,46 @@
>   updatePlacesCommand("placesCmd_new:livemark");
>   updatePlacesCommand("placesCmd_new:separator");
>   updatePlacesCommand("placesCmd_show:info");
>   updatePlacesCommand("placesCmd_moveBookmarks");
>   updatePlacesCommand("placesCmd_reload");
>   updatePlacesCommand("placesCmd_reloadMicrosummary");
>   updatePlacesCommand("placesCmd_sortBy:name");
> }
>+
>+function doGetPlacesControllerForCommand(aCommand)
>+{
>+  var placesController;
>+  try {
>+    placesController = top.document.commandDispatcher
>+                          .getControllerForCommand(aCommand);
>+    if (!placesController) {
>+      // if building commands for a context menu, look for an element in the
>+      // current popup

uppercase first and end with . please

>+      var element = document.popupNode;
>+      if (element)
>+        element = element.parentNode;

is this if needed? why not directly going into the while loop? if it's detectable in the while loop a continue would probably be more readable. If this is needed it needs a comment explaining why.

>+      while (element) {
>+        if ((element.localName == "menupopup" || element.localName == "hbox") &&
>+            ("_contextMenuShown" in element) && element._contextMenuShown) {

please define a temp var isContextMenuShown = ("_contextMenuShown" in element) && element._contextMenuShown; for readability
also add a comment about which case "hbox" is catching.


>+function goDoPlacesCommand(aCommand)
>+{
>+  try {
>+    var controller = doGetPlacesControllerForCommand(aCommand);
>+    if (controller && controller.isCommandEnabled(aCommand))
>+      controller.doCommand(aCommand);
>+  }
>+  catch (e) { }

which case is catching this try/catch? doGetPlacesControllerForCommand cannot throw, is it usual to throw for isCommandEnabled or doCommand? is there not a risk to hide real bugs/errors this way?

>diff --git a/browser/components/places/content/placesOverlay.xul b/browser/components/places/content/placesOverlay.xul
>--- a/browser/components/places/content/placesOverlay.xul
>+++ b/browser/components/places/content/placesOverlay.xul
>@@ -56,48 +56,48 @@
>           src="chrome://browser/content/places/controller.js"/>
>   <script type="application/x-javascript"
>           src="chrome://browser/content/places/treeView.js"/>
>   <script type="application/x-javascript"
>           src="chrome://global/content/nsDragAndDrop.js"/>
> 
>   <commandset id="placesCommands"
>               commandupdater="true"
>-              events="focus,sort"
>+              events="focus,sort,places"
>               oncommandupdate="goUpdatePlacesCommands();">
>     <command id="placesCmd_open"
>-             oncommand="goDoCommand('placesCmd_open');"/>
>+             oncommand="goDoPlacesCommand('placesCmd_open');"/>

The call to goDoCommand in movebookmarks.js should not be a problem since it's focusing a tree, right? (We should probably unify that dialog with bookmarksProperties in future, though).

I'll do some testing before r+, but looks correct so far.

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 375059 [details] [diff] [review]
as described

also, please unbitrot patch.

Thank you for taking care of our hacky focus behavior.

Comment 6

[Marco Bonardo [:mak]]

i notice a problem with common operations (cut/copy/paste/delete). Those are always disabled both in toolbar and in menu subfolders of any type.

Comment 7

[Neil Deakin]

Attachment: updated patch, use special places commands for cut/copy/paste/delete

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 379899 [details] [diff] [review]
updated patch, use special places commands for cut/copy/paste/delete

>diff --git a/browser/components/places/content/controller.js b/browser/components/places/content/controller.js
>--- a/browser/components/places/content/controller.js
>+++ b/browser/components/places/content/controller.js
>@@ -196,16 +200,20 @@
>     switch (aCommand) {
>     case "cmd_undo":
>     case "cmd_redo":
>     case "cmd_cut":
>     case "cmd_copy":
>     case "cmd_paste":
>     case "cmd_delete":
>     case "cmd_selectAll":
>+    case "placesCmd_cut":
>+    case "placesCmd_copy":
>+    case "placesCmd_paste":
>+    case "placesCmd_delete":
>       return true;
>     }
> 
>     // All other Places Commands are prefixed with "placesCmd_" ... this 
>     // filters out other commands that we do _not_ support (see 329587).
>     const CMD_PREFIX = "placesCmd_";
>     return (aCommand.substr(0, CMD_PREFIX.length) == CMD_PREFIX);
>   },

Those should not be added to the switch above since the below code handles any placesCmd_ prefixed command.

Initially i did not like much the fact we still have to support cmd_x if we have our own placesCmd_x, but i see those are used in other places (Library, tests) and could be used by extensions, so having both is fine for now.

>+function goDoPlacesCommand(aCommand)
>+{
>+  try {
>+    var controller = doGetPlacesControllerForCommand(aCommand);
>+    if (controller && controller.isCommandEnabled(aCommand))
>+      controller.doCommand(aCommand);
>+  }
>+  catch (e) { }
>+}

I still have a concern about this catch, i think we should reportError any exception or don't catch unless there's a clear reason to do so.

>diff --git a/browser/components/places/content/placesOverlay.xul b/browser/components/places/content/placesOverlay.xul
>--- a/browser/components/places/content/placesOverlay.xul
>+++ b/browser/components/places/content/placesOverlay.xul

>     <menuitem id="placesContext_delete_history"
>               command="cmd_delete"
>               label="&cmd.delete.label;"
>               accesskey="&cmd.delete.accesskey;"

this delete could happen in menu/toolbar and should be handled like the other ones.

This patch still does not apply clean to m-c.

Comment 9

[Neil Deakin]

> This patch still does not apply clean to m-c.

It applies on top of the patch in bug 178324.

Comment 10

[Marco Bonardo [:mak]]

i have apply failures only in Places files, none of the patches in bug 178324 touch the same files and i have pulled the latest m-c. I just point that because i'm pretty sure we added a context menu voice lately on m-c.

Comment 11

[Neil Deakin]

Attachment: fix issues

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 380826 [details] [diff] [review]
fix issues

>diff --git a/browser/components/places/content/controller.js b/browser/components/places/content/controller.js
>--- a/browser/components/places/content/controller.js
>+++ b/browser/components/places/content/controller.js
>@@ -130,25 +130,29 @@
>       var nodes = this._view.getSelectionNodes();
>       // If selection includes history nodes there's no reason to allow cut.
>       for (var i = 0; i < nodes.length; i++) {
>         if (nodes[i].itemId == -1)
>           return false;
>       }
>       // Otherwise fallback to cmd_delete check.
>     case "cmd_delete":
>+    case "placesCmd_cut":
>+    case "placesCmd_delete":

placesCmd_cut should be grouped with cmd_cut (see above in the switch), or it will lose history nodes case.
This problem has probably been caused by the unbitrot.

r=me with that fixed

As far as i can test this works fine, we should still fix bug 486850 to provide better automated tests for context menus voices, i'll add a dependancy.

Thank you very much for this fix, it clarify lot of things that were lying in the dust.

Comment 13

[Marco Bonardo [:mak]]

pushed by Neil:
http://hg.mozilla.org/mozilla-central/rev/771509552a2e

Comment 14

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv