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.

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

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.

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

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.

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

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

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

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

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

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

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.

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.

Why is this 7 separate patches?

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

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

(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

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

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?

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.

This puts the search focus back in. :)

Comment on attachment 223629 [details] [diff] [review] Last Patch before sr? Assuming that this has cl+ too. ;)

Comment on attachment 223629 [details] [diff] [review] Last Patch before sr? additional r=me

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.

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?

(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?

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

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.

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

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 on attachment 227974 [details] [diff] [review] Unbitrots it sr=pink

Fixed trunk and branch.