496635 - Middle click bookmark in the right-upper pane of the library does not open in new tab.

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Alice0775 White]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090605 Firefox/3.5.0 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090605 Shiretoko/3.5pre ID:20090605044036

Probably it is typo.
chrome\browser.jar\content\browser\places\places.js

  onTreeClick: function PO_onTreeClick(aEvent) {
    // Only handle clicks on tree children.
    if (aEvent.target.localName != "treechildren")
      return;

    var currentView = aEvent.currentTarget;
    var selectedNode = currentView.selectedNode;
    if (selectedNode) {
      var doubleClickOnFlatList = (aEvent.button == 0 && aEvent.detail == 2 &&
                                   aEvent.target.parentNode.flatList);
-      var middleClick = (Event.button == 1 && aEvent.detail == 1);
+      var middleClick = (aEvent.button == 1 && aEvent.detail == 1);

      if (PlacesUtils.nodeIsURI(selectedNode) &&
          (doubleClickOnFlatList || middleClick)) {
        // Open associated uri in the browser.
        PlacesOrganizer.openSelectedNode(aEvent);
      }


Reproducible: Always

Comment 1

[Alice0775 White]

This typo causes the following isue.

Middle click bookmark in the right-upper pane of the library does not open in new tab.

Comment 2

[Alice0775 White]

Regression range:
Works:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e49c05fc9122
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b5pre) Gecko/20090519 Shiretoko/3.5b5pre ID:20090519043912

Broken:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/acd2d4638228
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b5pre) Gecko/20090520 Shiretoko/3.5b5pre ID:20090520041838

Pushlog:
http://hg.mozilla.org/releases/mozilla-1.9.1/pushloghtml?fromchange=e49c05fc9122&tochange=acd2d4638228

Regression:
Bug 480238 -  Double-clicking tree column separator opens highlighted link

Comment 3

[Marco Bonardo [:mak]]

ugh, this is a small fix so we can most likely get approval. Thanks for the catch.

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch

includes a browser-chrome test that tests middle-clicking on a bookmark, a folder and a query in Library.

The code change is really small, simple typo fix as originally reporter.

Comment 5

[Marco Bonardo [:mak]]

pushed to the tryserver to ensure test has no issues on other platforms.

Comment 6

[Marco Bonardo [:mak]]

due to the fact the change is 1 char only i'd really like to see this in 3.5.

Comment 7

[Marco Bonardo [:mak]]

tryserver did not show any failure in browser-chrome tests (really linux busted but with a random/unrelated failure).

Comment 8

[Dietrich Ayala (:dietrich)]

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


>+    if (this._loadedURIs.length == gCurrentTest.URIs.length) {
>+      // We have correctly opened all URIs.

iiuc, this also means that if we never hit this point, due to a failure of some kind, the test will still pass.

is there a way to ensure that all tests ran, and tested all expected URLs, before completing?

>+  setup: function() {
>+    var bs = PlacesUtils.bookmarks;
>+    // Add a new unsorted bookmark.
>+    this._itemId = bs.insertBookmark(bs.unfiledBookmarksFolder,
>+                                     PlacesUtils._uri(this.URIs[0]),
>+                                     bs.DEFAULT_INDEX,
>+                                     "Title");
>+    // Select unsorted bookmarks root in the left pane.
>+    gLibrary.PlacesOrganizer.selectLeftPaneQuery("UnfiledBookmarks");
>+    isnot(gLibrary.PlacesOrganizer._places.selectedNode, null,
>+          "We correctly have selection in the Library left pane");

just checking for a selection is a little too trusting. should check node title or something to ensure the correct left-pane node was selected.

r=me otherwise

Comment 9

[Marco Bonardo [:mak]]

(In reply to comment #8)
> (From update of attachment 382120 [details] [diff] [review])
> 
> >+    if (this._loadedURIs.length == gCurrentTest.URIs.length) {
> >+      // We have correctly opened all URIs.
> 
> iiuc, this also means that if we never hit this point, due to a failure of some
> kind, the test will still pass.

it will start to timeout at any run, since finish() won't ever be called, nor we will move to the next test.
I thought about some way to make this fail, but it's hard to detect what could happen in such a case.

> >+  setup: function() {
> >+    var bs = PlacesUtils.bookmarks;
> >+    // Add a new unsorted bookmark.
> >+    this._itemId = bs.insertBookmark(bs.unfiledBookmarksFolder,
> >+                                     PlacesUtils._uri(this.URIs[0]),
> >+                                     bs.DEFAULT_INDEX,
> >+                                     "Title");
> >+    // Select unsorted bookmarks root in the left pane.
> >+    gLibrary.PlacesOrganizer.selectLeftPaneQuery("UnfiledBookmarks");
> >+    isnot(gLibrary.PlacesOrganizer._places.selectedNode, null,
> >+          "We correctly have selection in the Library left pane");
> 
> just checking for a selection is a little too trusting. should check node title
> or something to ensure the correct left-pane node was selected.

Checking node title would require getting a bundle, and i don't know itemId because it's a left pane shortcut... Btw if this selects the wrong node all next tests will fail since we won't find the bookmark we are trying to click onto.

Comment 10

[Dietrich Ayala (:dietrich)]

(In reply to comment #9)

ok, r=me as-is.

Comment 11

[Marco Bonardo [:mak]]

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

Asking approval for this 1 char-only change (just a typo fix), risk is practically zero.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

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

a191=beltzner

Comment 13

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/releases/mozilla-1.9.1/rev/e21aecdfa286

Comment 14

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/9003dcd9211f

Comment 15

[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