Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED

Status

()

Core
Security
--
minor
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: Jesse Ruderman, Assigned: mkaply)

Tracking

({fixed-aviary1.0, fixed1.4.3, fixed1.7})

Trunk
x86
Windows XP
fixed-aviary1.0, fixed1.4.3, fixed1.7
Points:
---
Bug Flags:
blocking1.7 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

14 years ago
The default button in the script-permission dialog should be "No", like it is in
the XPI dialog.
Taking. Do we want to block 1.7 for this?
Assignee: security-bugs → dveditz+bmo
Flags: blocking1.7?

Updated

13 years ago
Flags: blocking1.7? → blocking1.7+

Comment 2

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

Comment 3

13 years ago
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.
Assignee: dveditz → dougt
Status: NEW → ASSIGNED

Updated

13 years ago
Attachment #148169 - Flags: review?(dveditz)
Attachment #148169 - Flags: approval1.7?
(Assignee)

Comment 4

13 years ago
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

13 years ago
before adding it, how about suggesting the fix?
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.
(Assignee)

Comment 7

13 years ago
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.
(Assignee)

Comment 8

13 years ago
update for you too dan.
(Assignee)

Comment 9

13 years ago
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.
Assignee: dougt → mkaply
Attachment #148169 - Attachment is obsolete: true
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)
Attachment #148411 - Flags: superreview+
Attachment #148411 - Flags: review?(danm-moz)
Comment on attachment 148169 [details] [diff] [review]
Switches the default to "No"

r-, like mkaply's change better.
Attachment #148169 - Flags: review?(dveditz)
Attachment #148169 - Flags: review-
Attachment #148169 - Flags: approval1.7?
Comment on attachment 148411 [details] [diff] [review]
Patch as described

need another reviewer? Who owns this stuff anyway?
Attachment #148411 - Flags: approval1.7?

Comment 13

13 years ago
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.

(Assignee)

Comment 14

13 years ago
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.

Updated

13 years ago
Attachment #148411 - Flags: review?(danm-moz) → review+
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.
(Assignee)

Comment 16

13 years ago
Created attachment 148853 [details] [diff] [review]
Update patch that uses upper byte.

Patch that uses upper byte.

Much uglier :)
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
Attachment #148853 - Flags: superreview+
Attachment #148853 - Flags: review?(danm-moz)
Attachment #148853 - Flags: approval1.8a1?
Attachment #148853 - Flags: approval1.7?

Updated

13 years ago
Attachment #148853 - Flags: review?(danm-moz) → review+

Comment 18

13 years ago
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
Attachment #148853 - Flags: approval1.8a1?
Attachment #148853 - Flags: approval1.8a1+
Attachment #148853 - Flags: approval1.7?
Attachment #148853 - Flags: approval1.7+
(Assignee)

Comment 19

13 years ago
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 :)

Updated

13 years ago
Attachment #148411 - Flags: approval1.7?
(Assignee)

Comment 20

13 years ago
fixed

There probably needs to be a firefox bug for this?
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Keywords: fixed1.7
Resolution: --- → FIXED
Fix checked in on aviary branch. 
Whiteboard: fixed-aviary1.0
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

13 years ago
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.
Whiteboard: fixed-aviary1.0 → needed-aviary1.0
Comment on attachment 148853 [details] [diff] [review]
Update patch that uses upper byte.

Easy backport.	a=blizzard for 1.4.3
Attachment #148853 - Flags: approval1.4.3+
Keywords: fixed1.4.3

Comment 25

13 years ago
/toolkit version checked in ala bug 253945, trunk and branch
Keywords: fixed-aviary1.0
Whiteboard: needed-aviary1.0
You need to log in before you can comment on or make changes to this bug.