Closed Bug 228017 Opened 21 years ago Closed 21 years ago

UI Consistency: "make default search engine" dialog's button choice is counter-intuitive


(SeaMonkey :: Search, defect)

Not set


(Not tracked)



(Reporter: chris, Assigned: durbacher)



(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20031007
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20031007

When choosing a non-default search engine from the sidebar search feature, the
user is presented with a dialog "Change default search engine?" with text "Would
you like to make %s your default search engine?".  This is a yes/no question,
and the user should be presented with a yes/no response.  Instead, this dialog
has the "OK/Cancel" button choice.  The meaning of OK/Cancel in this context is
unclear, and will potentially confuse novice users.

Reproducible: Always

Steps to Reproduce:
1. Choose non-default search engine in sidebar search feature.
Actual Results:  
User presented with OK/Cancel

Expected Results:  
User presented with Yes/No
Attached patch patch (obsolete) — Splinter Review
Fixes what this bug report is about.
Assignee: search → durbacher
OS: Windows XP → All
Hardware: PC → All
Comment on attachment 145805 [details] [diff] [review]

Asking Neil for review (or wontfix or whatever).

Bug 115852 is about why this dialog is generally bad, but I don't have the time
to fix that one.
Attachment #145805 - Flags: review?(
Comment on attachment 145805 [details] [diff] [review]

Wait a minute, yes/no logic is reversed...
Attachment #145805 - Flags: review?(
Attached patch correct patchSplinter Review
Stupid me, confirmEx returns the pressed button, of course.
This patch does it correctly (also, when the dialog close box in the upper
right corner is pressed, also "1" for "No" is returned, as one would expect).
Attachment #145805 - Attachment is obsolete: true
Comment on attachment 145808 [details] [diff] [review]
correct patch

Requesting r= from Neil (see comment #2).

(You would not rather like a hack like:
change = !promptSvc.confirmEx([...]
or something similar confusing boolean and int vars, would you?)
Attachment #145808 - Flags: review?(
Comment on attachment 145808 [details] [diff] [review]
correct patch

No, your test is fine, given the state of the rest of the code in that function
Attachment #145808 - Flags: review?( → review+
Comment on attachment 145808 [details] [diff] [review]
correct patch

Requesting sr= from alecf.
Attachment #145808 - Flags: superreview?(alecf)
Comment on attachment 145808 [details] [diff] [review]
correct patch

ugh, I think this should be &, not + - obviously it doesn't make a difference
in this case, but this is a bitmask, right?
sr=alecf with & or with a good explanation as to why you're using +
Attachment #145808 - Flags: superreview?(alecf) → superreview+
My "good reason" is that ConfirmEx seems to be only - or at least in the vast
mayority of cases - used in this manner ("+").
I just asked lxr and the first ten occurrences (all over the codebase) all had a
"+", none a "&".
I think I don't need to list all of them here, just for starters:

After thinking about it, It's clear that "&" makes more sense, but I'm not able
to find anybody using it like this, so maybe it should be left as it is, for
consistency. (I don't remember, is consistency with bad things good?)
What do you think?
I checked in as-is for consistency with the rest of the code...
(you two did mean | when you said &, right?)

Checking in xpfe/components/search/resources/search-panel.js;
/cvsroot/mozilla/xpfe/components/search/resources/search-panel.js,v  <-- 
new revision: 1.83; previous revision: 1.82
Closed: 21 years ago
Resolution: --- → FIXED
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.