Closed Bug 345067 Opened 18 years ago Closed 1 year ago

Issues with prompt service's confirmEx - confirmEx always returns 1 when user closes dialog window using the X button in titlebar

Categories

(Toolkit :: UI Widgets, defect)

defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: Biesinger, Unassigned)

References

Details

Attachments

(1 file)

confirmEx always returns 1 if the user closes the window using the X button in the titlebar (or whatever feature the window manager offers).

That's bad because that button need not be the cancel button. Even if it is, the caller might want to distinguish between the second button being pressed and the dialog being closed by the X.

At the very least this behaviour needs to be documented.
Flags: blocking1.9a2?
Flags: blocking1.9a2? → blocking1.9-
Whiteboard: [wanted-1.9]
Flags: wanted1.9+
Whiteboard: [wanted-1.9]
Would changing the behavior negatively impact any users of confirmEx?

elevating to sev=major given bug 380862.

is bug 329414 a duplicate?
Severity: normal → major
Summary: Issues with prompt service's confirmEx → Issues with prompt service's confirmEx - confirmEx always returns 1 when user closes dialog window using the X button in titlebar
(In reply to comment #3)
> Would changing the behavior negatively impact any users of confirmEx?

Depends on how we fix this,,,
Blocks: 384641
This seems to happen also when the user presses escape key (at least on mac). A suggestion for solving this is by adding a flag BUTTON_DEFAULT_CANCEL (or multiple for each button). And then leaving the default cancel button to be the second button (button 1) so that the behavior doesn't change for the current apps.
No longer blocks: 380862
By the way, can You fix other problem with confirmEx:

I used MDC example for confirmEx:

var prompts = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
                        .getService(Components.interfaces.nsIPromptService);

var check = {value: false};                  // default the checkbox to false

var flags = prompts.BUTTON_POS_0 * prompts.BUTTON_TITLE_SAVE +
            prompts.BUTTON_POS_1 * prompts.BUTTON_TITLE_IS_STRING  +
            prompts.BUTTON_POS_2 * prompts.BUTTON_TITLE_CANCEL;
// This value of flags will create 3 buttons. The first will be "Save", the
// second will be the value of aButtonTitle1, and the third will be "Cancel"

var button = prompts.confirmEx(null, "Title of this Dialog", "What do you want to do?",
                               flags, "", "Button 1", "", null, check);


This example tells I should get prompt with buttons:
  "Save", "Button 1", "Cancel"
But instead I get 
  "Save", "Cancel", "Button 1"

Tested on Thunderbird 3.0.4 and 3.1 beta 2, Windows XP.


Or maybe I should fill another bug for this problem?
(In reply to comment #7)
> Or maybe I should fill another bug for this problem?

I've filed one for you here: Bug 624043
I can't believe this bug has been open for 5 years.

confirmEx is still broken. There's no way to distinguish the closing the dialog from clicking on button 1.

If there are no plans to fix this, so be it. But leaving this bug untouched for 5 years is silly.
Kaply, who were you going to fix it? I don't think we need the fix for any of the in-product dialogs, which makes it a pretty low priority for people who are primarily working on making Firefox better.
What's sad is that the prompt interface was completely rewritten last year as part of bug 563274, but this bug wasn't included in that.

Adding Justin Dolske since he did the rewrite.
Attached patch very rough patchSplinter Review
This is a very rough patch, but it gets the concept across.

It modifies confirmEx to allow for a button 2 without a button 1.

By leaving button 1 blank, you can see that the "!" value from confirmEx did not apply to any of the buttons you specified.

This allows for the new behavior without changing any of the old behavior.
(In reply to Michael Kaply (mkaply) from comment #12)
> What's sad is that the prompt interface was completely rewritten last year
> as part of bug 563274, but this bug wasn't included in that.

The point of the rewrite was to convert the incomprehensible code that used to exist into something maintainable. In some cases, such as this, existing behavior was left alone ("no worse") to minimize the already-high regression risk and speed reviewing along. Giant refactorings that try and fix a million unrelated things are almost impossible to review.
Comment on attachment 556890 [details] [diff] [review]
very rough patch

Review of attachment 556890 [details] [diff] [review]:
-----------------------------------------------------------------

I think a better solution here would be to add BUTTON_POS_#_DEFAULTCANCEL constants, ala the existing BUTTON_POS_#_DEFAULT constants (in nsIPrompt).

Looks like the current state of things is that if a ConfirmEx caller doesn't specify the default button (for success), we default to 0. Similarly, we default to 1 for the default cancel button -- the core of this bug is that there's no way to change it.

::: toolkit/components/prompts/src/nsPrompter.js
@@ +655,2 @@
>          }
>  

I don't think it's a good idea to allow callers to set up noncontiguous button sets. Makes for confusing UI and code. Doesn't look like this was supported in the old code; it also assumed the number of specified button titles == number of buttons to show.

I'd take a patch to make a more explicit failure if a caller tries to use noncontiguous buttons...
Attachment #556890 - Flags: feedback-
I'm having trouble figuring out of I can do a bitmask for this situation. Would appreciate thoughts.

So to maintain old behavior, the 1 option needs to be mapped to 0. So something like this:

    const unsigned long BUTTON_POS_1_CANCEL = 0 << 28;
    const unsigned long BUTTON_POS_0_CANCEL = 1 << 28;
    const unsigned long BUTTON_POS_2_CANCEL = 2 << 28;

So when we mask the bits, we want 

00 to become one
10 to become 0
11 to stay 2

Am I over thinking this in trying to get it to work with bit flags?
Hmm, that sounds about right to me. Not breaking existing callers means we have to either made the no-bits-set case == button1, or do something to differentiate "caller didn't specify" and "caller made an explicit choice". EG:

    const unsigned long BUTTON_POS_DEFAULT_CANCEL = 0 << 28;
    const unsigned long BUTTON_POS_0_CANCEL = 1 << 28;
    const unsigned long BUTTON_POS_1_CANCEL = 2 << 28;
    const unsigned long BUTTON_POS_2_CANCEL = 3 << 28;

But that's a little weird, and not sure it's useful.

From an API sanity POV, callers won't care about the actual values of the consts, so even though the bits are kinda out of order it's just an quirk we can deal with in the prompt code.
Blocks: 521158
No longer blocks: 521158
Ops, midair mis-click, sorry.
Blocks: 521158
Marking a bunch of bugs in the "Embedding: APIs" component INCOMPLETE in preparation to archive that component. If I have done this incorrectly, please reopen the bugs and move them to a more correct component as we don't have "embedding" APIs any more.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → INCOMPLETE
I'd like to reopen this, but I don't know where it belongs...
How about widgets?
Status: RESOLVED → REOPENED
Component: Embedding: APIs → Widget
Resolution: INCOMPLETE → ---
Or even better, Toolkit.
Component: Widget → XUL Widgets
Product: Core → Toolkit

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Severity: major → --

I'd imagine this is INACTIVE or WONTFIX but maybe you have ideas, Mike?

Flags: needinfo?(mozilla)

Yeah, I don't think this matters.

Status: REOPENED → RESOLVED
Closed: 8 years ago1 year ago
Flags: needinfo?(mozilla)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.