36208 - Open Web Loc has New Composer as default when there are no windows

Status: RESOLVED WONTFIX

(SeaMonkey :: UI Design, defect, P4)

Description

[sairuh (rarely reading bugmail)]

i've seen this in both the branch (m15) and trunk (m16) for the opt commercial
Mac bits. it's minor, but kinda annoying, imho.

1. close all browser windows so that only the menubar is present.
2. select File > Open Web Location to bring up the Open Web Location dialog.

observe that "Open in which window" is set to "New Composer." shouldn't it be
"New Navigator"? that would make more sense, since (when there is a browser
window present) the default is "Current Navigator" --ie, methinks a user would
be more likely to open a browser window than a composer window.

if this is expected, i'd like to understand why. :-)

Comment 1

[don]

Move to M21 target milestone.

Comment 2

[Blake Ross]

Sarah, are you still seeing this?

Comment 3

[sairuh (rarely reading bugmail)]

yep, still seeing this (Mac only), at least on today's commercial branch bits.
[hm, what happened to the Mac System 9.0 option in the OS droplist here...?]

Comment 4

[Peter "jag" Annema]

Yay! Found a bug for my fix!

Comment 5

[Peter "jag" Annema]

Attachment: [patch] Make Open Location dialog default to New Window when no windows open

Comment 6

[Peter "jag" Annema]

So what I'm doing here is getting rid of the need to pass in appCore (eeew) and
making the "what to disable, what to default to" logic a bit more ... logical.

Next wish is to be able to pass in a default URL, but that's another bug.
It can be enhanced more if so desired, so please comment.

Comment 7

[sairuh (rarely reading bugmail)]

i couldn't find the bugid for the one where we should use the last used url
[i'll do another query soon], but speaking of enhancements, there's bug 38865
and bug 38409...

Comment 8

[sairuh (rarely reading bugmail)]

found it --the "use last url" one is bug 42732.

Comment 9

[Peter "jag" Annema]

Since this same patch covers bug 41527 too, I'll mention it here for
completeness sake.

Comment 10

[sairuh (rarely reading bugmail)]

removing m21 milestone --i've tentatively set it to mozilla0.9... zat okay?
could the patch be reviewed for checkin, too? thx!

Comment 11

[Bill Law]

I have a couple of comments re: the proposed patch:

1. The use of getElementsByTagName should generally be avoided whenever
possible, due to performance issues (it has to iterate over the entire
document).  Would it be better to just put the windowType as a property of
window (e.g., window.windowType = "mozilla:hidden" in hiddenWindow.js) which
would then be accessible via window.opener.windowType in openLocation.js?  That
would likely be more efficient.

2. If one steps back and looks at this "component" (viewing the dialog as a
"component" and by rights it should be; but that's a whole 'nother can of
worms), then its current interface is basically:

   interface openLocation {
      void open( in nsIBrowserInstance browser );
   };

where "browser" is optional.  Your patch changes this method (effectively) to:
      void open( in nsIDOMWindow opener );

Actually, the "argument" is not just any window, it must be one with the JS
properties loadURI and delayedOpenWindow (not to mention being a XUL window with
certain attributes on the <window> element).

Ignoring the issue of whether that "interface" is an improvement or not (which
is technically orthogonal to the issue of whether the implementation is better),
here are the issues I would take if this were a newly proposed interface:

1. There is no way to give the caller control over the dialog's selection of the
default control (or any other aspect of the dialog).  Consider using this dialog
from the viewSource window.  We probably wouldn't want that window to have the
dialog default to opening a new composer window.  And we would likely want to
add an option to open a new view-source window, perhaps, or to change the labels
of the default options.

2. There is no way to extend the dialog so it can do something different (e.g.,
use a different chrome url for Composer windows, not open a .xul window for a
new browser window, open a view-source window if the caller was a view-source
window, etc.).  This sort of thing is likely to become more important in terms
of embedding where we need more flexibility in the UI.

If I was designing this from scratch, I would design it more like that of a
typical windowing system dialog interface (i.e., Win32).  In such systems, you
would subclass the dialog and override the handling of various messages to
tailor the dialog for your use.  Alternatively, or in addition, some dialogs
have notification and callback interfaces with their parent window.

Since we don't really have a window subclassing mechanism (although we could
likely invent one), if we wanted to facilitate similar "customization" of the
dialog it would have to be via a notification/callback mechanism.  In this case,
it would look something like this:

interface openLocationParent {
   void onLoad( in nsIDOMWindow dialog );
   void onCommand( in int selection );
};

Next, I'd formalize the "openLocation" dialog as a service (this would enable it
to be implemented via means other than .xul/.js which then facilitates embedders
use of this service):

interface openLocationDialogService {
   void open( in openLocationParent parent );
};

The default implementation of this service would simply do an openDialog of the
existing .xul/.js dialog code, but would pass the dialog the openLocationParent
object.  The onload handler would call openLocationParent::onLoad so that the
parent could tweak the UI, select the proper default, etc.  The code that
currently does the action would instead call openLocationParent::onCommand,
passing the various dialog input fields (or just the dialog itself, from which
these fields could be obtained).

OK, so this is all pie-in-the-sky brainstorming.  But I'd really like to see us
move towards that model and we'll never get there without taking the first step.

On the one hand, I'm thinking that if we're going to change the "openLocation
dialog interface", I'd prefer that it move in that direction rather than
implement an intermediate step that's still broken.  On the other hand, the
intermediate step fixes this bug, and right now :-).

So I guess I should really open a new bug that says our whole dialog/window
model needs to be improved and take this patch in the interim.  The down side is
that if we keep "patching" the current (non?) design, there's less incentive to
fix the design.  But that's a discussion for some other forum.

Sorry for rambling on so much.  Go ahead and check this in.  But please think
about this and comment.  I'll post something to mozilla-xpfe after I've had time
to think this through a bit more.

Comment 12

[Peter "jag" Annema]

I'll be looking forward to discussing this further in the newsgroups. I have
been thinking about the issues you address here, but chose this solution for
these two bugs and get rid of appCore in the mean time.
I realize it's from bad to (slighty less) bad, but this unblocks me elsewhere
(bug 46200) so I can better focus on these kinds of issues when I'm done with
that.

Btw, the content of the dialog is an implementation detail you shouldn't (have
to) expose to the caller. Instead I think it would be better to let the dialog
determine which items to enable/disable through the window mediator, and pass in
a default selection and a callback (I'd opt for a different name than onCommand
to avoid confusion) to make that relationship more explicit.

Actually, I could do this tomorrow, it would be a better step in the right
direction than the current patch, yet not much more work.

Comment 13

[Paul Chen]

nav triage team:

A "simple" thing to fix, though the more general solution Bill proposes is 
better. Marking nsbeta1, p4

Comment 14

[Paul Chen]

no, really, marking nsbeta1, reassigning to pchen

Comment 15

[Paul Chen]

Looks like the right way to fix may take a bit of work. Doesn't look like I'll
get to this for mozilla0.9. Marking nsbeta1-, resetting target milestone, adding
polish keyword.

Comment 16

[Paul Chen]

Marking nsbeta1+, mozilla0.9

Comment 17

[Paul Chen]

Moving to mozilla0.9.1

Comment 18

[Viswanath Ramachandran]

as discussed in nav team meeting, moving P4 and P5 nsbeta1+ bugs to Future. 

Comment 19

[Aleksey Nogin]

Is this bug related to bug #81793 "First browser window always goes to the home
page."?

Comment 20

[sairuh (rarely reading bugmail)]

trudelle to triage

Comment 21

[sairuh (rarely reading bugmail)]

moving open bugs pertaining to the Open Web Location dialog to [e-mail]
as qa contact.

to find all bugspam pertaining to this, set your search string to
"GottaGetThisHenOut!"

Comment 22

[Peter Trudelle]

->jag/future

Comment 23

[jag (Peter Annema)]

filter keyword: nothingnessless

Comment 24

[Simon Paquet [:sipaq]]

This bug is targeted at a Mac classic platform/OS, which is no longer supported
by mozilla.org. Please re-target it to another platform/OS if this bug applies
there as well or resolve this bug.

I will resolve this bug as WONTFIX in four weeks if no action has been taken.
To filter this and similar messages out, please filter for "mac_cla_reorg".

Comment 25

[Simon Paquet [:sipaq]]

WFM on Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.6a)
Gecko/20031013, marking WONTFIX because we no longer support Mac classic.