Closed Bug 402558 Opened 13 years ago Closed 13 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3 beta5

People

(Reporter: aryx, Assigned: mak)

References

Details

(Keywords: regression)

Attachments

(1 file, 9 obsolete files)

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.
Flags: blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3+
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → Firefox 3 M10
Target Milestone: Firefox 3 M10 → Firefox 3 Mx
Target Milestone: Firefox 3 Mx → Firefox 3 M11
Priority: -- → P3
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 :)
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attachment #293492 - Flags: review?(mano)
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)
Attachment #293492 - Flags: review?(mano) → review-
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...
Attached patch open urls for all containers (obsolete) — Splinter Review
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
Attachment #293492 - Attachment is obsolete: true
Attachment #293513 - Flags: review?(mano)
I think we should consider re-disabling dynamic containers for 1.9, the implementation is not all that usable at this point.
open only visible items will be fixed in Bug 409998
- 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...
Attachment #293513 - Attachment is obsolete: true
Attachment #295230 - Flags: review?(mano)
Attachment #293513 - Flags: review?(mano)
Since when "Open All
in Tabs is disabled for closed containers", that seems like a bug/regression.
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
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 on attachment 295230 [details] [diff] [review]
open container urls on middle-click

i need to re-check a couple of things, clearing requests
Attachment #295230 - Attachment is obsolete: true
Attachment #295230 - Flags: review?(mano)
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)
Attached patch wip (obsolete) — Splinter Review
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 on attachment 296319 [details] [diff] [review]
wip

partly fixed in Bug 411803
Attachment #296319 - Attachment is obsolete: true
Attached patch fixes modifiers too (obsolete) — Splinter Review
- 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)
Attachment #296524 - Flags: review?(mano)
Target Milestone: Firefox 3 beta3 → ---
Duplicate of this bug: 418407
Duplicate of this bug: 418611
Comment on attachment 296524 [details] [diff] [review]
fixes modifiers too

this patch needs a refresh
Attachment #296524 - Attachment is obsolete: true
Attachment #296524 - Flags: review?(mano)
Attached patch patch (obsolete) — Splinter Review
unbitrot, used new functions in utils
Attachment #305975 - Flags: review?(mano)
This covers the regression that middle click stopped working on "Open All in Tabs"?
in the context menu or in the menu added to popups? can you explain better? thank you.
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.
Keywords: regression
Priority: P3 → P2
Whiteboard: [has patch][needs review mano]
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 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.
Attachment #305975 - Flags: review?(mano) → review-
(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

I think shift+middle-click for open-tabs-in-new-tabs makes sense in the toolbar and menu context as well.
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.

Target Milestone: --- → Firefox 3
Whiteboard: [has patch][needs review mano] → [needs new patch]
Whiteboard: [needs new patch] → [needs def][swag: 0.5d]
Attached patch patch (obsolete) — Splinter Review
implements comment #30
Attachment #305975 - Attachment is obsolete: true
i want to add gutter selection in onClick to this patch, should solve Bug 421210
Blocks: 421210
Comment on attachment 307676 [details] [diff] [review]
patch

Mike, could you define what is the expected behaviour about comment #30?
Attachment #307676 - Flags: ui-review?(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...
Attachment #307676 - Flags: ui-review?(mconnor) → ui-review+
Attached patch patch (obsolete) — Splinter Review
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
Attachment #307676 - Attachment is obsolete: true
Attachment #308873 - Flags: review?(mano)
Whiteboard: [needs def][swag: 0.5d] → [has patch][needs review Mano]
Attached patch unbitrot for PlacesUIUtils (obsolete) — Splinter Review
Attachment #308873 - Attachment is obsolete: true
Attachment #309413 - Flags: review?(mano)
Attachment #308873 - Flags: review?(mano)
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
Attachment #309413 - Flags: review?(mano) → review+
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;
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][needs review Mano]
Target Milestone: Firefox 3 → Firefox 3 beta5
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

onemen.one: Please file a bug on that and cc me and Marco.
Duplicate of this bug: 420828
verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008031704 Minefield/3.0b5pre
Status: RESOLVED → VERIFIED
Blocks: 415651
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.