239580 - default button in script-permission dialog should be "No"

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Jesse Ruderman]

The default button in the script-permission dialog should be "No", like it is in
the XPI dialog.

Comment 1

[Daniel Veditz [:dveditz]]

Taking. Do we want to block 1.7 for this?

Comment 2

[chris hofmann]

yeah we should get this in, espcially if it is a one-liner.

Comment 3

[Doug Turner (:dougt)]

Attachment: 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]]

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)]

before adding it, how about suggesting the fix?

Comment 6

[Daniel Veditz [:dveditz]]

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]]

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]]

update for you too dan.

Comment 9

[Mike Kaply [:mkaply]]

Attachment: 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]]

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]]

Comment on attachment 148169 [details] [diff] [review]
Switches the default to "No"

r-, like mkaply's change better.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 148411 [details] [diff] [review]
Patch as described

need another reviewer? Who owns this stuff anyway?

Comment 13

[Doug Turner (:dougt)]

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]]

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]]

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]]

Attachment: Update patch that uses upper byte.

Patch that uses upper byte.

Much uglier :)

Comment 17

[Daniel Veditz [:dveditz]]

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]]

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]]

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]]

fixed

There probably needs to be a firefox bug for this?

Comment 21

[Ben Goodger (use ben at mozilla dot org for email)]

Fix checked in on aviary branch. 

Comment 22

[Daniel Veditz [:dveditz]]

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]

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)]

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)]

/toolkit version checked in ala bug 253945, trunk and branch