909102 - Middle click on folder in Library left panel is broken

Status: RESOLVED FIXED

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

Description

[tabmix.onemen]

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

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 909102 - Fix middle-clicking on a folder in the left panel of the Library (to open all items within the folder).

Review commit: https://reviewboard.mozilla.org/r/116602/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116602/

Comment 2

[Mark Banner (:standard8)]

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 3

[Marco Bonardo [:mak]]

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.

Comment 4

[Mark Banner (:standard8)]

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).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116602/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
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

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=81506913&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/7b8d033f7717

Comment 7

[Mark Banner (:standard8)]

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).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116602/diff/2-3/

Comment 8

[Mark Banner (:standard8)]

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.

Comment 9

[Marco Bonardo [:mak]]

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)

Comment 10

[Mark Banner (:standard8)]

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).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116602/diff/3-4/

Comment 11

[Mark Banner (:standard8)]

Thanks for the hints, that's much nicer!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/15fa3319c37f

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 15

[Mark Banner (:standard8)]

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.