Closed Bug 62690 Opened 24 years ago Closed 24 years ago

Language picker in preferences dialog ignores double clicks

Categories

(SeaMonkey :: Preferences, defect, P3)

x86
Linux

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: cesarb, Assigned: shanjian)

Details

(Keywords: regression)

Attachments

(5 files)

From Bugzilla Helper: User-Agent: Mozilla/5.0 (X11; U; Linux 2.4.0-test10 i686; en-US; m18) Gecko/20001211 BuildID: 2000121112 It used to be (2000120808 and before) that double-clicking on a language in the list of languages to add would add that language. Now, double clicks there seems to be treated as single clicks; to add a language, I have to click and then press "OK". Reproducible: Didn't try Steps to Reproduce: 1. Do a fresh install of mozilla 2. Open Preferences, Languages, and click Add 3. Try to double-click in pt-BR Actual Results: Selected pt-BR Expected Results: Added pt-BR to the languages list Using modern skin.
confirming... not sure if this is a toolkit or i18n issue. jrgm, d'you know if this is a known issue, trying to select from list in a dialog by double-clicking [thus dismissing the dialog and adding the selection to the parent dialog/window]?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Um, I don't see double-clicks doing anything in a build before 1208. Looking (briefly) at the code in ./pref-languages[-add].(js|xul), there is no handler defined or registered and I don't see that this code has changed in a while. Since double-click works in other trees, this just looks like a bit of UI with no implementation (perhaps none was intended to be added, but adding the double click would be a (small) gain in usability). Who owns this dialog?
matt or nhotta? someone else?
Assignee: matt → nhotta
Reassign to tao.
Assignee: nhotta → tao
Naoki: This is the same type Accept-Language UI bug we discussed earlier. Whoever owns Jurai's bugs now will own this one, too. Back to you :-)
Assignee: tao → nhotta
Linux only? I don't see this problem with Windows RTM.
Status: NEW → ASSIGNED
I mean on windows double-click does not add the language to the list.
The assertion in the initial bug report was that double-click _used_ to add the treeitem to the active list. [I don't believe that's true (see above), but we haven't heard back from cesarb]. Nevertheless, it's still a reasonable bit of UI -- that double-click should add the selected item to the active list, instead of doing nothing. So, it's an RFE.
Yes, it used to work. That's how I found the problem - I do a clean install of mozilla every time I install a new nightly, and follow the same sequence of steps every time to configure it to the way I like. This includes adding pt-br and pt to the list of languages, and I got used to do that by double-clicking.
Shanjian, could you take a look at this. I think you are making changes to this currently, please try if we can enable double-click to add a language.
Assignee: nhotta → shanjian
Status: ASSIGNED → NEW
Attached patch proposed patch, part 1 (deleted) — Splinter Review
Attached patch proposed patch, part 2 (deleted) — Splinter Review
naoki, can you do a code review?
Status: NEW → ASSIGNED
+ var Languagename = new String(); + var Languageid = new String(); Are these necessary? Can you just assign from getAttribute? + + Languagename = node.getAttribute('value'); + Languageid = node.getAttribute('id'); Indent is not right.
Do you really want to do |window.close| after adding the item on the double-click? More commonly, this form of UI allows a user to add multiple items, and then call close manually. (Cheap opinions from the peanut gallery).
Naoki, The 2 assignment might not be necessary, but they will not do any harm either. John, Yes, window.close is necessary here. In most cases, use just want to add one languages, and double click is a shortcut only for this purpose. Otherwise they would click add after selecting multiple items.
r=nhotta >The 2 assignment might not be necessary If not necessary then please simplify.
Matthew, what do you think of that double-click discussion? + function HandleDoubleClick(event, node) The event is unused, please don't pass it in. + var Languagename = new String(); + var Languageid = new String(); As Naoki mentioned, these are unnecessary. Please just do it directly. Also, please follow our typical variable naming convention if you don't mind (e.g. languageName instead of Languagename). + return true; This is unnecessary. Also, it looks like we don't need to retrieve the languageName unless !LangAlreadyActive(Languageid), so please move that into the |if| Thanks.
I can't see the relevant dialog. Double clicking should only close a dialog if the widget you are manipulating is very likely to be the only control you need to change before the dialog is closed. (An example of this is the Shut Down dialog in Windows 9x, where there's nothing else to change besides the radio buttons.) If this is a standalone dialog with just one tree widget (and `Add' and `Cancel' buttons), then closing on double click is fine.
Attached patch new patch, part 1 (deleted) — Splinter Review
Attached patch new patch , part 2 (deleted) — Splinter Review
who can give me sr=?
r=nhotta for new patch, you can e-mail erik for a super review and cc to reviewers@mozilla.org.
Thanks for making those changes. One more minor point that isn't worth attaching a new patch... ! datasources="rdf:null" multiple="true" ondblclick="return HandleDoubleClick(event.target)"> Generally, the only reason you'd want to return the value in an event handler is to possibly return false and prevent the default action from happening. Since that isn't the case here, this can really just be ondblclick="HandleDoubleClick(event.target);" (with semicolon). Thanks!
you don't need to do a getAttribute of 'id', just node.id works fine. Also, it may be worth determining whether or not the event.target is the correct type of node before going off and doing something with that data. It's possible to double click on other parts of the tree (empty space, headers) and have getAttribute return an empty string and have the code continue executing with that data (which is probably undesirable).
Attached patch revised part2 fix . (deleted) — Splinter Review
Ben pointed out a good point in his comment. If I double click scroll bar area, the function will put a undesired item to accepted languages list. Since all of our language items has value associated with them, I am using value to verify node. Not sure if there is any better way, but my current code sounds good enough.
seems reasonable, a=ben
fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
vrfy fixed on the 3 main platforms using comm branch bits, 2001.06.07.0x.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: