Last Comment Bug 239580 - default button in script-permission dialog should be "No"
: default button in script-permission dialog should be "No"
Status: RESOLVED FIXED
: fixed-aviary1.0, fixed1.4.3, fixed1.7
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: x86 Windows XP
: -- minor (vote)
: ---
Assigned To: Mike Kaply [:mkaply]
:
: David Keeler [:keeler] (use needinfo?)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2004-04-04 03:28 PDT by Jesse Ruderman
Modified: 2004-08-13 17:53 PDT (History)
9 users (show)
asa: blocking1.7+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Switches the default to "No" (1.15 KB, patch)
2004-05-10 20:54 PDT, Doug Turner (:dougt)
dveditz: review-
Details | Diff | Splinter Review
Patch as described (6.15 KB, patch)
2004-05-13 06:51 PDT, Mike Kaply [:mkaply]
danm.moz: review+
dveditz: superreview+
Details | Diff | Splinter Review
Update patch that uses upper byte. (6.62 KB, patch)
2004-05-19 08:38 PDT, Mike Kaply [:mkaply]
danm.moz: review+
dveditz: superreview+
caillon: approval1.4.3+
asa: approval1.7+
asa: approval1.8a1+
Details | Diff | Splinter Review

Description Jesse Ruderman 2004-04-04 03:28:08 PDT
The default button in the script-permission dialog should be "No", like it is in
the XPI dialog.
Comment 1 Daniel Veditz [:dveditz] 2004-04-05 01:00:22 PDT
Taking. Do we want to block 1.7 for this?
Comment 2 chris hofmann 2004-05-10 15:09:56 PDT
yeah we should get this in, espcially if it is a one-liner.
Comment 3 Doug Turner (:dougt) 2004-05-10 20:54:39 PDT
Created attachment 148169 [details] [diff] [review]
Switches the default to "No"

This, unfortunately, switches not only the default button, but also the
ordering of the buttons (a flaw in the API, i think).  So, now, NO is on the
left and YES is on the right.
Comment 4 Mike Kaply [:mkaply] 2004-05-12 11:45:15 PDT
I don't think this is the right fix, as it sets a very bad UI precedent.

I think adding support for "defaultButton" to the prompt service is actually
pretty trivial.

I am doing it now.

I'll have a patch tomorrow.
Comment 5 Doug Turner (:dougt) 2004-05-12 12:49:38 PDT
before adding it, how about suggesting the fix?
Comment 6 Daniel Veditz [:dveditz] 2004-05-12 14:20:58 PDT
This must've been what I saw mkaply talking about on IRC (not positive, saw this
in scroll-back and he was gone). It appears there are 8 bits left in the flags
that he was going to try to use, or maybe it was the high-bit in each button
position.
Comment 7 Mike Kaply [:mkaply] 2004-05-12 16:51:29 PDT
Probably a good idea :)

Looking at nsIPromptService.idl:

http://lxr.mozilla.org/seamonkey/source/embedding/components/windowwatcher/public/nsIPromptService.idl#120

There is already a buttonFlags parameter passed in to confirmEX.

It has one free bit (it could have more if changed BUTTON_TITLE_IS_STRING to be
lower)

Anyway, I was thinking of adding a new value BUTTON_IS_DEFAULT set to 128 and
then this would get passed in with the other flags.

I would add a new enum to nsIPIPromptService.idl 

http://lxr.mozilla.org/seamonkey/source/embedding/components/windowwatcher/public/nsPIPromptService.idl#54

for another int called eDefaultButton.

Then in the ConfirmEx function I would translate the new flag to eDefaultButton
which would then be queryable in commonDialog.js

In commonDialog.js, I could query this value to decide which button to focus.

currently, commonDialog.js just queries the firstbutton and focuses it - my new
code would work no matter what because if it isn't set, it would default to
position 0 (the first button) otherwise it would focus the asked for button.

How's that sound?

The biggest part of the change will actually be the switch statement in
commonDialog.js.
Comment 8 Mike Kaply [:mkaply] 2004-05-12 16:52:28 PDT
update for you too dan.
Comment 9 Mike Kaply [:mkaply] 2004-05-13 06:51:59 PDT
Created attachment 148411 [details] [diff] [review]
Patch as described

This is a patch exactly as I described (very low impact)

I have verified that other ConfirmEx dialogs (security for instance) behave as
they did before.
Comment 10 Daniel Veditz [:dveditz] 2004-05-13 13:33:31 PDT
Comment on attachment 148411 [details] [diff] [review]
Patch as described

sr=dveditz

(The raw hex masks in nsPromptService.cpp are ugly, use a #define or constant
to make the code clearer)
Comment 11 Daniel Veditz [:dveditz] 2004-05-13 23:37:34 PDT
Comment on attachment 148169 [details] [diff] [review]
Switches the default to "No"

r-, like mkaply's change better.
Comment 12 Daniel Veditz [:dveditz] 2004-05-13 23:42:05 PDT
Comment on attachment 148411 [details] [diff] [review]
Patch as described

need another reviewer? Who owns this stuff anyway?
Comment 13 Doug Turner (:dougt) 2004-05-15 22:50:45 PDT
This kind of change isn't clean as you do not know if the underlying
implementation honors this flag.  New implementations will, but old
implementations won't.

Comment 14 Mike Kaply [:mkaply] 2004-05-16 06:25:30 PDT
old implementations will simply ignore the flag.

The API is designed such that thew flag is used when set otherwise you get the
old behavior.

using this bits seems like a very logical way to do this.
Comment 15 Daniel Veditz [:dveditz] 2004-05-17 14:21:16 PDT
Doug's probably right that embedding implementations will break. The fact that
the "string" value is 127 rather than 255 is the only clue that the high bit
might be usable for something else, and it's not much of a clue. The button
values themselves aren't bit values so implementations aren't going to be
thinking of masking anything out, they're just going to do
   switch(buttonvalue) {
      case BUTTON_TITLE_OK:
and anything with the new "defaultbutton" bit set won't match.

Much like the assumption made by the mozilla implementation you had to change
from using xff as a mask to x7f, in fact. Embeddors are likely to make the same
assumtions, epecially so if they used that code as a guide to fill out their
understanding of the .idl contract.

Reclaiming the wasted bit probably too clever. Safer to change this to use the
bottom two bits of the unused high byte in the flags. Any current
nsIPrompt[Service] implementation I can imagine would be ignoring that flag byte
so adding flags there would be safer.
Comment 16 Mike Kaply [:mkaply] 2004-05-19 08:38:42 PDT
Created attachment 148853 [details] [diff] [review]
Update patch that uses upper byte.

Patch that uses upper byte.

Much uglier :)
Comment 17 Daniel Veditz [:dveditz] 2004-05-19 09:51:51 PDT
Comment on attachment 148853 [details] [diff] [review]
Update patch that uses upper byte.

Not much uglier, about the same as your first patch (diff the patches and see).

>+#define BUTTON_DEFAULT_MASK 0xff000000

This is asking for the same kinds of potential problems as trying to use x80 --
what if we want to use another bit up there (say, to indicate delay enabling
the buttons, bug 162020)?

#define BUTTON_DEFAULT_MASK 0x03000000

>+  switch (buttonFlags & BUTTON_DEFAULT_MASK) {
>+    case BUTTON_POS_1_DEFAULT:
>+      block->SetInt(eDefaultButton, 1);
>+      break;
>+    case BUTTON_POS_2_DEFAULT:
>+      block->SetInt(eDefaultButton, 2);
>+      break;
>+    default:
>+      block->SetInt(eDefaultButton, 0);
>+      break;
>+  }

If you called it "ugly" because of the added switch you could always do it in
one line

block->SetInt(eDefaultButton, (buttonFlags & BUTTON_DEFAULT_MASK) >> 24);

pick your ugly, I guess.

sr=dveditz if you shrink the mask
Comment 18 Asa Dotzler [:asa] 2004-05-19 11:13:34 PDT
Comment on attachment 148853 [details] [diff] [review]
Update patch that uses upper byte.

a=asa (on behalf of drivers) for checkin to 1.8a1 and 1.7
Comment 19 Mike Kaply [:mkaply] 2004-05-19 12:08:28 PDT
No, I think it's ugly because I thought using the spare upper bit was very
elegant because it was setting a style on each button in the upper bit :)
Comment 20 Mike Kaply [:mkaply] 2004-05-24 06:36:30 PDT
fixed

There probably needs to be a firefox bug for this?
Comment 21 Ben Goodger (use ben at mozilla dot org for email) 2004-06-01 13:04:49 PDT
Fix checked in on aviary branch. 
Comment 22 Daniel Veditz [:dveditz] 2004-06-02 01:41:05 PDT
Wouldn't Firefox use mozilla/toolkit/content/commonDialog.js? The AVIARY branch
check-in included mozilla/xpfe/global/resources/content/commonDialog.js instead.
Comment 23 Michael Lefevre 2004-06-21 07:57:33 PDT
according to mconnor, this should indeed go into the other file for Firefox, and
that still needs to happen.  should also go into Firefox trunk.
Comment 24 Christopher Aillon (sabbatical, not receiving bugmail) 2004-07-29 12:50:06 PDT
Comment on attachment 148853 [details] [diff] [review]
Update patch that uses upper byte.

Easy backport.	a=blizzard for 1.4.3
Comment 25 Ryan Polk (Quark) 2004-08-13 17:53:14 PDT
/toolkit version checked in ala bug 253945, trunk and branch

Note You need to log in before you can comment on or make changes to this bug.