563291 - Selectively open tabs from about:sessionrestore

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Mick Weiss]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.4) Gecko/20100413 Firefox/3.6.4
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.4) Gecko/20100413 Firefox/3.6.4

You had 70 tabs open, and you really just want to open 1 and you can't double click on it in the list and you don't remember the url either.

You'd think that double clicking it would open it in a new tab/window - but that doesn't happen.

Reproducible: Always

Steps to Reproduce:
1. get a session:restore page
2. try to double click any of the links that show up
3.
Actual Results:  
It selects the link, but nothing happens.

Expected Results:  
A tab should open.

I'd like to fix this. If anyone has any thoughts/suggestions or objections please comment or email me.

Comment 1

[Mick Weiss]

Attachment: Double click on links in session restore

This patch adds the ability to open links from session:restore selectively. The ability to do so was apparently available through ctrl clicking (which didn't work for me on a MacBook). I've seen users wonder why they can't double click on the links, since it would seem to be a common use case. The only thing that I find a bit annoying is that you need to change browser.tabs.loadInBackground to false for the tab to load in the foreground (which is what people would expect to be the default behavior). Maybe "false" should be the default configuration under about:config, or is there is a better way to do that?

- Mick

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 443802 [details] [diff] [review]
Double click on links in session restore

>-    // restore this specific tab in the same window for middle-clicking
>-    // or Ctrl+clicking on a tab's title
>-    if ((aEvent.button == 1 || aEvent.ctrlKey) && col.value.id == "title" &&
>+    // restore this specific tab in the same window for double clicking or middle clicking
>+    // or Ctrl+clicking on a tab's title - note: ctrl clicking doesn't work on Mac
>+    if ((aEvent.button == 1 || aEvent.button == 0 && aEvent.detail == 2 || aEvent.ctrlKey) && col.value.id == "title" &&
>         !treeView.isContainer(row.value))

Please add a line break before col.value.id == "title".

Comment 3

[Mick Weiss]

Attachment: fixed formatting

I hope that I have the formatting correct this time. Let me know if I need to change it again.

thanks

Comment 4

[Dão Gottwald [:dao]]

I was thinking of this:

if ((aEvent.button == 1 || aEvent.button == 0 && aEvent.detail == 2 || aEvent.ctrlKey) &&
    col.value.id == "title" &&
    !treeView.isContainer(row.value))

Comment 5

[Mick Weiss]

Attachment: fixed formatting again

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 443830 [details] [diff] [review]
fixed formatting again

>+    if ((aEvent.button == 1 || aEvent.button == 0 && aEvent.detail == 2 || aEvent.ctrlKey) &&
>+      col.value.id == "title" &&
>+      !treeView.isContainer(row.value))

Sorry for being picky again, but 'col' and '!treeView' should line up with '(aEvent', i.e. there are two more spaces needed on both lines.

Comment 7

[Mick Weiss]

Attachment: format fix

Comment 8

[Simon Bünzli]

Comment on attachment 443943 [details] [diff] [review]
format fix

Now it's one space too many... ;-)

Comment 9

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Heh, we're really making you jump through hoops here. But thanks for doing it :)

(In reply to comment #1)
> ... The only thing that
> I find a bit annoying is that you need to change browser.tabs.loadInBackground
> to false for the tab to load in the foreground (which is what people would
> expect to be the default behavior). Maybe "false" should be the default
> configuration under about:config, or is there is a better way to do that?

So currently, you can hold shift when opening the single tab, and the tab is loaded in the foreground. Not super discoverable though.

Changing the default pref value would have other consequences, so we don't want to do that. What we can do is reverse the logic in restoreSingleTab for aShifted.

Feel free to file a new bug about that (it should be done separately from this bug).

Comment 10

[Mick Weiss]

That is incorrect. You can hold ctrl and click (on linux and windows probably) or you can middle click. On Mac, I can assure you that shift + click (or ctrl + click) both don't work. Not that that really matters anyhow ;)

Comment 11

[Mick Weiss]

Attachment: please tell me that this is the right formatting or send me the arguments that i can pass to astyle seriously...

I'm not sure if Simon was joking or not when he said there was one space too many, but since Simon worked on the file I guess he wasn't.

Comment 12

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #11)
> Created an attachment (id=444004) [details]
> please tell me that this is the right formatting or send me the arguments that
> i can pass to astyle seriously...

This is correct. Thanks.

(In reply to comment #10)
> That is incorrect. You can hold ctrl and click (on linux and windows probably)
> or you can middle click. On Mac, I can assure you that shift + click (or ctrl +
> click) both don't work. Not that that really matters anyhow ;)

I meant that ctrl + shift + click (on win/linux) currently opens the "link" in the foreground. I assume shift + middle click does the same. With your patch applied, shift + double clicking does as well on Mac.

Comment 13

[Simon Bünzli]

(In reply to comment #11)
Please read https://developer.mozilla.org/En/Developer_Guide/Coding_Style#Operators for the style rule behind the specific indentation required in this case (|col.value.id == "title"| belongs to the same conjunction as the whole |(aEvent.button == 1 && ...)| bit, so the "c" of "col" has to be aligned with the opening parenthesis). Even though our code base isn't perfectly consistent in style, we try to stick to these guidelines as far as reasonably possible.

Comment 14

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/e7f38ba5e6a5

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #1)
> ability to do so was apparently available through ctrl clicking (which didn't
> work for me on a MacBook).

Filed bug 565024 on this.

Comment 16

[u279076]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a5pre) Gecko/20100526 Minefield/3.7a5pre

Verified fixed.