82104 - `Software Installation' alert is unneccessarily complicated

Status: VERIFIED FIXED

(Core Graveyard :: Installer: XPInstall Engine, defect)

Description

[Matthew T (active 1999-2002)]

To reproduce:
*   Go to <http://segment7.net/mozilla/linktoolbar/>.
*   Click the `install the link toolbar' button.
*   Look at the resulting window.

What you see:
*   A dialog featuring text in a non-standard font at a non-standard size, a
    throbber (a throbber? what does it think it is, a Web browser?), an
    unnecessary separator line, enough text to ensure that no-one will read it,
    and buttons worded such that if you *don't* read the dialog text you'll have
    no idea what's going on.

What you should see:
*   A simple alert, saying `Are you sure you want to install “LinkToolbar” from
    “http://segment7.net/mozilla/linktoolbar/”? ( Cancel ) (( Install ))'

Comment 1

[Matthew T (active 1999-2002)]

Attachment: Screenshot

Comment 2

[Boris Zbarsky [:bzbarsky]]

over to xp apps: gui

Comment 3

[Doron Rosenberg (IBM)]

hmm, i think you can install numerous packages at once, so you might need to
keep the list stuff

Comment 4

[Blake Ross]

--> xpinstall

Comment 5

[Daniel Veditz [:dveditz]]

Accepting all help with font, layout and wording. Don't just tell us it's 
wrong, give us a spec -- us back end guys have no clue what "standard" is.

The scary wording, however, is on purpose. "Would you like to add a Sidebar?" 
with a simple prompt is one thing, with an install people need to be warned 
they are stepping outside the bounds of safe actions. If you've got better 
ideas how to do that we're all ears. Would you be happier with moving the 
warning to a second modal dialog ("Warning, installing stuff can be dangerous 
to your computer's health") with a "don't show me again" checkbox a la the 
security dialogs?

If at some future point we support signed installs we could combine the text 
with the certificate acceptance somehow.

Comment 6

[Matthew T (active 1999-2002)]

By `a simple alert', I meant an ordinary confirmation alert -- with /!\ icon, 
`Cancel' and `Install' buttons, and text as I described. I didn't realize it 
was possible for more than one item to be installed -- that makes it a bit more 
complicated.

+-----------------------------------------------------------+
|:::::::::::::::::::::::::::::::::::::::::::::::::::::::::::|
+-----------------------------------------------------------+
|   ,   Are you sure you want to install these 3 packages?  |
|  /!\                                                      |
|  """  +-----------------+-----------------------------+-+ |
|       | Name            | Source                      | | |
|       +-----------------+-----------------------------+-+ |
|       | Foo               http://xyz.foo.bar/foo.xpi  |A| |
|       | Bar               http://xyz.foo.bar/bar.xpi  | | |
|       | Hum               http://xyz.foo.bar/hum.xpi  | | |
|       |                                               |V| |
|       +-----------------+-----------------------------+-+ |
|       Malicious software could damage or spread files     |
|       stored on your computer. You should only install    |
|       packages from sources which you trust.              |
|                                                           |
|                                  (( Cancel )) (_Install ) |
+-----------------------------------------------------------+

Random notes:
*   The initial sentence will need to be split into three localizable strings:
    1.  `Are you sure you want to install this package?'
    2.  `Are you sure you want to install these' (the string before the number
        of packages)
    3.  `packages?' (the string after the number of packages).
*   No special font should be used. The text above the tree should be a
    <label>, and the text below the tree should be a <description>.
*   Because this process would be very easy to invoke maliciously (the button
    would look just like any other form submission button in a Web page), the
    default should be `Cancel' rather than `Install' -- that is, the `Cancel'
    button should respond to both Enter and Escape keys. Because this means the
    `Install' button doesn't respond to either Enter or Escape, it should have
    an accesskey (I).

Comment 7

[Alex Bishop]

Re warning text: "You should only install packages from sources which you trust."

I'm not certain, but I think it should be "sources that you trust." Also, most
people won't know what "packages" means. Maybe "software" could be used again
instead.

Comment 8

[Håkan Waara]

Got a patch 90% done. However, it won't include support for the flexible wording
proposed by mpt.

Comment 9

[Håkan Waara]

Attachment: Screenshot using patch

Comment 10

[Håkan Waara]

Attachment: Full patch (diff -u)

Comment 11

[Håkan Waara]

Attachment: Patch for review-purposes (diff -uw)

Comment 12

[Håkan Waara]

Comment on attachment 47817 [details] [diff] [review]
Patch for review-purposes (diff -uw)

Ok, need review on this patch (syd, dveditz)?

I also attached a screenshot that shows the goodness.

Comment 13

[Håkan Waara]

Attachment: Patch for review-purposes (-uw)

Comment 14

[Håkan Waara]

Comment on attachment 47821 [details] [diff] [review]
Patch for review-purposes (-uw)

I am sorry for the endless spam - I am learning the new patch-manager fu. ;)

This new patch fixes two problems:

1. Makes the dialog non-resizable as it should be.
2. Makes the dialog a tiny bit narrower.

Comment 15

[Syd Logan]

I'm concerned that by asking the "Are you sure" question, above the list and 
before the text that explains why it may be dangerous to install, that the 
message of the dialog to be ignored, or missed, and the user will just hit the 
install button.

Because it is a security related dialog, I think being a bit verbose and making 
the message appear before the question is important, so I cannot r= this patch 
as it is.

 

Comment 16

[Syd Logan]

One other issue, sometimes the path or name of the item to install is wide and a 
resize is needed to see all the content. I don't mind making the dialog fixed 
size but not sure that taking away the resizable flag is a really good idea.

Comment 17

[Daniel Veditz [:dveditz]]

bug 38505 asked us to make the dialog resizable so I'm not sure I like it being
taken away again. Unless, that is, you have some alternate method of being able
to see a long URL such as a tooltip over the truncated string.

Really, I'd like the list box to go away entirely. Users aren't able to select
anything in there so what's the point? Then you have to worry about growing the
dialog based on the number of items to install.

Assuming you put mouseover tooltips on the package names (but would the user
discover that?) here's something you could do:

+-----------------------------------------------------------+
|:::::::::::::::::::::::::::::::::::::::::::::::::::::::::::|
+-----------------------------------------------------------+
|   ,   A web site is requesting permission to install      |
|  /!\  the following on your machine:                      |
|  """                                                      |
|         LinkToolbar                                       |
|         spellchk.xpi                                      |
|                                                           |
|       Malicious software could damage your computer or    |
|       spread information stored on your computer. You     |
|       should only install software from sources that      |
|       you trust.                                          |
|                                                           |
|       Do you want to install this software?               |
|                                                           |
|                                  (( Cancel )) (_Install ) |
+-----------------------------------------------------------+

Forgive me if that comes out messed up, hard to work with this proportional font

Comment 18

[Daniel Veditz [:dveditz]]

My last comments are meant as starter ideas for the eventual re-working of this
dialog. I should put that in a new bug instead.

I am concerned about the resizability because the source URLs can be quite long.
But if there's a way to make them pop up or hide them that'd be great because
most people won't need them. Showing the source site all the time might be nice,
though.

Comment 19

[Daniel Veditz [:dveditz]]

hwaara mentions that part of this redesign is to shrink the size of the dialog.
In that case maybe the scary warning could go on an *additional* dialog that
comes up before the actual install confirmation.

Let me stress that I **REALLY HATE** the idea of imposing another click in the
process, but if it were like the security dialogs that people can turn off after
they've seen it once maybe that wouldn't be so bad. Maybe a
once-per-profile-ever dialog like the Obscuring-vs.-Encrypting dialog, but that
could be bad in a shared-computer situation. 

This is just brainstorming, not sure it helps. It might allow us to reduce the
scary text on the dialog to the single line tag "Only install software from
trusted sites" (or "sources that you trust")

Comment 20

[Doron Rosenberg (IBM)]

I'm wondering, is it really possible to install multiple packages at once? I've
never seen it done before.

Comment 21

[Matthew T (active 1999-2002)]

dveditz is right: confirmation alerts are merely annoying, but multiple
consecutive confirmation alerts are just cruel. But making the alert text more
verbose isn't the answer, since the length of the text is inversely proportional
to the fraction of people who can be bothered reading it at all.

If the tree could be got rid of, that would be great -- it would reduce the
visual complexity, it would make the user more likely to read the text, it would
get rid of the problem with rows in the tree being selectable for no reason, and
it would make it easier for the alert to be converted to a native one later. As
for the resizability flag, I have yet to see a good reason for an alert to be
resizable (other than by using an expander), and this alert isn't the first.
It's not necessary to see every last character of a long URL (or long package
name) in order to make an informed decision about it.

Could someone please tell Håkan how to find the number of packages being
installed? It's needed not only for the `this package'/`these n packages' text,
but also for limiting the list of items to 6 (with a `...' row following) so
that a page installing 57 items doesn't produce a distended alert which falls
off the bottom of the screen.

There is a small error in the following mockup: the `http://' should not be
included.

Comment 22

[Matthew T (active 1999-2002)]

Attachment: Design v3

Comment 23

[Doron Rosenberg (IBM)]

in function onLoad() i see a |numberOfDialogTreeElements| variable. It should
not be hard to hack addTreeItem() to add a simple text node for each item.

Comment 24

[Håkan Waara]

Been there, done that; it gave me '2' when there was only one package that was
about to be installed.

Comment 25

[Håkan Waara]

Ok, I really don't feel like implementing mpt's design v3, and while my patch is
an obvious improvement to the current dialog, can we check it in and keep this
bug open?

Dveditz and Syd, is that OK with you?

Comment 26

[Blake Ross]

"spread files stored on your computer" makes no sense to me.

Comment 27

[Alex Bishop]

> "spread files stored on your computer" makes no sense to me."

I think that refers to malicious packages that send your files to other people
or allow unauthorised access to your system.

Comment 28

[Daniel Veditz [:dveditz]]

I don't see why we need to specify the number of packages being installed --
that's just asking for localization trouble. How about a simple "Do you want to
install the following software?" or some such generic variant? Not, however,
"package(s)" which is ugly in English and unacceptable in other languages.

Instead of "damage or spread files stored on your computer" how about something
like "damage your computer or compromise your privacy." It's about the same
length, but is in terms that the average person has heard about. Maybe "violate"
instead of "compromise".

You don't want to put the URL source into the initial question. Each package
could technically come from a different server.  Or more likely, a different
path on the same server, but that's less likely to be interesting unless it's a
free service like geocities. I'll come clean a little -- 99% of folks don't care
and don't want to see that, but **I** want to see the path so I can figure out
the source, cancel the install, manually download the .xpi, and inspect it
before installing it. (I'm not the only one... while we're re-designing this
dialog keep rfe bug 47805 in mind if possible.)
numberOfDialogTreeElements is twice the number of installs; there is a name
element and a URL element for each one.

Multiple installs: see
http://developer.netscape.com/docs/manuals/xpinstall/Chap35.html#1016425
although the example unfortunately shows a single install. You would need this
to update Mozilla itself, in fact, since if you relied on the user to explicitly
click on each install they might forget to upgrade a part. This in turn leads to
a broken install if old components are left hanging around.  (See bug 94108 for
a relatively benign example caused by obsolete files; it can get worse.) At the
very least the old components themselves won't work (such as mail or security),
and at worst can stop the browser from starting at all (mismatched language pack)

I absolutely won't accept MPT's suggestion to elide a list of packages at some
arbitrary number.  An elided list with the last item being "6 more" and a link
to more information I could live with. A "details" or "more info" button could
be a place to hide the full URL information if you wanted. Makes the dialog
prettier, and still has the information for geeks like me.

back to the patch at hand... I think I agree with Håkan that his patch is an
incremental improvement (for folks at small resolutions) that could go in sooner
than the entire dialog could get redesigned. I think the text still needs some
improvement (see the top of this comment), and we need a patch in "diff -u10" to
give more context and elimintate the -w flag. The -w is useful as a secondary
review aide if there are a lot of whitespace changes, but we need a patch that
will actually apply and represents what will actually be checked in.

Comment 29

[Daniel Veditz [:dveditz]]

Comment on attachment 47821 [details] [diff] [review]
Patch for review-purposes (-uw)

Needs more context in diff (I usually prefer -10 or more; the default -3 is never enough). Also -w patches don't apply well for reviewers who want to try out the fix.

Please also consider the wording changes mentioned in bug comments.

I don't see how the resizability was hurting anyone, it only helps folks who wanted to see long URLs. Please leave the nsXPInstallManager.cpp change out.

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 47821 [details] [diff] [review]
Patch for review-purposes (-uw)

Needs more context in diff (I usually prefer -10 or more; the default -3 is never enough). Also -w patches don't apply well for reviewers who want to try out the fix.

Please also consider the wording changes mentioned in bug comments.

I don't see how the resizability was hurting anyone, it only helps folks who wanted to see long URLs. Please leave the nsXPInstallManager.cpp change out.

Comment 31

[Håkan Waara]

Comment on attachment 47821 [details] [diff] [review]
Patch for review-purposes (-uw)

New patch coming up with my own wording variant inspired by dveditz.

Comment 32

[Håkan Waara]

Attachment: Patch -u

Comment 33

[Håkan Waara]

The changes in the new patch:

* Tweaked the dialog's width slightly
* Changed the wording to be "Malicious software could damage files stored on
your computer or violate your privacy. You should only install software from
sources that you trust."
* Changed the wording to be "Are you sure you want to install the following
software?"

This new patch is diff -u format, so you can apply and test it if you want.
Requesting review from dveditz.

Comment 34

[Daniel Veditz [:dveditz]]

Comment on attachment 48149 [details] [diff] [review]
Patch -u

r=dveditz, looks great!

In the future give more context on diffs, like -u10 or thereabouts.

Comment 35

[Håkan Waara]

Blake, requesting your sr= here. Thanks.

Comment 36

[Christopher Blizzard (:blizzard)]

Comment on attachment 48149 [details] [diff] [review]
Patch -u

sr=blizzard

Comment 37

[Håkan Waara]

fixed.

I suggest you create a new bug for further changes/enhancements to this alert.

Comment 38

[Jimmy Lee]

Build: 2001-09-13-11-0.9.4(WIN), 2001-09-13-11-0.9.4(MAC),
2001-09-13-11-0.9.4(LINUX)

Looks good on all platforms.  Marking Verified!

Comment 39

[Denis Antrushin]

Attachment: Are you sure this looks nice? ;-)

Comment 40

[Denis Antrushin]

I don't think that hardcoded height of dialog is a good idea,
see screenshot (attachment 49572 [details]).
Is there any way to compute required window height?
Otherwise, isn't it better to remove 'height="260"'
window attribute?

Comment 41

[Håkan Waara]

Ouch! Thanks for notifying me about this, Dennis.

I will try to come up with a fix that does not hardcode the height.

Will you be able to test it on your system? That would help.

Comment 42

[Håkan Waara]

Without help from someone who has a system where this dialog is now horked, I
can't fix the bug.   Volunteers?

Comment 43

[Denis Antrushin]

How can I help you? (Well, I'm volunteer :-) )

Comment 44

[Denis Antrushin]

It's not reproducible anymore (checked on Sept. 21st build)....

Comment 45

[Håkan Waara]

Marking FIXED then.

Thanks for reporting back.

Comment 46

[Jimmy Lee]

All seems better. :-)  Marking Verified.