94836 - `Open Web Location' dialog needs cleanup [`Open Address']

Status: RESOLVED WONTFIX

(SeaMonkey :: UI Design, defect)

Description

[Matthew T (active 1999-2002)]

Build: 2001080708, Mac OS 9.1

To reproduce:
1.  Choose `Open Web Location ...' from the `File' menu.

What you see:
    +----------------------------------------------------+
    |:::::::::::::::: Open Web Location :::::::::::::::::|
    +----------------------------------------------------+
    |   _   Enter the web location (URL), or specify the |
    |  (?)  local file you would like to open:           |
    |   `                                                |
    |       [http://lastopenedaddre|v] (Choose File ...) |
    |                                                    |
    |       Open in: [ Current Navigator wind...:^]      |
    |                                                    |
    |                              ( Cancel ) (( Open )) |
    +----------------------------------------------------+

What you should see:
    +----------------------------------------------------+
    |::::::::::::::::::: Open Address :::::::::::::::::::|
    +----------------------------------------------------+
    | Enter the address (URL) of the Web page, file, or  |
    | resource that you want to open.                    |
    |                                                    |
    |            ____________________________[B v]_[H v] |
    | _Address: [______________________________________] |
    |                                                    |
    | Open _in: (*) current Navigator window             |
    |           ( ) new Navigator window                 |
    |           ( ) Composer for editing                 |
    |                                                    |
    | (_Open File ...)             ( Cancel ) (( Open )) |
    +----------------------------------------------------+
where:
*   `B' is a bookmarks menu
*   `H' is a history menu.

Explanations:
*   The dialog isn't just for opening `Web Locations'; it can also be used to
    open FTP addresses, gopher listings, and even mailto: URLs.
*   The term `address' (e.g. `Web address', `FTP address') is considerably more
    familiar than `Location' is. (The menu item should be renamed too.)
*   This is a dialog, not an alert, so it shouldn't have an icon. (*Especially*
    not that horrible question icon.)
*   Labelling the text field (`Address:') helps make it unneccessary to read the
    text at the top of the dialog.
*   The menu part of the text field is just a dysfunctional history menu, one
    which only lists a subset of the URLs you are interested in.
*   There is no point in requiring the user to click two separate `Open' buttons
    (one in the filepicker and one in this dialog) if they go in here wanting
    to open a local file. (No, Timeless, there really isn't. If you want to edit
    the file:// URL later, you can do so in Navigator's address field.)
*   Where you have only three options, the number of options isn't variable, the
    options aren't intimidating, and there is room to do so, radio buttons
    should be used in preference to a popup menu.

Comment 1

[Håkan Waara]

Taking bug, I have done a patch without the bookmarks/history (RFE) menus coming
right up.

Comment 2

[Håkan Waara]

Attachment: screenshot

Comment 3

[Håkan Waara]

I have a problem with renaming the menuitem. All accesskeys for the characters
in "Open Address..." are already taken by existing menuitems.

What now?

Comment 4

[Håkan Waara]

Attachment: Patch -uw for review purposes

Comment 5

[Håkan Waara]

Blake and Ben, can you review this patch?

Comment 6

[Blake Ross]

If Open File... is going to open the filepicker and put the path of the chosen
file into the textbox then I'm not sure I understand its placement.

Comment 7

[Matthew T (active 1999-2002)]

No, `Open File ...' should close this dialog and then open the filepicker.

Comment 8

[Håkan Waara]

Comment on attachment 47980 [details] [diff] [review]
Patch -uw for review purposes

Better patch on the way which solves the accesskey problem by using 'r' as the accesskey.

It also readds autocomplete which I accidently removed.

Need reviews.

Comment 9

[Håkan Waara]

Attachment: Better patch, -uw

Comment 10

[Håkan Waara]

Attachment: Screenshot in Classic, by request

Comment 11

[Håkan Waara]

Anyone care to review?

Comment 12

[Kathleen :Brade]

Why "Composer for editing" instead of "New Composer Window"?
It seems odd that Composer is worded so differently from the others.
How about "New Composer window to edit"? or some variation of that? or leave as 
"New Composer Window"?

Comment 13

[Charles Manske]

The radio buttons begining with "new" and "current" should be "New" and "Current"
I agree with Kathy about being consistent with others, but I do see the motivation
of putting in the word "edit" someplace!
"New Compose window for editing" sounds good to me.

Comment 14

[Håkan Waara]

Comment on attachment 48010 [details] [diff] [review]
Better patch, -uw

I have a new patch that incorporates cmanske's suggestion.

Comment 15

[Håkan Waara]

Attachment: patch -u

Comment 16

[Håkan Waara]

Ok, requestion review from possibly cmanske/brade. This patch should be final,
and incorporates cmanske's suggestion about the wording.

Please r=/sr=.

Comment 17

[Charles Manske]

r=cmanske

Comment 18

[Blake Ross]

`new Composer window for editing' is pretty long.  Matthew, what do you think?

sr=blake

Comment 19

[Håkan Waara]

Fix checked in. 

mpt, if you want a new/improved wording, feel free to log a new bug on me.

Comment 20

[Blake Ross]

Please see Matthew's 9/2 comment, he specifically explains the behavior of the 
Open File button.

Comment 21

[sairuh (rarely reading bugmail)]

sorry to jump in late here --i [still] don't understand why clicking the Open
File [to get the file picker] should dismiss the Open Address dialog, thus
preventing the user from opening it in another browser window or an editor window?

Comment 22

[Blake Ross]

Furthermore, the menuitem in Composer remains Open Web Location.  And
autostretch is deprecated, please use align="start".

Comment 23

[Blake Ross]

And the first radiobutton is not properly disabled when Open Address is opened
from Composer with no open Navigator window.  It looks disabled, but you're able
to click it etc.  This does not happen in other places where a radio is disabled.

Comment 24

[timeless]

I object to automatically closing the open location dialog.

Comment 25

[Håkan Waara]

Open File: I discussed with mpt on IRC and was not convinced this should close
the dialog.

Radibutton disabling: I think this is a XPToolkit bug of some sort. The
radiobutton is properly disabled through the DOM. Any help appreciated.

Autostretch/Composer menu: Will come up with a new patch shortly.

Comment 26

[Håkan Waara]

Now that we have Open Address and Open File, perhaps we should just throw away
the Open File button in Open Address, since that has its own menuitem and to
prevent further confusion?

Objections?

Comment 27

[timeless]

Yes, I object.

it's not easy to select what component should open a local file w/o using the 
open location/address dialog.

the open file button also allows me to get a url for a file path, this is 
something that i can't easily get otherwise (and I might not want to load the 
page in navigator -- it might crash my browser or computer ...) -- I might want 
to load it w/ view-source, but i can't do that w/ file>open file.

Comment 28

[Håkan Waara]

1. File > Open File
2. View > Page Source

There. That's simple enough.

Comment 29

[Blake Ross]

Radiobutton disabling works elsewhere. Try .disabled.

Regardless of whether you were convinced that Open File should close the Open
Address dialog, the dialog as is is flawed. If you think it should put the file
path in the textbox then the button should be next to the textbox.

Comment 30

[timeless]

do 1. crash.
whoops. try to do 2. error: no mozilla left w/ which to do 2.

Comment 31

[Håkan Waara]

Comment on attachment 48723 [details] [diff] [review]
patch -u

New patch coming up with "Choose File..." reimplemented next to the textbox. Also fixes the menuitem in Editor to be "Open Address...".

The radiobutton issue is a XUL bug (no, .disabled didn't work...) so that I can't control. :(

Comment 32

[Håkan Waara]

Attachment: patch addressing (most of) blake's concerns

Comment 33

[timeless]

indentation in @@ -70,6 +71,8 @@ appears wrong.

Comment 34

[Håkan Waara]

Will correct that before I checkin.

Seeking reviewers for this patch...

Comment 35

[Doron Rosenberg (IBM)]

Comment on attachment 49475 [details] [diff] [review]
patch addressing (most of) blake's concerns

r=doron, assuming you fix the indentation issue timeless noted

Comment 36

[Alec Flett]

Comment on attachment 49475 [details] [diff] [review]
patch addressing (most of) blake's concerns

sr=alecf with the above changes

Comment 37

[Håkan Waara]

I checked this in earlier today. fixed.

Comment 38

[David Hyatt]

The checkins related to this bug have been backed out.  The UI changes in this
bug were never reviewed or approved by the Navigator UI module owner (Ben
Goodger), and since the landing, there have been numerous objections and
concerns raised over the changes made by this patch.

Changes to prominent dialogs cannot simply be landed without discussion and UI
review.  If you wish to discuss these changes rather than just landing them, I
suggest mailing pixeljockeys and/or dialing in to the next pixeljockeys meeting.

Comment 39

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Reopening since this was backed out.

Comment 40

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I don't see Ben Goodger listed anywhere but ViXeN in owners.html, and I do see
review and super-review of the original checkin, and none for your backout.  (I
note that we don't have Ben's all-important approval for the backout marked, for
example.)  In fact, I see only _one_ .xul checkin in the last week that had
Ben's review stamp.  And _you_ provided super-review for several of them.

There are no unaddressed concerns in this bug, and the reviewer and
super-reviewer were both happy with the quality of the patch.

I also don't believe that dialing into pixeljockeys or posting to
[e-mail] should be a requirement for checking in UI changes. 
The rules were followed here (and ben has been cc:d on this bug for the past two
three weeks), so if you want to file specific bugs about it, I think that's the
best thing to do.  The summary backout was highly inappropriate.  Please check
it back in at once.

Comment 41

[Brendan Eich [:brendan]]

I'm on [e-mail] and I'm going to dissent publicly, so behold our dirty
laundry.

shaver, we don't have any iron-clad rules about back-out.  I know of patches to
the style system which, if checked in with r= and sr= but without sr=hyatt or
sr=waterson, would lead to great conflict and likely back-out.  I'm not saying
that those patches are bad (I've got my opinions, but won't digress).  My point
is that where there is controversy, r= and sr= cannot be enough to get a patch
in -- otherwise, we could easily have back-out wars between r=/sr= factions.

It seems best to me for all concerned to build consensus.  Everyone professes
not to want a fork, and it would be foolish to fork at this point, before all
parties have argued the pros and cons.  If contributors are willing to pay the
costs of cooperating in order to achieve a common UI, we need a forum where a
change such as the one requested by this bug can be hashed out.  pixeljockeys
may not be enough, so crossposting there and to m.ui sounds better.  I know,
everyone who cares and has standing should read m.ui already....

/be

Comment 42

[David Hyatt]

Here are the technical objections that have been pointed out to me (as well as
some I have observed on my own) with this patch:

(1) The use of the term "Address" conflicts with address book terminology and is
quite a bit geekier than the term "Location".     

(2) The accelerator remained CTRL+SHIFT+L, the L of course standing for
Location.  Your change makes this accelerator nonsensical.
   
(3) The dialog is less than half its original width, and URLs can barely be
seen.   Only the first handful of characters are visible.
   
(4) The use of a radio group over a menu list bloats the height of the dialog
and puts "in your face" a handful of options that novice users simply don't need
to see.  A menulist is far better in this scenario than a radio group. 

Comment 43

[kurros (Wade Majors)]

I just want to back up Hyatt on number 4 above. A prominent example is Microsoft
changing the Shut Down dialog to a drop down list instead of radio buttons in
recent revisions of Windows. One would like to think Microsoft has spent
manhours researching this and doing real-world testing, and found it to be less
intimidating to users. Another point is that drop down boxes are much clearer as
to what is going to happen then a 3 pixel dot.

I still think its unnessesary to have separate initial open dialogs for internet
and local resources, but that's another bug discussion I guess.

Comment 44

[sairuh (rarely reading bugmail)]

keeping bill in the loop, since he has worked on this feature. :)

Comment 45

[Blake Ross]

Errr...there are indeed remaining issues with the dialog.  I myself meant to 
comment on the length of the dialog and the nonsensical Ctrl+Shift+L days ago, 
but my DSL went (and still is) down and I got distracted.  If you'll notice, 
the patch I reviewed did not have the length problem.  The dialog acquired it 
when the button was moved.

But I think it was pretty absurd to back this out so dramatically...you'll 
note, first, that Hakan asked superreview from Ben or I three weeks before 
checking in the patch.  There were issues with the original patch as well (I 
raised 4 of them on 9/11), and they were quickly fixed.  The patches were 
reviewed by two browser superreviewers. There are still obvious problems with 
the dialog, but I think raising them as I did earlier would have been a little 
nicer than backing it out, which I don't think was justified (there's no release 
any time soon...no hurry).   Of course I believe in getting consensus but I 
didn't realize Pixeljockies was for something as trivial as changing the name 
of a dialog and turning a menulist into some radiobuttons (the length is an 
obvious defect that needs fixing), in a dialog as obscure (I don't consider this 
prominent at all) as this dialog.

Oh, and if we're really going to get picky, this dialog really doesn't belong to 
Navigator any more than it does Composer ;-)

Comment 46

[Håkan Waara]

Hyatt, your opinions on these changes are valid, but I don't understand your
reasons for backing me out in this way.

I don't understand the module-owner-approval joker card that is played out. As
shaver pointed out, Ben is not on the module-owners page (specified as a
Navigator FE owner that requires to sr= all changes to xpfe/*) and I think that
both Blake and Alecf are both good and competent super-reviewers for this area
that gives me sufficent reviews so I can checkin to the trunk.

All the changes were thoroughly UI reviewed, code reviewed and super-reviewed...
everything according to the process. Concensus was reached among all CCs in this
bug.  

No one has contacted me about these negative opinions that you talk about
regarding this dialog. Neither you, nor Ben Goodger.

Where is it written that I am supposed to talk to "pixeljockeys" before changing
UI? Please give me information in other ways other than the "hard way" (by
backing out).

Hyatt, people that have opinions and requests for changes usually file bugs. You
backed all my work out -- I don't think that is fair!  There are other, better
ways to notify me about your concerns.

Comment 47

[Håkan Waara]

Back to module owner.

Comment 48

[Blake Ross]

I'm not the module owner, I'm the default component owner, but anyways...

Yes, please keep in mind that contributors outside of Netscape have no way of
knowing about Pixeljockeys or the pixeljockeys alias...I don't remember seeing
it anywhere.

So though I think a backout in this case was pretty irrational, I'll hold onto
this for now and not recheckin the patch, since it obviously needs some work...

Comment 49

[David Hyatt]

There's no intent to be dramatic here.  We seem to be living in a "fear of
backout" culture these days where every patch is sacred once it has been
committed.  The dialog clearly regressed with this checkin.  

If we must be sticklers for process, then feel free to check it back in.  I can
open another bug to back it out, and then I'll get my r and sr from the
appropriate people, and it will be backed out anyway.

Comment 50

[Blake Ross]

My point is that it's easier to leave the code checked in and fix the one or two
minor problems that remain.  I'm all for backing things out when they're visible
enough or blocking development or testing, when there's a release/deadline
imminent, or when the person who caused the regression shows unwillingness,
sluggishness or unresponsiveness in fixing the problems.  None of that was the
case here.  We didn't back out the patch that caused 101177, as the regression
was quickly fixed.

Comment 51

[Matthew T (active 1999-2002)]

> A prominent example is Microsoft changing the Shut Down dialog to a drop down
> list instead of radio buttons in recent revisions of Windows.

Heh. That's one example I use when talking about how bad Microsoft's UI design 
is. I could go on for hours about the problems with that dialog ...

Ahem. Håkan, could you possibly supply a new patch which is exactly the same as 
the previous ones combined, except that the dialog is wider? That's the only 
problem with the dialog as it was (apart from the behavior of the `Open 
File...' button, but you said you don't want to change that). Bonus points if 
you can change the keyboard shortcut from Shift+accel+L to Shift+accel+O too, 
but that's really a separate bug. (Ctrl+W, Ctrl+Z, Ctrl+X, and Ctrl+V don't use 
any letters of their en-US labels, either, and no-one's particularly concerned 
about changing those.) The rest of Hyatt's points are already rebutted in the 
original bug report.

Hyatt, Brendan gave Blake permission to sr= UI changes several months ago. If 
all Mozilla UI changes have to be approved by the Netscape Pixeljockies group, 
then given that Blake is no longer at Netscape perhaps his ability to sr= 
should be reconsidered.

Comment 52

[Blake Ross]

Matthew: I think the goal is to inform people when dialogs are being
restructured and changed around, so that people who don't happen to stumble upon
the bug can make comments and suggestions, and hopefully some form of consensus
can be reached.  This makes sense to me.

However, no, getting consensus from or even informing pixeljockeys can't yet be
a requirement, for the simple reason that I've yet to see mozilla.org even post
the alias of the group anywhere.  Everyone in this bug complaining about
pixeljockeys not having been informed seems to have forgotten that this request
was not made of mozilla contributors anywhere as far as I can tell.  Slapping
someone on the wrist for not abiding by a request never made of them is nonsensical.

Comment 53

[David Hyatt]

The radio button vs. menu list is an apples vs. oranges issue, and I can see
both sides of that argument.

Changing the term "Location" to "Address" is not a good change, however, and
should be reconsidered.

Comment 54

[Asa Dotzler [:asa]]

->0.9.6

Comment 55

[Blake Ross]

I don't believe there is anything wrong with the current design.  It's simple,
easy, what 4.x users expect, and not much of a departure from IE's comparable
dialog.  As more options (e.g. 'New Navigator tab') are added, I don't think the
use of radiobuttons is advantageous.  Marking WONTFIX.