402558 - urls from bookmarks folder in sidebar don't open in tabs on middle-click

Status: VERIFIED FIXED

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

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9a9pre) Gecko/2007110405 Minefield/3.0a9pre

In the bookmarks sidebar, middle-clicking a bookmarks folder doesn't open the bookmarks in tabs (it doesn't open anything).

In Firefox 2.0.0.*, it opens them.

Comment 1

[Brian Polidoro]

2007051804 works
2007051821 broke

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-05-18+04%3A00%3A00&maxdate=2007-05-18+21%3A00%3A00&cvsroot=%2Fcvsroot

Probably broke from:
"Turing on Bookmarks on Places."

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: serve middle-click on bookmark folders

this is for firefox 2 parity

serve middle-click on bookmarks folders + 
trailing spaces fix +
a fix to openContainerNodeInTabs since if there are no urls to open, it opens an (untitled) tab 

Mano: should i fix the last problem in _openTabset with an "if (aURLS.length <= 0) return;" instead of in openContainerNodeInTabs (that uses _openTabset)?

Reed: please, don't check-in until i put checkin-needed in keywords, i need to address review comments, thank you :)

Comment 3

[Asaf Romano (gone)]

Comment on attachment 293492 [details] [diff] [review]
serve middle-click on bookmark folders

Why just folders? In Places, Open-In-Tabs is supported for all urls-containers (except day containers, known bug)

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

i was looking at FF2 parity,  FF2 does not open history containers

also, open in tabs does not get all urls recursively, it opens only the first sublevel, so in history sidebar it would serve only hosts folders and date folders, but not grouped by date & hosts.

changing to open all containers has problems with day containers (as you said, it tries to open a bunch of not visible visits), but also on host containers, and also gives me an assertion in CountVisibleRowsForItems...

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: open urls for all containers

this is for all containers.

notes: 
on middle-click the node container opens, than urls are loaded, this is because i need to get only childs with child.viewIndex >= 0 (or open in tabs will try to open all hidden duplicates, so for a folder with 1 visible item i end up with a request to open 64 tabs!)

still don't know if the check on urlsToOpen.length should be done in _openTabset

after opening, itemInserted for dynamic containers is calling _countVisibleRowsForItem for a non visible item, and that is causing an assertion, so i'm excluding dynamic containers from _countVisibleRowsForItem

Comment 6

[Asaf Romano (gone)]

I think we should consider re-disabling dynamic containers for 1.9, the implementation is not all that usable at this point.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

open only visible items will be fixed in Bug 409998

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: open container urls on middle-click

- removed fix for non visible items (Bug 409998)
- moved empty url array check in _openTabset
- middle click open urls ONLY if container is open (since context menu Open All in Tabs is disabled for closed containers)

note: i don't get asserts anymore on dynamic containers, don't know what other patch could have fixed that though...

Comment 9

[Asaf Romano (gone)]

Since when "Open All
in Tabs is disabled for closed containers", that seems like a bug/regression.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

see https://bugzilla.mozilla.org/show_bug.cgi?id=409998#c8

Comment 11

[Asaf Romano (gone)]

No reasoning there either.

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

the problem is that to open only visible items we rely on viewIndex, so the container must be opened before calling open in tabs... 

a better solution could be change getURLsForContainerNode to return an array of unique urls, and remove check on viewIndex

Comment 13

[Asaf Romano (gone)]

This should be done only the  query/site container in question is closed, when it's opened, any visible url nodes should be "mapped" to tabs.

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 295230 [details] [diff] [review]
open container urls on middle-click

i need to re-check a couple of things, clearing requests

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

i was tryiong to create a new util to see if a folder has url childs (without having to use getURLsForContainerNode(node).length == 0 that is slow), but for the right pane when i call node.containerOpen = true to use childCount i get a "cannot change properties for a Native Wrapped object".

I think that is because folders in the right pane are not containers (see https://bugzilla.mozilla.org/show_bug.cgi?id=405198#c24)

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: wip

mano: this is working for everything but not for dynamic containers in the left pane,  mainly because
this.getFolderContents(aNode.itemId, false, false).root; 
returns an empty container for left pane bookmark menu, toolbar and unfiled, because they are place:folder=x... so having no children the option open all in tabs is disabled.
how can i get correct content? changing expandQueries option does not work (the interface says that for simple folder queries it has no effect since they should be expanded by default)

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 296319 [details] [diff] [review]
wip

partly fixed in Bug 411803

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: fixes modifiers too

- add a new util to check if a container has child urls (perf)
- fix right click context to use that
- fix sidebar to open in tabs on middle-click, CTRL+click, META+click

still does not count correctly for Library left pane folders (need some hint, could be spun-off to a new bug)

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 296524 [details] [diff] [review]
fixes modifiers too

this patch needs a refresh

Comment 22

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch

unbitrot, used new functions in utils

Comment 23

[Damian Shaw [Quan]]

This covers the regression that middle click stopped working on "Open All in Tabs"?

Comment 24

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

in the context menu or in the menu added to popups? can you explain better? thank you.

Comment 25

[Damian Shaw [Quan]]

1) Go to bookmark menu
2) Go to sub-menu in bookmark menu, with multiple bookmarks in it
3) Middle click on "Open all in tabs"
4) Nothing happens!!

Uses latest trunk build as I write this. I searched for a bug on this, kind of looked like Bug 418611 and this was its duplicate.

Comment 26

[Craig Ringer]

Same with control-click:

Steps:
- Use the bookmark organizer to make a folder under the bookmarks toolbar folder and populate it with a couple of individual bookmarks.
- Control click on a folder in the bookmarks toolbar (under the address bar)

Actual (FF3):
- The folder opens as if an unmodified left click had been made

Expected (and FF2 behaviour):
- Each bookmark in the folder is opened as a tab. 

The above is also valid if "control-click" is replaced with "middle-click".

Comment 27

[Asaf Romano (gone)]

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


>Index: browser/components/places/content/sidebarUtils.js
>===================================================================

>-    var modifKey = aEvent.shiftKey || aEvent.ctrlKey || aEvent.altKey || 
>-                   aEvent.metaKey  || (aEvent.button != 0);
>-    if (!modifKey && tbo.view.isContainer(row.value)) {
>+
>+    var modifKey = aEvent.ctrlKey || Event.metaKey;
>+    var openInTabs = aEvent.button == 1 || (aEvent.button == 0 && modifKey);

this should rather be #ifdef'ed.... Cmd+click on mac and Ctrl+click on windows. Shift is supposed to open in tabs in a new window, I think. Check the toolbar.

Comment 28

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #27)

> Shift is supposed to open in tabs in a new window, I think. Check the toolbar.

the toolbar does open tabs in new window if you Shift-click the "open all in tabs" item, while shift-click on the folder is like a normal click (opens the folder).

the FX2 sidebar opens in new window when you shift-click on the folder, so i'm adding this check to get back the same behaviour as FX2

Comment 29

[Asaf Romano (gone)]

I think shift+middle-click for open-tabs-in-new-tabs makes sense in the toolbar and menu context as well.

Comment 30

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Mano, this is the actual behaviour with an updated patch:

1. left-click: toggle container open (CORRECT)
2. ctrl+left-click: open container contents in tabs (append) (CORRECT)
3. shift+left-click: open container contents in tabs in new window (CORRECT)
4. ctrl+shift+left-click: open container contents in tabs (replace) (CORRECT?)

5. middle-click: open container contents in tabs (append) (CORRECT)
6. ctrl+middle-click: open container contents in tabs (append) (CORRECT)
7. shift+middle-click: open container contents in tabs (replace) (CORRECT?)
8. ctrl+shift+middle-click: open container contents in tabs (replace) (CORRECT?)

"CORRECT?" items are mostly due to the fact we are simply passing the work to openContainerNodeInTabs that ends up calling whereToOpenLink(aEvent, false, true); If we want to change that behaviour we must patch _openTabSet too to force the opening in a new window.

Clicking both keys makes unclear the user will, in FX2 that does not open anything, should be the same here?

(In reply to comment #29)
> I think shift+middle-click for open-tabs-in-new-tabs makes sense in the toolbar and menu context as well.

this will most probably have the same behaviour as previous points, with shift+middle-click, whereToOpenLink will _replace_ contents with new tabs.

Comment 31

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch

implements comment #30

Comment 32

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

i want to add gutter selection in onClick to this patch, should solve Bug 421210

Comment 33

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Mike, could you define what is the expected behaviour about comment #30?

Comment 34

[Mike Connor [:mconnor]]

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

I believe that the behaviour outlined in comment 30 is correct, as I understand it.  I think we've got pretty solid logic in whereToOpenLink at this point, so we should really just trust it to do the right thing...

Comment 35

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch

this fixes:

- containers in sidebar open on middle-click or left-click + modifiers
- selection in sidebar can be done in gutter (space before the favicon)
- you can middle-click the "Open all in tabs" option in menupopups
- open all in tabs context menu option disabled state is calculated faster
- open all in tabs context menu option works correctly for folder shortcuts
- onclick handler in BookmarksEventHandler code cleanup (rem. useless code)
- middle-click or left-click with modifiers works on folders in toolbar and menus

after ev. review + checkin will look around to close related bugs

Comment 36

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: unbitrot for PlacesUIUtils

Comment 37

[Asaf Romano (gone)]

Comment on attachment 309413 [details] [diff] [review]
unbitrot for PlacesUIUtils

>+    var modifKey = aEvent.metaKey || aEvent.shiftKey);

Syntax error, in both places. I'll fix this on checkin.

r=mano

Comment 38

[Asaf Romano (gone)]

I'm simplifying the check to:
#ifdef XP_MACOSX
    var modifKey = aEvent.metaKey || aEvent.shiftKey;
#else
    var modifKey = aEvent.ctrlKey || aEvent.shiftKey;
#endif
    if (aEvent.button == 2 || (aEvent.button == 0 && !modifKey))
      return;

Comment 39

[Asaf Romano (gone)]

Attachment: for checkin

Comment 40

[Asaf Romano (gone)]

mozilla/browser/base/content/browser-places.js 1.120
mozilla/browser/components/places/content/bookmarksPanel.xul 1.12
mozilla/browser/components/places/content/controller.js 1.223
mozilla/browser/components/places/content/history-panel.xul 1.16

Comment 41

[tabmix.onemen]

after this fix when i open folder with many tab with middle-click the Confirm open dialog is showing before the menus is closing.

the dialog is behind the open menus

Comment 42

[Asaf Romano (gone)]

onemen.one: Please file a bug on that and cc me and Marco.

Comment 44

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008031704 Minefield/3.0b5pre

Comment 45

[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