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

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
General
--
minor
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Gary Yuen, Assigned: froodian (Ian Leue))

Tracking

({fixed1.8.1})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1
Dependency tree / graph

Details

Attachments

(1 attachment, 12 obsolete attachments)

25.07 KB, patch
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
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.
Version: unspecified → Trunk
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.)

Comment 4

12 years ago
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.

Updated

12 years ago
Severity: normal → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: Trunk → unspecified
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)....
Summary: Open Link in New Window - new windows open with Address bar focused → Open Link in New Window - new background windows open with Address bar focused
Keywords: qawanted
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).
Keywords: qawanted
Target Milestone: --- → Camino1.1
*** Bug 316395 has been marked as a duplicate of this bug. ***
Blocks: 185385
(Assignee)

Comment 8

11 years ago
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
Status: NEW → ASSIGNED
Assignee: mikepinkerton → stridey
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
(Assignee)

Comment 9

11 years ago
Created attachment 222573 [details] [diff] [review]
Makes new windows get focus irregardles of the load in bg setting
Attachment #222573 - Flags: review?
(Assignee)

Comment 10

11 years ago
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.
(Assignee)

Updated

11 years ago
Attachment #222573 - Flags: review? → review?(hwaara)

Comment 11

11 years ago
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.
Attachment #222573 - Flags: review?(hwaara) → review-
(Assignee)

Comment 12

11 years ago
> * 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.
(Assignee)

Comment 13

11 years ago
Created attachment 222831 [details] [diff] [review]
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.
Attachment #222573 - Attachment is obsolete: true
Attachment #222831 - Flags: review?(hwaara)
(Assignee)

Comment 14

11 years ago
Created attachment 222833 [details] [diff] [review]
New BrowserTabView.mm
Attachment #222833 - Flags: review?(hwaara)
(Assignee)

Comment 15

11 years ago
Created attachment 222834 [details] [diff] [review]
New BrowserWindowController.h
Attachment #222834 - Flags: review?(hwaara)
(Assignee)

Comment 16

11 years ago
Created attachment 222835 [details] [diff] [review]
New BrowserWrapper.mm
Attachment #222835 - Flags: review?(hwaara)
(Assignee)

Comment 17

11 years ago
Created attachment 222836 [details] [diff] [review]
New BrowserWrapper.h
Attachment #222836 - Flags: review?(hwaara)
(Assignee)

Comment 18

11 years ago
Created attachment 222837 [details] [diff] [review]
New GoMenu.mm
Attachment #222837 - Flags: review?(hwaara)
(Assignee)

Comment 19

11 years ago
Created attachment 222839 [details] [diff] [review]
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.
Attachment #222839 - Flags: review?(hwaara)
(Assignee)

Updated

11 years ago
Blocks: 299548

Comment 20

11 years ago
Why is this 7 separate patches?
(Assignee)

Comment 21

11 years ago
Created attachment 223069 [details] [diff] [review]
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.
Attachment #222831 - Attachment is obsolete: true
Attachment #222833 - Attachment is obsolete: true
Attachment #222834 - Attachment is obsolete: true
Attachment #222835 - Attachment is obsolete: true
Attachment #222836 - Attachment is obsolete: true
Attachment #222837 - Attachment is obsolete: true
Attachment #222839 - Attachment is obsolete: true
Attachment #223069 - Flags: review?(hwaara)
Attachment #222831 - Flags: review?(hwaara)
Attachment #222833 - Flags: review?(hwaara)
Attachment #222834 - Flags: review?(hwaara)
Attachment #222835 - Flags: review?(hwaara)
Attachment #222836 - Flags: review?(hwaara)
Attachment #222837 - Flags: review?(hwaara)
Attachment #222839 - Flags: review?(hwaara)

Comment 22

11 years ago
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.
Attachment #223069 - Flags: review?(hwaara) → review-
(Assignee)

Comment 23

11 years ago
Created attachment 223560 [details] [diff] [review]
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.
Attachment #223069 - Attachment is obsolete: true
Attachment #223560 - Flags: review?(hwaara)

Comment 24

11 years ago
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

11 years ago
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.
Attachment #223560 - Flags: review?(hwaara) → review-
(Assignee)

Comment 26

11 years ago
Created attachment 223615 [details] [diff] [review]
Addresses cl and hwaara's comments
Attachment #223560 - Attachment is obsolete: true
Attachment #223615 - Flags: review?(hwaara)

Comment 27

11 years ago
(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

11 years ago
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
(Assignee)

Comment 29

11 years ago
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

11 years ago
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.
(Assignee)

Comment 31

11 years ago
Created attachment 223629 [details] [diff] [review]
Last Patch before sr?

This puts the search focus back in. :)
Attachment #223615 - Attachment is obsolete: true
Attachment #223629 - Flags: review?(hwaara)
Attachment #223615 - Flags: review?(hwaara)

Updated

11 years ago
Attachment #223629 - Flags: review?(hwaara) → review+
(Assignee)

Comment 32

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

Assuming that this has cl+ too. ;)
Attachment #223629 - Flags: superreview?(mikepinkerton)

Comment 33

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

additional r=me
Attachment #223629 - Flags: review+
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.
QA Contact: general
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?
(Assignee)

Comment 36

11 years ago
(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).
(Assignee)

Comment 38

11 years ago
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.
(Assignee)

Comment 39

11 years ago
(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)
(Assignee)

Comment 40

11 years ago
Created attachment 227974 [details] [diff] [review]
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. :)
Attachment #223629 - Attachment is obsolete: true
Attachment #227974 - Flags: superreview?(mikepinkerton)
Attachment #223629 - Flags: superreview?(mikepinkerton)
Comment on attachment 227974 [details] [diff] [review]
Unbitrots it

sr=pink
Attachment #227974 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Updated

11 years ago
Whiteboard: [needs checkin]

Comment 42

11 years ago
Fixed trunk and branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [needs checkin]
You need to log in before you can comment on or make changes to this bug.