94437 - Clean up wallet prefill dialog

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Asa Dotzler [:asa]]

The preview form dialog is the worst piece of UI in our entire product.  Please
fix this.

German, is this to your spec?

screenshot coming.

Comment 1

[Asa Dotzler [:asa]]

Attachment: please help me

Comment 2

[Asa Dotzler [:asa]]

Attachment: even if you make it taller it still has a vertical scrollbar and what's with all the differently sized select texts?

Comment 3

[Asa Dotzler [:asa]]

You've got to be kidding!!! (yes, that was three exclamation points. three.)  

Steve, are you seriously saying that you don't care that your feature is
represented by the worst dialog in the entire product?  How did that ever pass
review or super review?  Surely you could talk to blake or hyatt or goodger or
just about anyone who knows XUL and get some suggestions about making this
dialog better. 

 

Comment 4

[german]

the spec image is located at
http://www.mozilla.org/projects/ui/communicator/browser/formfill/proposedconfirm
2.png
the design did not intend to use hor. scrollbars, as this makes the dialog 
indeed illegible. The label text was supposed to be using abbreviation 
(using ..., looking at it now we should be using middle ellipses as the end of 
the label text seems to be at least as important to distinguish the fields)when 
the dialog width was smaller than that plus the popup width. It seems like its 
throwing up the hor. scrollbars anyway. You need the vertical scrollbars as 
overflow method as you don't know at designtime of the dialog how many 
fields/rows it will contain.
Clearly the checkboxes should be aligned with the text in the row (remember 
election 2000 :-), and the text in both labels and popup contents should be 
using the same font an height and be vertically aligned. 
Clearly this dialog needs front end engineering help here, Steve was probably 
trying to get it as close as possible to the spec.

Comment 5

[german]

to be clear, I meant to see vertical scrollbars are needed as overflow method 
meaning they would show up only when there are more rows and then fit the 
visible area given the current dialog height. The list area should stretch when 
increasing the dialog height.

Comment 6

[Jason Kersey]

I've fixed this up.  Code coming.

Comment 7

[Jason Kersey]

Attachment: Clean up the prefill window.

Comment 8

[Stephen P. Morse]

Attachment: screen shot before patch

Comment 9

[Stephen P. Morse]

Attachment: same screen shot after patch

Comment 10

[Stephen P. Morse]

1. Text and associated checkbox is still misaligned
2. Text items are no longer aligned with each other
3. Menulists are no longer left-aligned with each other
4. Position of "View Stored Form Data" button no longer conforms to german's 
spec

Comment 11

[Stephen P. Morse]

Attachment: second before image

Comment 12

[Stephen P. Morse]

Attachment: after shot for second before shot

Comment 13

[Stephen P. Morse]

Second before/after pair have same problems as mentioned above plus:

5. Window now has a horizontal scroll which previously wasn't necessary

Comment 14

[Asa Dotzler [:asa]]

Not sure what's going on here but I saw this patch in action and it looks
nothing like morse's screenshots. The dialog was absolutely beautiful.

Comment 15

[Jason Kersey]

Doesn't look like this for me.  I tried in both classic and modern, and both
looked ok.  Can you post a screenshot of it in modern?  I'll post my screenshots
tomorrow when i get back to my build.

Comment 16

[Stephen P. Morse]

Attachment: per request, here's an after shot in modern

Comment 17

[Jason Kersey]

Attachment: My screenshot

Comment 18

[Jason Kersey]

I've had two others apply the patch, and the see the same window I do.  Not sure
what's going wrong in your build, did it apply cleanly?

looking for r/sr.

Comment 19

[Stephen P. Morse]

Looks like my build was outdated.  I just pulled a fresh tree and applied the 
patch.  This time I don't see the problems.

a=morse as module owner.

Comment 20

[Stephen P. Morse]

Also r=morse

Comment 21

[tdowling]

The dialog box that sticks out the worst for me is the Basic Authentication
dialog when the prompt "Enter username and password for..." wraps to a second
line.  The dialog does not resize, and the result is to push the OK and Cancel
buttons halfway out of the window.

Comment 22

[tdowling]

Attachment: Basic Auth dialog box with buttons cropped

Comment 23

[Jason Kersey]

This isn't a bug for posting all the rotten dialogs in the product.  Cleaning
this up.

Comment 24

[Adam Lock]

Perhaps this is stating the obvious but this dialog clearly looks like it should
be using a multi-column grid. A good place to see an alternate (though not
necessarily perfect) way to display the same information would be the
cookie/image viewer dialogs.

Is there some limitation that prevents grids from holding combo boxes? That's
the only thing I could see preventing such a change.

Comment 25

[Mike Shaver (:shaver emeritus)]

Comment on attachment 50646 [details] [diff] [review]
Clean up the prefill window.

>-//      menuList.setAttribute("size", Number(count)+1);
>+//    menuList.setAttribute("size", Number(count)+1);

>-//    menuList.setAttribute("editable", "true");  // done later to avoid crash
>+      //menuList.setAttribute("editable", "true");  // done later to avoid crash

Don't leave commented-out code in.  We have CVS if we need to
see other versions.

And if there's a crash you're working around, please cite a bug
number in a comment.

>+      if (colonPos != -1)
>+        var checkBoxLabel = prefillList[i+1].slice(colonPos + 1)
>+      else
>+        var checkBoxLabel = prefillList[i+1];

You should run with strict warnings: the double-declaration of
|var checkBoxLabel| would have shown up.

Fix these things, and any other strict warnings that show up, and
sr=shaver.

Comment 26

[german]

Kerz's screenshot looks like an good improvement to me. That way we save one
line at the top and make the text at the top more extensible to accomodate l10ns.

Comment 27

[Jason Kersey]

Attachment: New patch addressing shaver's comments.

Comment 28

[Mike Shaver (:shaver emeritus)]

Comment on attachment 51596 [details] [diff] [review]
New patch addressing shaver's comments.

To know you is to love you.  sr=shaver.

Comment 29

[Jason Kersey]

donner wanna check this in for me?

Comment 30

[[e-mail] (gone - use [e-mail] instead)]

I checked in the fix.