555547 - A command of placesContextMenu is carried out for a wrong bookmark item

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Alice0775 White]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US;
                  rv:1.9.3a4pre) Gecko/20100327 Minefield/3.7a4pre ID:20100327035846
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US;
                  rv:1.9.3a4pre) Gecko/20100327 Minefield/3.7a4pre ID:20100327035846

Not the item which I clicked the right button of, a command is carried out for a focus element in the sidebar

When there is a focus to the bookmark item of the sidebar,
A command of PlacesContextMenu is carried out for the focused item of the sidebar not the item which I clicked the right button of on the bookmark item on the main window.

I think this issue should be BLOCKER. because unexpected bookmark item will be deleted when i chose "Delete" on the PlacesContextMenu.

Reproducible: Always

Steps to Reproduce -1:
1. Start Minefield with new profile
2. Open Sidebar Bookmarks (Ctrl + B)
3. Select a Bookmark item(for example "Getting Started") in the Sidebar.
4. Right click on a Bookmark item (for example "latest Headlines") on the Bookmarks toolbar. (
5. Execute Delete or Properties or Copy command

Steps to Reproduce -2:
1. Start Minefield with new profile
2. Open Sidebar Bookmarks (Ctrl + B)
3. Select a Bookmark item(for example "Most Visitedd") in the Sidebar.
4-1. Click "Bookmarks" on the menu bar.
4-2. Right click on a Bookmark item (for example "Mozilla Firefox") on the Bookmarks pull down menu. (
5. Execute Delete or Properties or Copy command

Actual Results:
 The command is executed for the selected item in step 3.

Expected Results:
 The command should be executed for the right clicked item in step 4 (or 4-2).

This issue happens on the following Namoroka/3.6.3pre also.
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.3pre) Gecko/20100327 Namoroka/3.6.3pre ID:20100327042606

regression window:

works:
http://hg.mozilla.org/mozilla-central/rev/90d3e6d2cbb9
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090610 Namoroka/3.6a1pre ID:20090610042525

fails:
http://hg.mozilla.org/mozilla-central/rev/4430cae50dad
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090611 Namoroka/3.6a1pre ID:20090611044033

pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=90d3e6d2cbb9&tochange=4430cae50dad

candidate bug:
 Bug 488846 -  Places code assumes non-focusable elements can be focused

Comment 1

[Alice0775 White]

Workaround fix: places/controller.js

function doGetPlacesControllerForCommand(aCommand)
{
-  var placesController = top.document.commandDispatcher
-                            .getControllerForCommand(aCommand);
-  if (!placesController) {
-    // If building commands for a context menu, look for an element in the
-    // current popup.
-    var element = document.popupNode;
-    while (element) {
-      var isContextMenuShown = ("_contextMenuShown" in element) && element._contextMenuShown;
-      // Check for the parent menupopup or the hbox used for toolbars
-      if ((element.localName == "menupopup" || element.localName == "hbox") &&
-          isContextMenuShown) {
-        placesController = element.controllers.getControllerForCommand(aCommand);
-        break;
-      }
-      element = element.parentNode;
-    }
-  }
-
-  return placesController;
+  if (document.popupNode) {
+    var view = PlacesUIUtils.getViewForNode(document.popupNode);
+    if (view)
+      return view._controller
+  }
+  // aCommand is not calling from placesContext
+  return top.document.commandDispatcher
+                        .getControllerForCommand(aCommand);
}

Comment 5

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

Assigning to Mano since he was thinking to a solution today

Comment 8

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

should probably block final for the dataloss

Comment 11

[Asaf Romano (gone)]

Attachment: patch

Comment 12

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

Comment on attachment 476588 [details] [diff] [review]
patch

> function doGetPlacesControllerForCommand(aCommand)
> {
>+  // A context menu may be built for views which are not focusable.

s/views which are not focusable/non-focusable views/

>+  // Thus, we first try to look for a view associated with document.poupNode

typo: poupNode should be popupNode, while here I'd replace "associated with document.popupNode" with "associated with current popupNode.", this is descriptive, not code.

>+  let element = document.popupNode;

while here, could you rename "element" that is too generic to "popupNode"

>+  if (element) {
>+    let view = PlacesUIUtils.getViewForNode(element);

here as well

>+  // When we're not building a context menu, only focusable views
>+  // are possible.  For those case, we can safely use the command dispatcher.

s/For those case,/Thus/

>diff -r 6066d938a0dd browser/components/places/content/tree.xml

>diff -r 6066d938a0dd browser/components/places/tests/browser/Makefile.in
>--- a/browser/components/places/tests/browser/Makefile.in	Sat Sep 18 12:38:43 2010 -0700
>+++ b/browser/components/places/tests/browser/Makefile.in	Sun Sep 19 02:45:48 2010 +0200
>@@ -60,12 +60,13 @@
> 	browser_forgetthissite_single.js \
> 	browser_library_left_pane_commands.js \
> 	browser_drag_bookmarks_on_toolbar.js \
> 	browser_library_middleclick.js \
> 	browser_library_views_liveupdate.js \
> 	browser_views_liveupdate.js \
> 	browser_sidebarpanels_click.js \
> 	browser_library_infoBox.js \
>+  browser_555547.js \
> 	$(NULL)

sad but true, this file still uses tabs, so either use tabs or convert the Makefile to use spaces.
I'm fine with both solutions.

>diff -r 6066d938a0dd browser/components/places/tests/browser/browser_555547.js

>+function test() {  

trailing spaces here.

>+  // If a sidebar is already open, close it.
>+  let sidebar = document.getElementById("sidebar");

move this down where it is first used.

>+  let sidebarBox = document.getElementById("sidebar-box");
>+  if (!sidebarBox.hidden) {
>+    info("Unexpected sidebar found - a previous test failed to cleanup correctly");

this should be a proper is(sidebarBox.hidden, "") error and not an info, otherwise people would not notice it and nobody would ever fix the unclean test. unless you've found we often fall in this case while running tests.

>+  waitForExplicitFinish();

put this just after test() { so it's immediately clear it's an async test.

>+  sidebar.addEventListener("load", function() {
>+    sidebar.removeEventListener("load", arguments.callee, true);
>+    let tree = sidebar.contentDocument.getElementById("bookmarks-view");
>+
>+    executeSoon(function() {

add a comment above that Places view is build on load, so we enqueue the testing code.

>+      // Focus the tree and check if its controller is returned.
>+      tree.focus();
>+      let controller = doGetPlacesControllerForCommand("placesCmd_copy");
>+      let treeController = tree.controllers
>+                               .getControllerForCommand("placesCmd_copy");
>+      ok(controller == treeController, "tree controller was returned");
>+
>+      // Open the context menu for a toolbar item, and check if the toolbar's
>+      // controller is returned.
>+      let toolbarItems = document.getElementById("PlacesToolbarItems");
>+      EventUtils.synthesizeMouse(toolbarItems.childNodes[0],
>+                                 4, 4, { type: "contextmenu", button: 2 },
>+                                 window);

should probably ensure we have some children because synthesizing an event on the first one.

>+      controller = doGetPlacesControllerForCommand("placesCmd_copy");
>+      let toolbarController = document.getElementById("PlacesToolbar")
>+                                      .controllers
>+                                      .getControllerForCommand("placesCmd_copy");
>+      ok(controller == toolbarController, "the toolbar controller was returned");
>+
>+      document.getElementById("placesContext").hidePopup();

is the context menu code synchronous on all platforms or should we wait for popupShowing?
I just fear that we call hidePopup when the popup has not yet been opened.

>+      // Now that the context menu is closed, try to get the tree controller again.
>+      tree.focus();
>+      controller = doGetPlacesControllerForCommand("placesCmd_copy");
>+      ok(controller == treeController, "tree controller was returned");
>+      

trailing spaces.

>+      toggleSidebar();
>+      finish();

you are not restoring personalToolbar collapsed status

>+    });
>+  }, true);

reasoning behind using capture mode?

The test is a nice addition but does not look like it is testing the reported case where with sidebar open one tries to delete a bookmark from toolbar and the current selected element in sidebar is removed as well, it is testing the underlying code. Would be nice to also add a test that creates 2 bookmarks, focuses the first one in the sidebar and then calls cmd_delete on the second one in the toolbar, then checks that only the second one has been removed.
Depending on you available time

Comment 13

[ithinc]

Please have a test with a Ctrl-right click.

Comment 15

[Asaf Romano (gone)]

Attachment: fixed

I haven't replaced my test - I think it's fine here and covers more than testing the bug in question. However, I could write a follow up test after this lands if you want me to.

Comment 16

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

Comment on attachment 502462 [details] [diff] [review]
fixed

>diff -r 4a3866321a14 browser/components/places/tests/browser/browser_555547.js
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+++ b/browser/components/places/tests/browser/browser_555547.js	Mon Jan 10 14:47:17 2011 +0200
>@@ -0,0 +1,60 @@
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/
>+ */
>+
>+function test() {
>+  waitForExplicitFinish();
>+
>+  let sidebarBox = document.getElementById("sidebar-box");
>+  alert(sidebarBox.hidden);

this alert is probably debug code that should be removed.


>+    // The laces view is build on load, so we enqueue our test.

typo: s/laces/Places/

Btw, the r- is not due to code but to the fact the test seems to fail if run with other b-c tests. The test itself runs fine, the Places tests as a bunch do too, but not with all b-c tests in topsrcdir/browser/ folder. Looks like there is a bad interaction with previous browser tests, maybe related to tests using Customize?

TEST-PASS | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_555547.js | The sidebar should be hidden
TEST-PASS | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_555547.js | tree controller was returned
JavaScript error: chrome://browser/content/browser.xul, line 1: this._view is null
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_555547.js | the toolbar controller was returned
JavaScript error: chrome://browser/content/browser.xul, line 1: this._view is null
TEST-PASS | chrome://mochitests/content/browser/browser/components/places/tests/browser/browser_555547.js | tree controller was returned

the this._view is null seems to be fired by context menu onpopupshowing (most likely http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/placesOverlay.xul#127)

Comment 18

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

ugh, interesting, when we fail in getViewForNode, node.localName is "tab" rather than "toolbarbutton"... Doesn't make much sense.

Comment 19

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

looking at other tests I've seen then set document.popupNode manually, most likely the popupNode we get is the one setup by a previous tabs test.
Does synthesizing the context click set the popupNode as well?

Comment 21

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

actually, we could even nullify document.popupNode in the browserTest harness, since looks like a bunch of tests are not clearing it correctly on finish.
There are at least 6 b-c tests not clearing it.

Comment 22

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

I've filed Bug 630838 to implement b-c tests harness automatic cleanup of document.popupNode, it's easy and sounds like a good choice against oranges.

Comment 23

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

Attachment: patch v1.2

addressed the 2 typos I found above, with bug 630838 in the tree the test passes, so my r- becomes a r+

Comment 24

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

http://hg.mozilla.org/mozilla-central/rev/58138404336f

Comment 25

[Ben]

Verified OK with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b11) Gecko/20100101 Firefox/4.0b11 ID:20110203141415