353265 - View Source window accepts commands it shouldn't

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, defect)

Description

[Stuart Morgan]

At least two menu commands are being applied to view source when they should go to the frontmost browser window:
- Open File…
- New Tab
We should find out if popups are also affected.

Comment 1

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

PC running 10.3???  What sort of hardware did you walk off with ;)

Comment 2

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #1)
> PC running 10.3???  What sort of hardware did you walk off with ;)

The "Hardware" field's default for bugs filed with Intel Macs is "PC", see bug 348770.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

... but of course you already know that because you commented on that bug right after your comment on this one. Nevermind! :)

Comment 4

[Chris Lawson (gone)]

(In reply to comment #0)
> At least two menu commands are being applied to view source when they should go
> to the frontmost browser window:
> - Open File…
> - New Tab
> We should find out if popups are also affected.

Using the popup generated by clicking the link on 

http://popuptest.com/popuptest4.html

I can confirm that both the aforementioned commands are applied to the popup rather than the frontmost browser window.

Might I suggest, however, that it would be awfully damn weird for either of those to apply to what appears (for all intents and purposes) to be a background window, and that a more logical course of action might be to simply disable these two menu commands when a window other than a browser window is frontmost?

cl

Comment 5

[Chris Lawson (gone)]

I'll take this for 1.6, and unless anyone has objections, I'm going to do what I suggested in comment 4; namely, I plan to disable these two menu items when (frontmost window) != [self getFrontmostBrowserWindow].

cl

Comment 6

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

Mass un-setting milestone per 1.6 roadmap.

Filter on RemoveRedonkulousBuglist to remove bugspam.

Developers: if you have a patch in hand for one of these bugs, you may pull the bug back to 1.6 *at that point*.

Comment 7

[Chris Lawson (gone)]

I don't think we need to worry about Open File until some sort of decision is reached on bug 361157.

I have a patch for the New Tab case ready to go, but I'd like to discuss the other bug first so I can submit a combined patch for both Open File and New Tab.

Comment 8

[Chris Lawson (gone)]

Attachment: fix

Fixes both cases as applicable to our current behaviour. If bug 361157 is fixed to match Safari's behaviour (always open new window for Open File regardless), then we won't need to validate the |openFile:| selector any more and that block can be removed.

Comment 9

[Chris Lawson (gone)]

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

Oh, and this removes |isMainWindowABrowserWindow|, which wasn't being used anywhere and had a totally different definition of "browser window" than |getFrontmostBrowserWindow|, despite a comment in the code implying otherwise.

Comment 10

[Jeff Dlouhy]

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


>-    return (browserController || ![NSApp mainWindow]);
>+    return (((browserController && ([NSApp mainWindow] == [self getFrontmostBrowserWindow])) ||
>+            ![NSApp mainWindow]);

You are missing a parenthesis right before the ;

So, change ![NSApp mainWindow]); to ![NSApp mainWindow]));

r=me with that change.

Comment 11

[Chris Lawson (gone)]

(In reply to comment #10)
> (From update of attachment 281420 [details] [diff] [review])

> You are missing a parenthesis right before the ;

So I am. Thanks! I'll wait to respin until pink comments for sr, or someone can fix the chicken.

cl

Comment 12

[Mike Pinkerton (not reading bugmail)]

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

sr=pink

Comment 13

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

Attachment: patch as checked in

Landed on the trunk and MOZILLA_1_8_BRANCH.  Someone should sanity-check this due to the bitrot from de-get-ification; Chris and I talked through it, and it built and ran without any noticeable issues.