XPI install confirmation should default to cancel, not Install

VERIFIED FIXED in mozilla1.0.1

Status

Core Graveyard
Installer: XPInstall Engine
VERIFIED FIXED
16 years ago
2 years ago

People

(Reporter: BenB, Assigned: dveditz)

Tracking

({fixed1.4.3})

Trunk
mozilla1.0.1
fixed1.4.3
Dependency tree / graph
Bug Flags:
blocking1.7b +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt1 rtm] [ETA 08/10] [regression fixed(again)1.7b])

Attachments

(4 obsolete attachments)

(Reporter)

Description

16 years ago
Reproduction:
1. Go to <http://www.beonex.com/communicator/version/0.8/add-ons/binaries/java/>
and click the install butten or by some other means trigger the install of an XPI.

Actual result:
The confirmation dialog has 2 buttons, Install and Cancel, and the install
button is the default.

Expected result:
The Cancel button is the default.

Additional Comments:
- I think this is a regression, it at some point defaulted to Cancel.

Rationale:
- XPIs have full local rights - the install scripts and/or binaries in them may
do everything the user can do.
- We know that many users just click the default button (without reading any
text), if they hit a msgbox they don't know. Since it is very easy to trigger
that dialog (we don't prevent that in any way - a simple link or JS will
suffice), it is insanely easy for attackers to take control over the machines of
naive users. Actually, even users who are aware of the risks, but just in a
hurry, might not recignize the dialog and just hit enter / click the highlighted
button.
(Assignee)

Comment 1

16 years ago
There have been times when neither button was the default because we had a focus
problem, but I've always intended the Install button to be the default. I can
see your point, though. I think I'd prefer no default to a default of cancel.
It's just as much work to Install, but a lot harder to accidentally cancel out
on auto-pilot.

At some point we should re-think the wording and presentation of the dialog,
too. It says all the right things but it's just not scary enough.
Severity: major → enhancement
Status: NEW → ASSIGNED
Summary: XPI install confirmation defaults to Install → XPI install confirmation should default to cancel, not Install
(Assignee)

Comment 2

16 years ago
Need UE input on this dialog, but we definitely need to change the default.
Severity: enhancement → normal
Keywords: nsbeta1+
OS: Linux → All
Hardware: PC → All
(Assignee)

Comment 3

16 years ago
*** Bug 161713 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

16 years ago
Blocks: 161721

Comment 4

16 years ago
Adding adt1 rtm per the adt.
Whiteboard: [adt1 rtm]
Target Milestone: --- → mozilla1.0.1

Comment 5

16 years ago
The skin install and enablePrivilege dialogs also have Install/Yes as the 
default buttons (Enter key), but they have checkboxes so bug 133582 keeps them 
from having inital focus (Space key).
(Assignee)

Comment 6

16 years ago
Created attachment 94580 [details] [diff] [review]
(Av1) Make "cancel" the default
[Checked in: Comment 13 and 14]

Comment 7

16 years ago
We also need to change the defaults for the enablePrivilege and skin-install
dialogs.  The same usability arguments and security holes apply to those
dialogs, but with "space" replaced with "enter" because of bug 133582.

Comment 8

16 years ago
Comment on attachment 94580 [details] [diff] [review]
(Av1) Make "cancel" the default
[Checked in: Comment 13 and 14]

r=syd
Attachment #94580 - Flags: review+

Updated

16 years ago
Blocks: 143043
Keywords: mozilla1.0.1
Whiteboard: [adt1 rtm] → [adt1 rtm] [ETA Needed]

Comment 9

16 years ago
adt1.0.1+ (on the ADT's behalf) approval for checkin to the 1.0 branch, pending
sr= and Drivers' approval. pls check this in asap, then replace "mozilla1.0.1+",
with "fixed1.0.1". thanks!
Blocks: 147043
No longer blocks: 143043
Keywords: adt1.0.1+, approval
Whiteboard: [adt1 rtm] [ETA Needed] → [adt1 rtm] [ETA 08/10]

Comment 10

16 years ago
Comment on attachment 94580 [details] [diff] [review]
(Av1) Make "cancel" the default
[Checked in: Comment 13 and 14]

sr=mscott
Attachment #94580 - Flags: superreview+
Comment on attachment 94580 [details] [diff] [review]
(Av1) Make "cancel" the default
[Checked in: Comment 13 and 14]

Approved for 1.0 branch; change mozilla1.0.1+ to fixed1.0.1 when done.
Attachment #94580 - Flags: approval+

Updated

16 years ago
Keywords: mozilla1.0.1 → mozilla1.0.1+

Comment 12

16 years ago
a=asa (on behalf of drivers) for checkin to 1.1
(Assignee)

Comment 13

16 years ago
Fixed on 1.0 and 1.1 branches
Keywords: mozilla1.0.1+ → fixed1.0.1
(Assignee)

Comment 14

16 years ago
Checked into trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Reporter)

Comment 15

16 years ago
Thanks! :-)

Comment 16

16 years ago
verified on branch 1.0 (8/11) branch 1.1 (8/12)
Status: RESOLVED → VERIFIED
QA Contact: jimmylee → gbush

Updated

16 years ago
Keywords: fixed1.0.1 → verified1.0.1

Comment 17

14 years ago
The default is Install in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.7a) Gecko/20040213.  Did this regress?
(Assignee)

Comment 18

14 years ago
This regressed with sspitzer's fix for bug 214721 -- focus setting code got zapped
Status: VERIFIED → REOPENED
Keywords: regression
Resolution: FIXED → ---
(Assignee)

Comment 19

14 years ago
Should get fixed for the next release, marking blocker in case I forget
Flags: blocking1.7b+
Attachment #94580 - Attachment description: Make "cancel" the default → Make "cancel" the default [Checked in: Comment 13 and 14]
Attachment #94580 - Attachment is obsolete: true
Attachment #94580 - Attachment description: Make "cancel" the default [Checked in: Comment 13 and 14] → (Av1) Make "cancel" the default [Checked in: Comment 13 and 14]
Created attachment 142877 [details] [diff] [review]
(Bv1) <institems.js> (doing Av1 again)

Regression fix:
"Av1 like" patch, with "dialog syntax" instead of "window syntax".
Attachment #142877 - Flags: superreview?(mscott)
Attachment #142877 - Flags: review?(slogan)
Attachment #142877 - Attachment is obsolete: true
Attachment #142877 - Flags: superreview?(mscott)
Attachment #142877 - Flags: review?(slogan)
Created attachment 142879 [details] [diff] [review]
(Bv1b) <institems.js> (doing Av1 again)

Regression fix:
"Av1 like" patch, with "dialog syntax" instead of "window syntax";
plus a little cleanup.
Attachment #142879 - Flags: superreview?(mscott)
Attachment #142879 - Flags: review?(slogan)

Comment 22

14 years ago
Comment on attachment 142879 [details] [diff] [review]
(Bv1b) <institems.js> (doing Av1 again)

Serge: slogan@cts.com (Syd Logan) doesn't work on Mozilla anymore.

>    // set the okay button in the param block
>    if (gParam)
>-     gParam.SetInt(0, 0 );
>+   gParam.SetInt(0, 0);

the indenting here was fine.

>     // set the cancel button in the param block
>     if (gParam)
>-      gParam.SetInt(0, 1 );
>+    gParam.SetInt(0, 1);

same here.
Attachment #142879 - Attachment is obsolete: true
Attachment #142879 - Flags: superreview?(mscott)
Attachment #142879 - Flags: review?(slogan)
Created attachment 142929 [details] [diff] [review]
(Bv1c) <institems.*> (doing Av1 again)
[Checked in: Comment 33]

Bv1b, with comment 22 suggestion(s).

(Enforces 2-car. indentation on the few remaining lines which were not.)
Attachment #142929 - Flags: superreview?(mscott)
Attachment #142929 - Flags: review?(dveditz+bmo)
Attachment #142929 - Attachment description: (Bv1c) <institems.js> (doing Av1 again) → (Bv1c) <institems.*> (doing Av1 again)
(Assignee)

Comment 24

14 years ago
Comment on attachment 142929 [details] [diff] [review]
(Bv1c) <institems.*> (doing Av1 again)
[Checked in: Comment 33]

sr=dveditz, you can get r= from bsmedberg or ajschult
Attachment #142929 - Flags: superreview?(mscott)
Attachment #142929 - Flags: superreview+
Attachment #142929 - Flags: review?(dveditz+bmo)
Attachment #142929 - Flags: review?(ajschult)

Comment 25

14 years ago
Comment on attachment 142929 [details] [diff] [review]
(Bv1c) <institems.*> (doing Av1 again)
[Checked in: Comment 33]

r=ajschult
Attachment #142929 - Flags: review?(ajschult) → review+

Comment 26

14 years ago
Has anyone tested this on a Mac?
Comment on attachment 142929 [details] [diff] [review]
(Bv1c) <institems.*> (doing Av1 again)
[Checked in: Comment 33]


(In reply to comment #26)
> Has anyone tested this on a Mac?

Not me: PC/W98SE only.

Comment 28

14 years ago
(In reply to comment #26)
> Has anyone tested this on a Mac?

I tested on Linux.  Are there specific Mac issues that might be exposed by this
patch?
(In reply to comment #28)
> (In reply to comment #26)
> > Has anyone tested this on a Mac?
> 
> I tested on Linux.  Are there specific Mac issues that might be exposed by this
> patch?

neil:
Is there a (potential) issue here, or could I seek approval1.7b ?

Comment 30

14 years ago
The Mac plays by different rules: the Tab key doesn't work and the Enter key is
always OK. It would therefore have been incorrect for the patch to change the
default action on the Mac. As it is I asked a Mac owner and it does not.
Comment on attachment 142929 [details] [diff] [review]
(Bv1c) <institems.*> (doing Av1 again)
[Checked in: Comment 33]


approval1.7b?:
Solves blocking1.7b+ :-)
Attachment #142929 - Flags: approval1.7b?

Comment 32

14 years ago
Comment on attachment 142929 [details] [diff] [review]
(Bv1c) <institems.*> (doing Av1 again)
[Checked in: Comment 33]

a=chofmann for 1.7b
Attachment #142929 - Flags: approval1.7b? → approval1.7b+
Comment on attachment 142929 [details] [diff] [review]
(Bv1c) <institems.*> (doing Av1 again)
[Checked in: Comment 33]


Check in: { 03/11/2004 13:03	neil%parkwaycc.co.uk }
Attachment #142929 - Attachment description: (Bv1c) <institems.*> (doing Av1 again) → (Bv1c) <institems.*> (doing Av1 again) [Checked in: Comment 33]
Attachment #142929 - Attachment is obsolete: true
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago14 years ago
Keywords: regression
Resolution: --- → FIXED
Whiteboard: [adt1 rtm] [ETA 08/10] → [adt1 rtm] [ETA 08/10] [regression fixed(again)1.7b]

Comment 34

14 years ago
verified build 2004031708
Status: RESOLVED → VERIFIED
Comment on attachment 142929 [details] [diff] [review]
(Bv1c) <institems.*> (doing Av1 again)
[Checked in: Comment 33]

a=blizzard for 1.4.3
Attachment #142929 - Flags: approval1.4.3+
Keywords: fixed1.4.3
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.