Closed Bug 228017 Opened 20 years ago Closed 20 years ago
UI Consistency: "make default search engine" dialog's button choice is counter-intuitive
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
Fixes what this bug report is about.
Assignee: search → durbacher
Status: UNCONFIRMED → ASSIGNED
Comment on attachment 145805 [details] [diff] [review] patch 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.
Comment on attachment 145805 [details] [diff] [review] patch Wait a minute, yes/no logic is reversed...
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).
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?(neil.parkwaycc.co.uk)
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?(neil.parkwaycc.co.uk) → 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+
alecf: 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: http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/navigator.js#2301 http://lxr.mozilla.org/seamonkey/source/editor/ui/composer/content/ComposerCommands.js#2101 http://lxr.mozilla.org/seamonkey/source/xpfe/components/winhooks/nsWindowsHooks.cpp#447 http://lxr.mozilla.org/seamonkey/source/mailnews/base/prefs/resources/content/AccountWizard.js#178 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 <-- search-panel.js new revision: 1.83; previous revision: 1.82 done
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.