Closed
Bug 417733
Opened 17 years ago
Closed 15 years ago
About box should be always-on-top on Windows and Linux
Categories
(Firefox :: General, defect)
Tracking
()
RESOLVED
FIXED
Firefox 3.7a3
People
(Reporter: zeniko, Assigned: dao)
References
Details
(Keywords: regression)
Attachments
(1 file, 6 obsolete files)
1.59 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
(From bug 275753 comment #6)
> After a lot of looking, I finally found a single non-modal About on Windows -
> the supremely non-native Safari.
Or if we really want to facilitate copying the user agent into a web form, we should make it always-on-top so that the About box doesn't remain hanging around uselessly in the background...
Comment 1•17 years ago
|
||
How can one make a XUL window always-on-top?
Reporter | ||
Comment 2•17 years ago
|
||
> window.openDialog("chrome://browser/content/aboutDialog.xul", "About", "centerscreen,dependent,chrome,resizable=no");
IIRC "dependent" doesn't work on all platforms, but at least on Windows it does.
Besides: You'd also have to make sure that on repeatedly selecting Help -> About, the same dialog gets focused again (which currently doesn't work, either). Cf. e.g. toOpenWindowByType
Comment 3•17 years ago
|
||
This patch implements "always on top" for Windows. With this patch, About dialogs on Windows will open dependent on their opener window. When re-selecting Help -> About, if a dependent window on the current window is already active, then that window gets focused, otherwise a new window is opened. This makes sure that users will see the About window no matter from which window they want to open it.
On Linux, this patch creates a modeless dialog, and makes sure it's focused when re-selecting Help -> About. On Mac, the behavior is not changed by this patch.
Asking gavin for review, because he already reviewed bug 275753. Asking Beltzner for ui-r to make sure the changes in the interaction here are OK with him.
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Attachment #304968 -
Flags: ui-review?(beltzner)
Attachment #304968 -
Flags: review?(gavin.sharp)
Reporter | ||
Comment 4•17 years ago
|
||
Requesting blocking because:
1. Help -> About Minefield
2. Move the browser window to the foreground so that it covers the about dialog
3. Help -> About Minefield
Actual result:
Noting happens after step 3, making the About command look broken.
Expected result:
The About box should at least have been re-focused. Of course, IMO step 2 never should have been possible in the first place...
Flags: blocking-firefox3?
Keywords: regression
Updated•17 years ago
|
Attachment #304968 -
Flags: ui-review?(beltzner) → ui-review+
Updated•17 years ago
|
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
Comment 5•17 years ago
|
||
Gavin: could you review this please, so that we can have a chance to land this for Firefox 3? This has ui-r+, and should be easy to review.
Attachment #317967 -
Flags: review?(gavin.sharp)
Updated•17 years ago
|
Attachment #304968 -
Attachment is obsolete: true
Attachment #304968 -
Flags: review?(gavin.sharp)
Updated•17 years ago
|
Whiteboard: [has patch] [needs review gavin]
Updated•17 years ago
|
Whiteboard: [has patch] [needs review gavin] → [has patch] [needs review gavin][RC2?]
Comment 6•17 years ago
|
||
This is not critical enough for RC2 - must wait for a later release.
Whiteboard: [has patch] [needs review gavin][RC2?] → [has patch] [needs review gavin][RC2-]
Comment 7•17 years ago
|
||
Just poking this bug - if this is to be considered for the branch release, it'll need a review and then approval.
Updated•17 years ago
|
Flags: wanted-firefox3.1?
Comment 8•16 years ago
|
||
Gavin: ping...
Updated•16 years ago
|
Attachment #317967 -
Flags: review?(gavin.sharp) → review?(mconnor)
Updated•16 years ago
|
Whiteboard: [has patch] [needs review gavin][RC2-] → [has patch] [needs review mconnor][RC2-]
Comment 9•16 years ago
|
||
Comment on attachment 317967 [details] [diff] [review]
Patch (v1) (unbitrotted)
>Index: browser/base/content/utilityOverlay.js
> function openAboutDialog()
>+#ifdef XP_WIN
>+ var enumerator = wm.getEnumerator("Browser:About");
>+ var found = false;
>+ while (enumerator.hasMoreElements()) {
>+ var win = enumerator.getNext();
>+ if (win && win.opener == window) {
Why would "win" be null?
>+ win.focus();
>+ found = true;
>+ break;
>+ }
>+ }
>+ if (!found)
>+ window.openDialog("chrome://browser/content/aboutDialog.xul", "", "centerscreen,chrome,dependent=yes,resizable=no");
Can't you just return early and avoid the need for "found"?
Seems like this method would be easier to read if the openDialog calls were outside the ifdefs (with an ifdeffed "features" local variable to account for those differences).
Attachment #317967 -
Flags: review?(mconnor)
Updated•16 years ago
|
Whiteboard: [has patch] [needs review mconnor][RC2-] → [has patch]
Comment 10•16 years ago
|
||
Does this patch look better?
Attachment #317967 -
Attachment is obsolete: true
Attachment #364762 -
Flags: review?(gavin.sharp)
Updated•16 years ago
|
Attachment #364762 -
Attachment is patch: true
Attachment #364762 -
Attachment mime type: application/octet-stream → text/plain
Updated•16 years ago
|
Whiteboard: [has patch] → [has patch][needs review gavin]
Comment 11•16 years ago
|
||
I took a shot at just rewriting the function, since that was easier than explaining what I thought should be done. What do you think?
Comment 12•16 years ago
|
||
Two points:
1. Is it OK to specify "_blank" for all platforms as the target?
2. With this function, the features have slightly changed. On Windows and Linux, resizable=no is not specified (I'm not sure but I think it's the default for a dialog).
So, I take it that this patch has your r+, right? ;-)
Comment 13•16 years ago
|
||
(In reply to comment #12)
> 1. Is it OK to specify "_blank" for all platforms as the target?
Actually, I suppose we could use a unique name ("FirefoxAboutDialog"?), and then get rid of the non-Windows focusing code in favor of window.open's targeting behavior. Need to test that that works, though...
> 2. With this function, the features have slightly changed. On Windows and
> Linux, resizable=no is not specified (I'm not sure but I think it's the default
> for a dialog).
Right, dialogs are not resizable.
Comment 14•16 years ago
|
||
Actually, I wonder whether we need the two window focusing paths at all... if we're OK with focusing other existing windows on Linux/Mac, why not do it for Windows as well? That the dialogs are dependent doesn't really change the behavior that much, does it?
Comment 15•16 years ago
|
||
(In reply to comment #14)
> Actually, I wonder whether we need the two window focusing paths at all... if
> we're OK with focusing other existing windows on Linux/Mac, why not do it for
> Windows as well? That the dialogs are dependent doesn't really change the
> behavior that much, does it?
Well, I think it does, because it will bring the other window who has originally opened the About dialog to front, which would defeat the purpose of the users who want to do something in the About window related to their current open page (such as the case in comment 0).
Comment 16•16 years ago
|
||
Nothing stops them from switching back to that window, right? In the common case, people wanting to copy/paste the UA string aren't going to have the dialog open in background windows.
Comment 17•16 years ago
|
||
(In reply to comment #14)
> Actually, I wonder whether we need the two window focusing paths at all... if
> we're OK with focusing other existing windows on Linux/Mac, why not do it for
> Windows as well? That the dialogs are dependent doesn't really change the
> behavior that much, does it?
I tested it under Windows, and it doesn't work (maybe because the dialog is set to dependent?
Are you fine with a patch which keeps the windows focusing path and removes the unix one in favor of window.open targeting?
Comment 19•16 years ago
|
||
Do we have any further progress on this bug?
Comment 21•16 years ago
|
||
Comment on attachment 364762 [details] [diff] [review]
Patch (v2)
(In reply to comment #19)
> Do we have any further progress on this bug?
Dao, can you please review this? It's been waiting for a review for ages now (it's actually missed two releases!).
Attachment #364762 -
Flags: review?(gavin.sharp) → review?(dao)
Updated•16 years ago
|
Whiteboard: [has patch][needs review gavin] → [has patch][needs review dao]
Assignee | ||
Comment 22•16 years ago
|
||
Why exactly is Windows special-cased here? The subtle platform-specific differences seem arbitrary to me.
Btw, as the patch doesn't make the window modal, please update the summary accordingly?
Assignee | ||
Updated•16 years ago
|
Attachment #364762 -
Flags: review?(dao) → review-
Assignee | ||
Comment 23•16 years ago
|
||
Comment on attachment 364762 [details] [diff] [review]
Patch (v2)
this looks overly complex
Assignee | ||
Comment 24•16 years ago
|
||
Assignee | ||
Updated•16 years ago
|
Attachment #386499 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Comment 25•16 years ago
|
||
(In reply to comment #13)
> Actually, I suppose we could use a unique name ("FirefoxAboutDialog"?), and
> then get rid of the non-Windows focusing code in favor of window.open's
> targeting behavior. Need to test that that works, though...
That has the disadvantage of reloading the window.
Comment 26•15 years ago
|
||
(In reply to comment #22)
> Why exactly is Windows special-cased here? The subtle platform-specific
> differences seem arbitrary to me.
See comment 0.
> Btw, as the patch doesn't make the window modal, please update the summary
> accordingly?
Done.
Flags: wanted-firefox3.5?
Summary: About box should remain modal on Windows → About box should be always-on-top on Windows
Assignee | ||
Comment 27•15 years ago
|
||
Comment 0 doesn't tell me why it shouldn't be always-on-top on Linux and OS X, for instance.
Comment 28•15 years ago
|
||
(In reply to comment #27)
> Comment 0 doesn't tell me why it shouldn't be always-on-top on Linux and OS X,
> for instance.
I don't know about OS X, but on Linux native about dialogs are usually a top-level window...
The major problem I tried to address in bug 275753 was that about dialog being modal makes it impossible to copy and paste the user-agent which is displayed in that window unless the About dialog is closed and re-opened later. But it seems like a UI decision needs to be made here, and once we have a UI decision, we can then work on the implementation.
Alex, what do you think?
Keywords: uiwanted
Comment 29•15 years ago
|
||
Perhaps it's been too long since I've used windows, but is there such a thing as an always on top, but still non-modal window? If so, then this change sounds fine.
In general fully modal windows should be avoided unless you absolutely have to get input from the user before unlocking the application (which in itself should be avoided).
(From bug 275753 comment #6)
> After a lot of looking, I finally found a single non-modal About on Windows -
> the supremely non-native Safari.
I'm all for trying to be as native as possible, but I think making the about window totally modal one of the obscure cases where being completely native means creating a slightly worse interface, so we should intentionally go non-native.
Assignee | ||
Comment 30•15 years ago
|
||
Alex: Is always-on-top-but-not-modal wanted for Linux and/or OS X?
Comment 31•15 years ago
|
||
As far as I can tell the always on top approach isn't common on OS X. Unfortunately I don't know what behavior is native on Linux.
Comment 32•15 years ago
|
||
Personally, I'm really surprised that you guys wouldn't want your application to have a modal About box like virtually every other Windows application, but nontheless, please ... for the love of God ... don't make the window "always-on-top". I absolutely can't stand when I see windows using that attribute. Using that attribute gives off a very "arrogant" feel you know? It kind of says, "Look at me. I'm more important than anything else on your desktop." With that attribute "no" other window can hide that window. So if you switch to another application ... like Word or whatever, that About box will be covering up your other work.
Let me make one last plea for the modal aspect -- I saw up above somewhere someone mention that the About box can't be modal because one would want to copy or drag the User Agent string into a web form or something. But couldn't one just copy the User Agent string on to the clipboard, close the dialog (because it's not needed anymore) and then paste the text into wherever you want? That's the beauty of the clipboard.
But even if the above paragraph didn't sway you guys for some reason, at least do something similar to the dialog which comes up in say Windows Notepad when you click Edit-->Find. In that situation the "Find" dialog box isn't "modal" (i.e. you can still keep using typing away in notepad) but the "Find" dialog box doesn't go hiding into the background (like Firefox's current About box). I believe Notepad's Find window is merely a modeless dialog using the main window as its parent and owner.
Anyway, just, please, whatever you do ... don't make the window "always-on-top" where the About box window would actually be covering each and every other window on one's desktop.
Oh! I just took a quick peek at the About window with Spy++. You guys made the window a top-level window for some reason. Why not just give the About box window a parent -- say the main application window -- and be done with this bug already.
Anyway, that's my two cents ... ;-)
Cheers,
Alan
Comment 33•15 years ago
|
||
(In reply to comment #32)
> nontheless, please ... for the love of God ... don't make the window
> "always-on-top". I absolutely can't stand when I see windows using that
> attribute. Using that attribute gives off a very "arrogant" feel you know? It
> kind of says, "Look at me. I'm more important than anything else on your
> desktop." With that attribute "no" other window can hide that window.
No one was suggesting making the window "always-on-top" at the OS level. The options being discussed were "always-on-top [the parent window]" (i.e. "dependent" as described here: https://developer.mozilla.org/en/DOM:window.open ) and "modal dialog".
Comment 34•15 years ago
|
||
(In reply to comment #29)
> Perhaps it's been too long since I've used windows, but is there such a thing
> as an always on top, but still non-modal window? If so, then this change
> sounds fine.
Yes, it's exactly what the "dependent" windows in Mozilla look like. A good example suggested by Alan is the Find dialog in Notepad. But please note that all "About" dialogs for native Windows apps are modal. So, the native Windows style for About dialogs is modal.
> In general fully modal windows should be avoided unless you absolutely have to
> get input from the user before unlocking the application (which in itself
> should be avoided).
This is clearly not the case for About dialogs.
> (From bug 275753 comment #6)
> > After a lot of looking, I finally found a single non-modal About on Windows -
> > the supremely non-native Safari.
>
> I'm all for trying to be as native as possible, but I think making the about
> window totally modal one of the obscure cases where being completely native
> means creating a slightly worse interface, so we should intentionally go
> non-native.
So, I take it to mean that the "dependent" style on Windows is OK.
(In reply to comment #30)
> Alex: Is always-on-top-but-not-modal wanted for Linux and/or OS X?
I was under the impression that on Linux, About dialogs were (used to be?) top-level windows like the current About dialog for Firefox, but after testing several native Gnome and KDE apps right now, they all use the always-on-top-but-not-modal (i.e., "dependent") style, so I guess the "dependent" style is also wanted on Linux.
This only leaves OS X. What's the native style for About dialogs on OS X?
Assignee | ||
Comment 35•15 years ago
|
||
Alex wrote:
(In reply to comment #31)
> As far as I can tell the always on top approach isn't common on OS X.
Keywords: uiwanted
OS: Windows XP → All
Summary: About box should be always-on-top on Windows → About box should be always-on-top on Windows and Linux
Whiteboard: [has patch][needs review dao]
Comment 36•15 years ago
|
||
(In reply to comment #35)
> Alex wrote:
>
> (In reply to comment #31)
> > As far as I can tell the always on top approach isn't common on OS X.
Yes, I was asking what the native behavior is. Is it modal dialogs? Or top-level windows? This might be too obvious, but unfortunately I have never used OS X :-(
Comment 37•15 years ago
|
||
(In reply to comment #33)
> No one was suggesting making the window "always-on-top" at the OS level. The
> options being discussed were "always-on-top [the parent window]" (i.e.
> "dependent" as described here: https://developer.mozilla.org/en/DOM:window.open
> ) and "modal dialog".
Gavin, sorry, then I misunderstood. This seems to be a case of coming from two different backgrounds. Being a hard-core windows developer for many years I've never heard of the "always-on-top" style used for anything other than for windows to always be displayed on top of all other windows on the system. And when Alex mentioned that it's been a very long time since he used windows and asked if there was such a thing as always on top I just assumed he meant the true "always on top" style at the OS-level because that is the only situation I've ever heard that term used in.
In any case, glad to hear that we're all on the same page. Oh, also just for reference, on Windows, the always-on-top at the OS-level uses the WS_EX_TOPMOST window style or DS_SYSMODAL for dialogs.
Also, I'm not quite sure why you pointed to the window.open() DOM method as I thought those methods were only used on web pages, but then again I'm not at all yet familiar with the architecture of how Firefox's user interface is constucted. I'll have to look into that when I get a chance. Perhaps the Firefox user interface is itself constructed as web pages that you mentioned DOM methods? I don't know, I tend to think in terms of lower-level closer-to-the-OS techniques.
In any case I'm glad you guys weren't talking about making that dialog always-on-top at-the-OS-level. :-)
Cheers,
Alan
Comment 38•15 years ago
|
||
(In reply to comment #37)
> I'll have to look into that when I get a chance. Perhaps the
> Firefox user interface is itself constructed as web pages that you mentioned
> DOM methods?
Indeed it is. It runs on top of the Gecko engine (using XUL, JS and CSS), which allows us to design one interface and run it on multiple platforms. But that's best discussed outside of this bug, I think :)
Comment 39•15 years ago
|
||
(In reply to comment #38)
> Indeed it is. It runs on top of the Gecko engine (using XUL, JS and CSS), which
> allows us to design one interface and run it on multiple platforms. But that's
> best discussed outside of this bug, I think :)
Oh cool! I'll definitely have to read up on the Firefox user interface architecture when I get a chance. Thanks for the info! :-)
Alan
Comment 40•15 years ago
|
||
(In reply to comment #36)
> (In reply to comment #35)
> > Alex wrote:
> >
> > (In reply to comment #31)
> > > As far as I can tell the always on top approach isn't common on OS X.
>
> Yes, I was asking what the native behavior is. Is it modal dialogs? Or
> top-level windows? This might be too obvious, but unfortunately I have never
> used OS X :-(
Any OS X users, ping?
Comment 41•15 years ago
|
||
(In reply to comment #40)
> > Yes, I was asking what the native behavior is. Is it modal dialogs? Or
> > top-level windows? This might be too obvious, but unfortunately I have never
> > used OS X :-(
>
> Any OS X users, ping?
Those are normal windows which will be in the background when clicking another app window. Selecting "About APP" again will bring the window to the front again.
Assignee | ||
Comment 43•15 years ago
|
||
Ping?
Assignee | ||
Comment 44•15 years ago
|
||
Assignee: ehsan.akhgari → dao
Attachment #364762 -
Attachment is obsolete: true
Attachment #364890 -
Attachment is obsolete: true
Attachment #386499 -
Attachment is obsolete: true
Attachment #425421 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•15 years ago
|
Assignee | ||
Comment 45•15 years ago
|
||
updated to tip
Attachment #425421 -
Attachment is obsolete: true
Attachment #429970 -
Flags: review?(gavin.sharp)
Attachment #425421 -
Flags: review?(gavin.sharp)
Comment 46•15 years ago
|
||
Comment on attachment 429970 [details] [diff] [review]
patch
>diff --git a/browser/base/content/utilityOverlay.js b/browser/base/content/utilityOverlay.js
>+function openAboutDialog() {
>+ var enumerator = Services.wm.getEnumerator("Browser:About");
>+ while (enumerator.hasMoreElements()) {
>+ let win = enumerator.getNext();
>+#ifdef XP_WIN
>+ if (win.opener != window)
>+ continue;
>+#endif
>+ win.focus();
>+ return;
>+ }
What use is this loop on Windows? Won't the fact that the dialogs are dependent mean we'll always hit the "win.opener != window" case, if we get into the loop? I.e., how is it possible for openAboutDialog() to be called for a window whose dependent about dialog is already open?
> #ifdef XP_MACOSX
>+ var features = "chrome,resizable=no,minimizable=no";
resizable=no isn't needed here, as far as I can tell (for the same reason it isn't needed on Windows/Linux)
Assignee | ||
Comment 47•15 years ago
|
||
(In reply to comment #46)
> I.e., how is it possible for openAboutDialog() to be called for a window whose
> dependent about dialog is already open?
Dependent doesn't mean modal, so you can just go to Help > About ...
Unless I'm missing something.
Comment 48•15 years ago
|
||
Hrm, right. It's "on-top", but not modal, which is a bit weird.
Assignee | ||
Comment 49•15 years ago
|
||
It's basically the point of bug 275753 and this one.
Comment 50•15 years ago
|
||
Yes, I know. I just wasn't clear on the precise meaning of "dependent".
Comment 51•15 years ago
|
||
Comment on attachment 429970 [details] [diff] [review]
patch
It looks like just "resizable=no" or "minimizable=no" alone are both enough to disable both resizing and minimizing on Mac. I suppose we probably shouldn't rely on that, so this is fine as is I guess.
Attachment #429970 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 52•15 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a3
You need to log in
before you can comment on or make changes to this bug.
Description
•