bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

View Source window accepts commands it shouldn't

RESOLVED FIXED

Status

Camino Graveyard
Toolbars & Menus
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Stuart Morgan, Assigned: Chris Lawson (gone))

Tracking

({fixed1.8.1.10})

Trunk
PowerPC
Mac OS X
fixed1.8.1.10

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

12 years ago
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.
PC running 10.3???  What sort of hardware did you walk off with ;)
Blocks: 341853
Hardware: PC → Macintosh
(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.
... but of course you already know that because you commented on that bug right after your comment on this one. Nevermind! :)
(Assignee)

Comment 4

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

Comment 5

11 years ago
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
Status: NEW → ASSIGNED
Target Milestone: --- → Camino1.6
Assignee: nobody → cl-bugs
Status: ASSIGNED → NEW
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
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*.
Target Milestone: Camino1.6 → ---
(Assignee)

Comment 7

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

Comment 8

11 years ago
Created attachment 281420 [details] [diff] [review]
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.
(Assignee)

Comment 9

11 years ago
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.
Attachment #281420 - Flags: review?

Comment 10

11 years ago
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.
Attachment #281420 - Flags: superreview?(mikepinkerton)
Attachment #281420 - Flags: review?
Attachment #281420 - Flags: review+
(Assignee)

Comment 11

11 years ago
(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 on attachment 281420 [details] [diff] [review]
fix

sr=pink
Attachment #281420 - Flags: superreview?(mikepinkerton) → superreview+
Created attachment 287284 [details] [diff] [review]
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.
Attachment #281420 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1.10
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.