Closed Bug 555547 Opened 10 years ago Closed 9 years ago

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

Categories

(Firefox :: Bookmarks & History, defect)

3.6 Branch
x86
Windows 7
defect
Not set

Tracking

()

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

People

(Reporter: alice0775, Assigned: mano)

References

Details

(Keywords: dataloss, regression, Whiteboard: [softblocker][fx4-fixed-bugday])

Attachments

(1 file, 2 obsolete files)

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
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);
}
Blocks: 560198
Duplicate of this bug: 558635
Duplicate of this bug: 557031
Duplicate of this bug: 546281
Assigning to Mano since he was thinking to a solution today
Assignee: nobody → mano
Duplicate of this bug: 565487
Summary: A command of plasecContextMenu is carried out for a wrong bookmark item → A command of placesContextMenu is carried out for a wrong bookmark item
Duplicate of this bug: 569396
should probably block final for the dataloss
blocking2.0: --- → ?
Keywords: dataloss
Duplicate of this bug: 580557
blocking2.0: ? → final+
Duplicate of this bug: 595537
Attached patch patch (obsolete) — Splinter Review
Attachment #476588 - Flags: review?(mak77)
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
Attachment #476588 - Flags: review?(mak77) → review-
Flags: in-testsuite?
Please have a test with a Ctrl-right click.
Whiteboard: [needs new patch]
Duplicate of this bug: 582909
Attached patch fixed (obsolete) — Splinter Review
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.
Attachment #476588 - Attachment is obsolete: true
Attachment #502462 - Flags: review?(mak77)
Whiteboard: [needs new patch]
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)
Attachment #502462 - Flags: review?(mak77) → review-
Duplicate of this bug: 624389
ugh, interesting, when we fail in getViewForNode, node.localName is "tab" rather than "toolbarbutton"... Doesn't make much sense.
Whiteboard: [softblocker]
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?
Whiteboard: [softblocker] → [softblocker][needs new patch]
Duplicate of this bug: 559507
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.
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.
Attached patch patch v1.2Splinter Review
addressed the 2 typos I found above, with bug 630838 in the tree the test passes, so my r- becomes a r+
Attachment #502462 - Attachment is obsolete: true
Attachment #509105 - Flags: review+
Depends on: 630838
http://hg.mozilla.org/mozilla-central/rev/58138404336f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [softblocker][needs new patch] → [softblocker]
Target Milestone: --- → Firefox 4.0b12
Verified OK with Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b11) Gecko/20100101 Firefox/4.0b11 ID:20110203141415
Status: RESOLVED → VERIFIED
Whiteboard: [softblocker] → [softblocker][fx4-fixed-bugday]
Duplicate of this bug: 642387
You need to log in before you can comment on or make changes to this bug.