339545 - Cmd-L fails to focus frontmost window from popup and view source windows

Status: VERIFIED WONTFIX

(Camino Graveyard :: Accessibility, defect)

Description

[froodian (Ian Leue)]

When cmd-L ing from View source and popup windows, the URL bar of the window one window behind the frontmost gets focused, and the window is brought to front.  This occurs even if the toolbar has been manually unminimized. STR:

1. View source in new window (or trigger popup window)
2. Unminimize the toolbar
3. Cmd-L

What happens: The URL bar of window behind the view source (or popup) window is focused.
What should happen: The URL bar of the view source (or popup) should be focused.

Comment 1

[Chris Lawson (gone)]

Attachment: fix

Anyone else who wants to have a look at this patch, feel free to test it and comment.

Comment 2

[Stuart Morgan]

Comment on attachment 223653 [details] [diff] [review]
fix

>+    if ((([curWindow isVisible] || [curWindow isMiniaturized] || [NSApp isHidden]) &&
>+          [[curWindow windowController] isMemberOfClass:[BrowserWindowController class]] &&
>+          [[curWindow windowController] hasFullBrowserChrome]) ||
>+          ([[curWindow windowController] isMemberOfClass:[BrowserWindowController class]] && [[curWindow toolbar] isVisible]))
>       [windowArray addObject:curWindow];
>-    }

You've reduced the readability of the if--there shouldn't be a straight line down the left size, since indentation should indicate parenthesization. This version looks like a lot of clauses at the same parenthesization level, but wrappen in ((( ))) for some reason, which isn't the case.  The correct format for your logic is:
if (((...) &&
     [...] &&
     [...]) ||
    ([...] &&
     [...]))
{
  ...
}

(Having the |{| on a new line is, by the way, correct for |if| statements with multi-line clauses--and should be included even if there is only one line of code in the body of the |if|--since it makes the transition point from clauses to code more obvious.)

In that format it's more clear that the new logic is wrong; it would incorrectly return hidden windows so long as they had toolbars.  It should be:

    if (([curWindow isVisible] || [curWindow isMiniaturized] || [NSApp isHidden]) &&
        [[curWindow windowController] isMemberOfClass:[BrowserWindowController class]] &&
        ([[curWindow windowController] hasFullBrowserChrome] ||
         [[curWindow toolbar] isVisible]))


However, the much bigger question is: is that really desirable behavior.  I'm not at all convinced that someone who shows the toolbar on a popup really wants, e.g., subsequent windows to take size and cascade location cues from that popup.

I don't think showing the toolbar should promote a window to a fully blessed chromed window.  I think an openLocation:-specific solution would probably be better here.  (And if something like this change is desirable, I think a better way to do it would be to make having the toolbar showing override/substitute for only the toolbar portion of the chrome flag, so that a window that isn't resizable or has no status bar still wouldn't count even with the toolbar showing.)

Comment 3

[Chris Lawson (gone)]

(In reply to comment #2)

> I don't think showing the toolbar should promote a window to a fully blessed
> chromed window.  I think an openLocation:-specific solution would probably be
> better here.  (And if something like this change is desirable, I think a better
> way to do it would be to make having the toolbar showing override/substitute
> for only the toolbar portion of the chrome flag, so that a window that isn't
> resizable or has no status bar still wouldn't count even with the toolbar
> showing.)

How does that affect people who have hidden (through whatever unsupported hacks) the status bar (and thus the resize handle, IIRC)?

cl

Comment 4

[Stuart Morgan]

I meant windows without the resizable and/or status bar chrome flags; those aren't affected by nib hackery.

Comment 5

[Chris Lawson (gone)]

Attachment: Fixed to use openLocation: instead

This one removes the getFrontmostBrowserWindow: call from openLocation: and replaces it with the logic I had originally intended for gFBW.

cl

Comment 6

[Stuart Morgan]

Comment on attachment 223712 [details] [diff] [review]
Fixed to use openLocation: instead

The more I think about this bug, the less I like it. Using the toolbar's visibility as an indicator seems sketchy, since some people browse without toolbars--If all my windows look like the view source window because I don't use the toolbar, then why should I have to show the toolbar to make it behave the same?

I think that allowing people to show the toolbar on non-toolbar-chromed windows opened up a can of worms (even though it is a useful ability).  An openLocation: hack is actually going to make things even more confusing (sorry for not having thought it through fully before suggesting it) since if a user is (apparently) promoting a window to full browser window status, it will be odd when nothing else based on browser windows works for that window.

So there are three things that could happen here, from my perspective:
1. The definition of a browser window could be redifined in some fashion. (Any resizable window? Any window where the user manually went to another location?)
2. View-source windows could be made to be fully-chromed windows that happen to have a hidden toolbar, sidestepping the lager issue while making the change requested here.
3. This bug could be WONTFIXed, since it is correct behavior. Command-L is *not* a shortcut that means "focus the current window's URL bar", it's a shortcut that means "Let me enter a location to go to in the front-most browser window".

Anything involving 1. would have to be chosen very carefully, in order to prevent making the common popup case worse by default.  Between 2 and 3, the question is, is the view-source window fundamentally a utility window that happens to be implemented as a browser window (3, or perhaps a new option: 4. change the implementation of view-source windows) or fundamentally a browser window that happens to be showing source (2).

Comment 7

[Stuart Morgan]

Oops, re-reading I see that this isn't just about view-source, but popups as well.  So that leaves 1 or 3.

Comment 8

[Chris Lawson (gone)]

Well, what we *really* need is a new window for view-source, like what Firefox has. The thing I really like about having view-source implemented as a browser window, though, is it allows you to change the URL source you're viewing without going back to the browser, going to another page, and then hitting view-source again.

I suppose we could special-case the view-source window and only treat *that* as a browser window if it has a visible toolbar. Would that be satisfactory?

I agree that this isn't all that important for popups; I'm much more likely to use a view-source window for further browsing (as noted above) than I am to use a popup window for further browsing.

cl

Comment 9

[Mike Pinkerton (not reading bugmail)]

a surprising number of people browse in windows with the toolbar hidden and rely on the location sheet. I dunno why, but they do. 

i sort of agree with (3) (use a real browser window, not a popup) but i don't want to break the ability people have to use their current browser window w/out a toolbar visible. 

can we have both?

Comment 10

[Chris Lawson (gone)]

(In reply to comment #9)
> a surprising number of people browse in windows with the toolbar hidden and
> rely on the location sheet. I dunno why, but they do. 
> 
> i sort of agree with (3) (use a real browser window, not a popup) but i don't
> want to break the ability people have to use their current browser window w/out
> a toolbar visible. 
> 
> can we have both?

I think the only real way to get both is to do what I asked about in comment 8. Namely, we could special-case the view-source window and only treat *that* as
a browser window if it has a visible toolbar.

cl

Comment 11

[Stuart Morgan]

I'm just going to WONTFIX this. Source windows aren't browser windows, so browsing related menu commands shouldn't apply to them (and the others that do are bugs). Things are messy with the ability to show the toolbar, but I don't think we should make it worse; I'm coming to the opinion that we should remove that ability again, and address the issue of wanting to be able to get a URL with a context menu or something.

Comment 12

[Chris Lawson (gone)]

Verified per comment 11. At some point we should probably discuss this again and come to an agreement on whether to continue allowing the toolbar to be toggled in such windows (as well as whether to create a new window class for view-source windows to solve some other associated problems).