Closed Bug 909102 Opened 11 years ago Closed 7 years ago

Middle click on folder in Library left panel is broken

Categories

(Firefox :: Bookmarks & History, defect, P3)

defect
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: tabmix.onemen, Assigned: standard8)

References

Details

(Keywords: regression)

Attachments

(1 file)

Middle click on folder in Library left panel doesn't work

Error: ReferenceError: selectedNode is not defined
Source File: chrome://browser/content/places/places.js
Line: 295

This function was broken by bug 675902
Keywords: regression
Whiteboard: [triage]
Blocks: fxdesktoptriage
No longer blocks: fxdesktopbacklog
Blocks: fxdesktopbacklog
No longer blocks: fxdesktoptriage
Whiteboard: [triage] → p=0
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Whiteboard: p=0 → p=2
Assignee: nobody → mano
Assignee: mano → nobody
Points: --- → 2
Flags: qe-verify?
Whiteboard: p=2
Priority: -- → P3
Assignee: nobody → standard8
Blocks: 1342459
Comment on attachment 8842824 [details]
Bug 909102 - Fix middle-clicking on a folder in the left panel of the Library (to open all items within the folder).

https://reviewboard.mozilla.org/r/116602/#review118220

I came across this as eslint picked it up when looking at enabling the no-undef rule. The fix is simple, the test was the longest bit ;-)

::: browser/components/places/tests/browser/browser_library_left_pane_middleclick.js:1
(Diff revision 1)
> +/* vim:set ts=2 sw=2 sts=2 et: */

This file is based on the existing browser_library_middleclick.js
Comment on attachment 8842824 [details]
Bug 909102 - Fix middle-clicking on a folder in the left panel of the Library (to open all items within the folder).

https://reviewboard.mozilla.org/r/116602/#review118644

::: browser/components/places/tests/browser/browser_library_left_pane_middleclick.js:1
(Diff revision 1)
> +/* vim:set ts=2 sw=2 sts=2 et: */

fwiw, since this only contains a single test case it could be far simpler, for example all the sub-harness to run multiple tests may go.

That said, I understand this is not critical and you are working on other more important stuff, so I'm not going to ask to rewrite the whole thing.
We'll do one day, in the remote future :)

::: browser/components/places/tests/browser/browser_library_left_pane_middleclick.js:118
(Diff revision 1)
> +// ------------------------------------------------------------------------------
> +
> +function test() {
> +  waitForExplicitFinish();
> +  // Increase timeout, this test can be quite slow due to waitForFocus calls.
> +  requestLongerTimeout(2);

This is probably not needed, it may be needed in browser_library_middleclick.js because it runs 3 sub-tests. Here we only have one.
Attachment #8842824 - Flags: review?(mak77) → review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/55d9a513c90e
Fix middle-clicking on a folder in the left panel of the Library (to open all items within the folder). r=mak
I've fixed the failures - I had to make the test double-click on the "Other Bookmarks" entry a second time, so that it was closed & didn't affect other tests.

Pushing the fix to try, just to make sure.
Flags: needinfo?(standard8)
In general this method of clicking on a given entry is not really "safe", for example if we'd add something else to the left pane, all of this would start failing.
You could probably use a combination of library.PlacesOrganizer.selectLeftPaneQuery("UnfiledBookmarks"); library.PlacesOrganizer._places.selectedNode.containerOpen = true/false, and library.PlacesOrganizer._places.view.treeIndexForNode(node)

That may also simplify your double-click needs (you could just set containerOpen)
Thanks for the hints, that's much nicer!
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/15fa3319c37f
Fix middle-clicking on a folder in the left panel of the Library (to open all items within the folder). r=mak
https://hg.mozilla.org/mozilla-central/rev/15fa3319c37f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Seems like a pretty low-risk fix if we wanted to consider it for 53/54 backport (OTOH, probably not horribly urgent given its age if the answer is no). What are your thoughts, Mark?
Flags: needinfo?(standard8)
Flags: in-testsuite+
It is a regression, and a simple fix, but I think due to the age issue (and no dups) we can just let it ride the trains.
Flags: needinfo?(standard8)
You need to log in before you can comment on or make changes to this bug.