310395 - Open Link in New Window - new background windows open with Address bar focused

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[Gary Yuen]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050928 Camino/1.0+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050928 Camino/1.0+

Opening a link in a new window places new window focus in the address bar. It
makes immediate keyboard scrolling with the page difficult.

Reproducible: Always

Steps to Reproduce:
1. Open a web page link in a new window
2. go to that window
3. the address bar has focus
Actual Results:  
The address bar has focus.

Expected Results:  
The window should have focus.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

WFM Camino 2005092804 (v1.0a1+), so it must be trunk-only.

Comment 2

[Samuel Sidler (old account; do not CC)]

Right, so this only happens if you have them open in the background (in the tab
prefs select "New windows and tabs: Load in background"). If they load in the
background and you switch to them, the location bar has focus. If they aren't
loaded in the background, content has focus.

Comment 3

[Samuel Sidler (old account; do not CC)]

(Edit: At least on the branch, someone should try this on the trunk to see if it
broke.)

Comment 4

[Wevah (Nate Weaver)]

I get the same behavior on the trunk. Specifically, new windows open in the
foreground start with the location field focused, but the content gets focus
once loading is complete. With background windows, the focus stays in the
location field when loading is complete.

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

OK, I see this in the branch, too, now that I understand what to do.  Tweaking
summary.

Better steps to reproduce:
1. Set Cmd-click to Open in a new window
2. Set New windows to Load in background (or use appropriate shift-modifiers)
3. Cmd-click a link to open a new background window

Results: location bar still focused
Exepected: content focused

Is this a regression (did it work in 0.8.4 or 0.9a2)?  There've been lots of
focus changes in the past month or so (leaving some regressions, bug 306245
among them)....

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I looked at this in 0.8.4 in my testing account, and the problem exists there, too.

We should probably look at this again for 1.1 when we try to fix the other focus problems (like bug 306245).

Comment 7

[Samuel Sidler (old account; do not CC)]

*** Bug 316395 has been marked as a duplicate of this bug. ***

Comment 8

[froodian (Ian Leue)]

I have a working patch for this, which I'll upload once I find a find a connection fast enough to update my source. -> me

Comment 9

[froodian (Ian Leue)]

Attachment: Makes new windows get focus irregardles of the load in bg setting

Comment 10

[froodian (Ian Leue)]

That is, makes the content pane of new windows get focus.  It doesn't break the load in bg setting.  That'd be dumb. Sorry for the bugspam.

Comment 11

[Håkan Waara]

Comment on attachment 222573 [details] [diff] [review]
Makes new windows get focus irregardles of the load in bg setting

* Will focus be stolen (from the front window) if we load a window with a URI in the background?

* There's a openNewTabWithURL: suffering from the same thing.

* I think the content area should only be activated if we're loading a URI; that is, not for about:blank or "".  This is what createNewTab: does.

* Search for loadURI: and loadURL:. You'll notice we have a lot of cases, and probably we should try to make them all consistent.

Comment 12

[froodian (Ian Leue)]

> * Will focus be stolen (from the front window) if we load a window with a URI
> in the background?

No, it does the right thing.

> * I think the content area should only be activated if we're loading a URI;
> that is, not for about:blank or "".  This is what createNewTab: does.
 
> * There's a openNewTabWithURL: suffering from the same thing.
>
> * Search for loadURI: and loadURL:. You'll notice we have a lot of cases, and
> probably we should try to make them all consistent.

Yeah.  The problem is that the name "activate" doesn't really describe the behavior, which would be better described as focusContent.  It looks like most people have been using activate as if it would bring the window to the front.  I think the best thing would be to not just fix all instances of activate to be consistent, but also to change the parameter name to focusContent.  

I'll make up a new patch addressing these comments.

Comment 13

[froodian (Ian Leue)]

Attachment: New BrowserWindowController.mm

The first of several upcoming patches.  This is the only one with real content (all others just change loadURL and loadURI's activate to focusContent).  90% of these cases are impossible to trigger, since switching tabs focuses the content area and various other reasons, but I tried to keep it as consistent as possible, and have it do what would be the right thing were the situation ever to somehow arise.  It also checks for "" or "about:blank" whenever it seemed appropriate.

Comment 14

[froodian (Ian Leue)]

Attachment: New BrowserTabView.mm

Comment 15

[froodian (Ian Leue)]

Attachment: New BrowserWindowController.h

Comment 16

[froodian (Ian Leue)]

Attachment: New BrowserWrapper.mm

Comment 17

[froodian (Ian Leue)]

Attachment: New BrowserWrapper.h

Comment 18

[froodian (Ian Leue)]

Attachment: New GoMenu.mm

Comment 19

[froodian (Ian Leue)]

Attachment: New MainController.mm

Forgot to mention up in the comments for BrowserWindowController.mm:  That patch also contains the fix for Bug 299548.  Since both fixes affect the same line of code, I'm making that bug dependent on this one.  Sorry for the bugspam, folks.

Comment 20

[Stuart Morgan]

Why is this 7 separate patches?

Comment 21

[froodian (Ian Leue)]

Attachment: Unified Patch

Because I didn't know about unified patches until I'd already uploaded all 7 of them, and I didn't want to generate any more traffic on this bug.  Here's a single patch making all the same changes.  Sorry about the mix-up; live and learn.

Comment 22

[Håkan Waara]

Comment on attachment 223069 [details] [diff] [review]
Unified Patch

I agree with the naming change. Now, you're changing all callers anyway, so perhaps you could make a change I've wanted to see for a while. Add a new method:

- (void)loadURL:(NSString*)aURLSpec;

That one would call:

 [self loadURL:aDomain referrer:nil focusContent:YES allowPopups:NO];

That is, with all the default values. Most callers seem to use those anyway. That way, all those callers that just want default options will be able to call loadURL:@"foo", and we won't have to worry about remembering to do the right thing in every place. It will also be much easier to debug later.

>@@ -1966,17 +1966,17 @@ enum BWCOpenDest {
>     if (loadInBackground)
>       [[controller window] orderWindow:NSWindowBelow relativeTo:[[self window] windowNumber]];
>     else
>       [controller showWindow:nil];
> 
>     if (canUseCache)
>       [[[controller getBrowserWrapper] getBrowserView] setPageDescriptor:desc displayType:nsIWebPageDescriptor::DISPLAY_AS_SOURCE];
>     else
>-      [controller loadURL:viewSource referrer:nil activate:!loadInBackground allowPopups:NO];
>+      [controller loadURL:viewSource referrer:nil focusContent:YES allowPopups:NO];

Do we want to focus content in background tabs?

>Index: browser/BrowserTabView.mm
>===================================================================
>RCS file: /cvsroot/mozilla/camino/src/browser/BrowserTabView.mm,v
>retrieving revision 1.34
>diff -u -8 -p -r1.34 BrowserTabView.mm
>--- browser/BrowserTabView.mm	10 Dec 2005 21:02:04 -0000	1.34
>+++ browser/BrowserTabView.mm	23 May 2006 18:27:24 -0000
>@@ -287,22 +287,22 @@ NSString* const kTabBarBackgroundDoubleC
>   else
>     [self showOrHideTabsAsAppropriate];
> }
> 
> - (BOOL)handleDropOnTab:(NSTabViewItem*)overTabViewItem overContent:(BOOL)overContentArea withURL:(NSString*)url
> {
>   if (overTabViewItem) 
>   {
>-    [[overTabViewItem view] loadURI: url referrer:nil flags: NSLoadFlagsNone activate:NO allowPopups:NO];
>+    [[overTabViewItem view] loadURI: url referrer:nil flags: NSLoadFlagsNone focusContent:YES allowPopups:NO];
>     return YES;

Same thing here, what happens if we open the dropped item in a background tab?

>   }
>   else if (overContentArea)
>   {
>-    [[[self selectedTabViewItem] view] loadURI: url referrer:nil flags: NSLoadFlagsNone activate:NO allowPopups:NO];
>+    [[[self selectedTabViewItem] view] loadURI: url referrer:nil flags: NSLoadFlagsNone focusContent:YES allowPopups:NO];
>     return YES;
>   }

and here.

Comment 23

[froodian (Ian Leue)]

Attachment: Implements loadURL:(NSString*)aURLSpec

I also replaced every time the default settings got called with the simplified loadURL:, and made an indentation fix.

(In reply to comment #22)
> Do we want to focus content in background tabs?
~snip
> Same thing here, what happens if we open the dropped item in a background tab?
~snip
> and here.

I tested these out pretty carefully, and nothing bad happens (it neither steals keyboard focus from the content pane nor the location bar, if it's in there), and like this it won't require changes once cl's patch to bug 337958 lands (which it otherwise would), so I left all three of those in.  The first one I replaced with the simplified loadURL:, and the other two are the same as they were.  If you're still convinced they should be elsewise I can change them, but I'm pretty confident that they're better that way.

Comment 24

[Chris Lawson (gone)]

Comment on attachment 223560 [details] [diff] [review]
Implements loadURL:(NSString*)aURLSpec

>Index: browser/BrowserWindowController.mm
>===================================================================

>-  [self loadURL:@"about:history" referrer:nil activate:YES allowPopups:YES];
>+  [self loadURL:@"about:history" referrer:nil focusContent:YES allowPopups:YES];

Under what circumstances would loading about:history ever pop up a window? I think we can change this to your new default call without consequences.

>Index: browser/BrowserTabView.mm
>===================================================================

>-    [[overTabViewItem view] loadURI: url referrer:nil flags: NSLoadFlagsNone activate:NO allowPopups:NO];
>+    [[overTabViewItem view] loadURI: url referrer:nil flags: NSLoadFlagsNone focusContent:YES allowPopups:NO];

While you're at it, wanna fix the spacing above? We seem to largely prefer no space after the parameter name and colon in this file. (Yes, this is inconsistent throughout the codebase too.)

>-    [[[self selectedTabViewItem] view] loadURI: url referrer:nil flags: NSLoadFlagsNone activate:NO allowPopups:NO];
>+    [[[self selectedTabViewItem] view] loadURI: url referrer:nil flags: NSLoadFlagsNone focusContent:YES allowPopups:NO];

See above.

r=me with those minor nits addressed.

cl

Comment 25

[Håkan Waara]

Comment on attachment 223560 [details] [diff] [review]
Implements loadURL:(NSString*)aURLSpec

>@@ -1967,17 +1967,17 @@ enum BWCOpenDest {
>     if (loadInBackground)
>       [[controller window] orderWindow:NSWindowBelow relativeTo:[[self window] windowNumber]];
>     else
>       [controller showWindow:nil];
> 
>     if (canUseCache)
>       [[[controller getBrowserWrapper] getBrowserView] setPageDescriptor:desc displayType:nsIWebPageDescriptor::DISPLAY_AS_SOURCE];
>     else
>-      [controller loadURL:viewSource referrer:nil activate:!loadInBackground allowPopups:NO];
>+      [controller loadURL:viewSource];
>   }
> }

I guess this is OK, because the code is opening a new window to view source.

> 
> - (IBAction)viewSource:(id)aSender
> {
>   BOOL loadInBackground = (([[NSApp currentEvent] modifierFlags] & NSShiftKeyMask) != 0);
>   NSString* urlStr = [[mBrowserView getBrowserView] getFocusedURLString];
>   [self loadSourceOfURL:urlStr inBackground:loadInBackground];
>@@ -2067,17 +2067,17 @@ enum BWCOpenDest {
>       
>       aDomain = [NSString stringWithUTF8String:spec.get()];
>       
>       if (inDest == kDestinationNewTab)
>         [self openNewTabWithURL:aDomain referrer:nil loadInBackground:inLoadInBG allowPopups:NO];
>       else if (inDest == kDestinationNewWindow)
>         [self openNewWindowWithURL:aDomain referrer:nil loadInBackground:inLoadInBG allowPopups:NO];
>       else // if it's not a new window or a new tab, load into the current view
>-        [self loadURL:aDomain referrer:nil activate:NO allowPopups:NO];
>+        [self loadURL:aDomain];
> 

This however, is quite bad. This would make the content steal focus from the search field, after you've hit enter.

>     } 
>   } else {
>     aURLSpec = [[[self getBrowserWrapper] getCurrentURI] UTF8String];
>     
>     // Get the domain so that we can replace %d in our searchURL
>     if (NS_NewURI(&aURI, aURLSpec, nsnull, nsnull) == NS_OK) {
>       nsCAutoString spec;
>@@ -2094,17 +2094,17 @@ enum BWCOpenDest {
>     [self transformFormatString:searchURL domain:aDomain search:escapedSearchString];
>     [escapedSearchString release];
>     
>     if (inDest == kDestinationNewTab)
>       [self openNewTabWithURL:searchURL referrer:nil loadInBackground:inLoadInBG allowPopups:NO];
>     else if (inDest == kDestinationNewWindow)
>       [self openNewWindowWithURL:searchURL referrer:nil loadInBackground:inLoadInBG allowPopups:NO];
>     else // if it's not a new window or a new tab, load into the current view
>-      [self loadURL:searchURL referrer:nil activate:NO allowPopups:NO];
>+      [self loadURL:searchURL];
>   }
> }

Same here, there's a good reason for not focusing there.

>@@ -3062,18 +3070,19 @@ enum BWCOpenDest {
>       tabViewItem = [self createNewTabItem];
>       [tabViewItem setLabel:NSLocalizedString(@"UntitledPageTitle", @"")];
>       [mTabBrowser addTabViewItem:tabViewItem];
>     }
>     
>     if (!tabViewToSelect)
>       tabViewToSelect = tabViewItem;
> 
>+    BOOL focusURLBar = [MainController isBlankURL:thisURL];
>     [[tabViewItem view] loadURI:thisURL referrer:nil
>-                          flags:NSLoadFlagsNone activate:(i == 0) allowPopups:inAllowPopups];
>+                          flags:NSLoadFlagsNone focusContent:!focusURLBar allowPopups:inAllowPopups];
>   }

This is inside the for-loop that opens a bunch of URLs (for example, tab groups), and makes sure that only the first opened tab gets focused, please don't change that.

Other than that (and Chris' comments), looks good.

Comment 26

[froodian (Ian Leue)]

Attachment: Addresses cl and hwaara's comments

Comment 27

[Chris Lawson (gone)]

(In reply to comment #25)
> (From update of attachment 223560 [details] [diff] [review] [edit])
> >       else // if it's not a new window or a new tab, load into the current view
> >-        [self loadURL:aDomain referrer:nil activate:NO allowPopups:NO];
> >+        [self loadURL:aDomain];
> > 
> 
> This however, is quite bad. This would make the content steal focus from the
> search field, after you've hit enter.

That's not bad at all. That's exactly what SHOULD happen when you hit return in the search field with no search string entered -- you should go to that search site, with focus placed in the content area. Try it; it really doesn't make sense to keep focus in the search field when loading in the current view.

> >     else // if it's not a new window or a new tab, load into the current view
> >-      [self loadURL:searchURL referrer:nil activate:NO allowPopups:NO];
> >+      [self loadURL:searchURL];
> >   }
> > }
> 
> Same here, there's a good reason for not focusing there.

See above. There's no reason whatsoever we shouldn't focus the search results page after a successful search.

I agree with Håkan's last comment, though. Trying to focus multiple content areas in background tabs is probably a bad idea ;)

cl

Comment 28

[Chris Lawson (gone)]

Oh, btw, I should probably point out that Camino 1.0.x's current behaviour is *not* to retain focus in the search field after loading a search engine or search results in the current view. I know what the code says to do, but I'm saying that its behaviour is not what you'd expect looking at the code, and that on further consideration, it didn't make sense for the code to be written that way in the first place.

cl

Comment 29

[froodian (Ian Leue)]

Another issue is when invoking searches from the context menu's "search" item.  with focusContent:NO (which is what currently happens, and what happens in my latest patch), searches invoked this way leave the focus in the search bar, which I think is kind of weird.  Also, when the NSSplitView is minimized, it causes some weirdness.  For those reasons and re: cl's comments, I'm thinking maybe the search cases should be focusContent:YES.  Thoughts?

Comment 30

[Håkan Waara]

Ok, I agree with you guys about the search focus thing. Let's get that change back in, and then I'll r+.

I think the lesson for all of us here is that one should look hard at every specific call when making mass-changes. :-)

The fact that the search field can get focus while collapsed is an issue I've been working on, but feel free to fix it if you find a good way.

Comment 31

[froodian (Ian Leue)]

Attachment: Last Patch before sr?

This puts the search focus back in. :)

Comment 32

[froodian (Ian Leue)]

Comment on attachment 223629 [details] [diff] [review]
Last Patch before sr?

Assuming that this has cl+ too. ;)

Comment 33

[Chris Lawson (gone)]

Comment on attachment 223629 [details] [diff] [review]
Last Patch before sr?

additional r=me

Comment 34

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Stuart, if you have a chance, could you look over this, too?  This sounds like it's potentially touching a lot of different focus/activate behaviors here, and you seem to know that stuff pretty well.

Comment 35

[Mike Pinkerton (not reading bugmail)]

i'm confused. you replaced all different param combinations with the same call:

+    [self loadURL:frameURL];

so we've lost any of the cases where activate was NO. is that what we really want here?

Comment 36

[froodian (Ian Leue)]

(In reply to comment #35)
> i'm confused. you replaced all different param combinations with the same call:
> 
> +    [self loadURL:frameURL];
> 
> so we've lost any of the cases where activate was NO. is that what we really
> want here?
> 

the line that replaced was 
-    [self loadURL:frameURL referrer:nil activate:YES allowPopups:NO];

How were there cases when activate was NO?

Comment 37

[Mike Pinkerton (not reading bugmail)]

that's one example, you replaced like 20 of those. some of those passed NO (or could have because it was a variable).

Comment 38

[froodian (Ian Leue)]

every simple loadURL: call explicitly passed |referrer:nil activate:YES allowPopups:YES| except for:

> [controller loadURL:viewSource referrer:nil activate:!loadInBackground allowPopups:NO];

Where it doesn't matter because the only way to get to the content is by switching tabs, which focuses the content anyway (discussed in comment 22 and comment 23), and

> [self loadURL:aDomain referrer:nil activate:NO allowPopups:NO];
and
> [self loadURL:searchURL referrer:nil activate:NO allowPopups:NO];

from (IBAction)viewSource, which also doesn't matter because there's no chrome, so of course the content is focused.

If you want me to change these back, I'm happy to, but I changed them because a) although they don't make a difference, we *would* want the content focused were there to be a choice and b) the simple loadURL is more compact and looks nicer.

Comment 39

[froodian (Ian Leue)]

(In reply to comment #38)
> every simple loadURL: call explicitly passed |referrer:nil activate:YES
> allowPopups:YES|

Dur.  Sorry, I mean |allowPopups:NO|, except for loading about:history (see comment 24)

Comment 40

[froodian (Ian Leue)]

Attachment: Unbitrots it

This patch unbitrots the previous one.  It touches all the high-traffic files, and took a long time to get back up to date, so lets try to get this reviewed and landed as quickly as possible. :)

Comment 41

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 227974 [details] [diff] [review]
Unbitrots it

sr=pink

Comment 42

[Nick Kreeger]

Fixed trunk and branch.