Closed Bug 496635 Opened 15 years ago Closed 15 years ago

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

Categories

(Firefox :: Bookmarks & History, defect)

3.5 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: alice0775, Assigned: mak)

References

Details

(Keywords: fixed1.9.1, regression)

Attachments

(1 file)

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
Version: unspecified → 3.5 Branch
This typo causes the following isue.

Middle click bookmark in the right-upper pane of the library does not open in new tab.
Keywords: regression
Summary: Typo in places.js, 'Event.button' shoud be 'aEvent.button' → Middle click bookmark in the right-upper pane of the library does not open in new tab.
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
Blocks: 480238
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-firefox3.5?
Assignee: nobody → mak77
Flags: wanted1.9.1.x?
Flags: blocking-firefox3.5?
Flags: blocking-firefox3.5-
ugh, this is a small fix so we can most likely get approval. Thanks for the catch.
Attached patch patchSplinter Review
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.
Attachment #382120 - Flags: review?(dietrich)
Flags: in-testsuite?
pushed to the tryserver to ensure test has no issues on other platforms.
due to the fact the change is 1 char only i'd really like to see this in 3.5.
Flags: wanted-firefox3.5?
Whiteboard: [needs review dietrich]
tryserver did not show any failure in browser-chrome tests (really linux busted but with a random/unrelated failure).
OS: Windows XP → All
Hardware: x86 → All
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
Attachment #382120 - Flags: review?(dietrich) → review+
Whiteboard: [needs review dietrich]
(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.
(In reply to comment #9)

ok, r=me as-is.
Comment on attachment 382120 [details] [diff] [review]
patch

Asking approval for this 1 char-only change (just a typo fix), risk is practically zero.
Attachment #382120 - Flags: approval1.9.1?
Whiteboard: [can land]
Attachment #382120 - Flags: approval1.9.1? → approval1.9.1+
https://hg.mozilla.org/mozilla-central/rev/9003dcd9211f
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [can land]
Flags: wanted1.9.1.x?
Flags: wanted-firefox3.5?
Flags: in-testsuite?
Flags: in-testsuite+
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: