Last Comment Bug 345067 - Issues with prompt service's confirmEx - confirmEx always returns 1 when user closes dialog window using the X button in titlebar
: Issues with prompt service's confirmEx - confirmEx always returns 1 when user...
Status: REOPENED
:
Product: Toolkit
Classification: Components
Component: XUL Widgets (show other bugs)
: Trunk
: All All
: -- major with 6 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
: Neil Deakin
Mentors:
: 329414 366985 (view as bug list)
Depends on:
Blocks: 384641 521158
  Show dependency treegraph
 
Reported: 2006-07-18 10:09 PDT by Christian :Biesinger (don't email me, ping me on IRC)
Modified: 2016-09-07 20:49 PDT (History)
25 users (show)
vladimir: blocking1.9-
reed: wanted1.9+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
very rough patch (4.04 KB, patch)
2011-08-30 10:01 PDT, Mike Kaply [:mkaply]
dolske: feedback-
Details | Diff | Splinter Review

Description Christian :Biesinger (don't email me, ping me on IRC) 2006-07-18 10:09:54 PDT
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.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2007-01-14 22:36:57 PST
*** Bug 366985 has been marked as a duplicate of this bug. ***
Comment 2 Marc Diethelm 2008-01-05 05:47:24 PST
Documented it on DevMo at:

http://developer.mozilla.org/en/docs/nsIPromptService#confirmEx
Comment 3 Wayne Mery (:wsmwk, NI for questions) 2008-03-16 15:37:02 PDT
Would changing the behavior negatively impact any users of confirmEx?

elevating to sev=major given bug 380862.

is bug 329414 a duplicate?
Comment 4 Christian :Biesinger (don't email me, ping me on IRC) 2008-03-17 06:29:08 PDT
(In reply to comment #3)
> Would changing the behavior negatively impact any users of confirmEx?

Depends on how we fix this,,,
Comment 5 Henrik Skupin (:whimboo) 2008-03-18 04:58:44 PDT
*** Bug 329414 has been marked as a duplicate of this bug. ***
Comment 6 Pooya Karimian - Sxip (:pooya) 2008-04-24 15:01:51 PDT
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.
Comment 7 Arivald 2010-05-18 06:25:15 PDT
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?
Comment 8 Tyler Breisacher 2011-01-07 15:38:27 PST
(In reply to comment #7)
> Or maybe I should fill another bug for this problem?

I've filed one for you here: Bug 624043
Comment 9 Mike Kaply [:mkaply] 2011-08-30 08:12:45 PDT
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.
Comment 10 Benjamin Smedberg [:bsmedberg] 2011-08-30 08:24:49 PDT
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.
Comment 11 Mike Kaply [:mkaply] 2011-08-30 08:29:30 PDT Comment hidden (typo)
Comment 12 Mike Kaply [:mkaply] 2011-08-30 08:32:06 PDT
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.
Comment 13 Mike Kaply [:mkaply] 2011-08-30 10:01:26 PDT
Created attachment 556890 [details] [diff] [review]
very rough patch

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.
Comment 14 Justin Dolske [:Dolske] 2011-09-01 00:12:28 PDT
(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 15 Justin Dolske [:Dolske] 2011-09-01 01:04:43 PDT
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...
Comment 16 Mike Kaply [:mkaply] 2011-09-01 07:34:27 PDT
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?
Comment 17 Justin Dolske [:Dolske] 2011-09-11 16:00:42 PDT
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.
Comment 18 Thomas D. (needinfo?me) 2013-10-18 13:43:07 PDT
Ops, midair mis-click, sorry.
Comment 19 Benjamin Smedberg [:bsmedberg] 2016-06-23 10:47:09 PDT
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.
Comment 20 Mike Kaply [:mkaply] 2016-07-07 07:35:20 PDT
I'd like to reopen this, but I don't know where it belongs...
Comment 21 Milan Sreckovic [:milan] 2016-07-11 09:03:43 PDT
How about widgets?
Comment 22 Milan Sreckovic [:milan] 2016-07-11 09:06:15 PDT
Or even better, Toolkit.

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