Closed Bug 367533 Opened 14 years ago Closed 14 years ago
Dictionary Options tab has issues when there is no dictionary
24.46 KB, image/png
20.29 KB, image/jpeg
8.18 KB, image/png
2.69 KB, patch
|Details | Diff | Splinter Review|
32.57 KB, image/jpeg
When installing a non-english version of Tb2b2, no dictionary is provided by default. Going to the Prefs panel to the dictionary tab, you can see a pseudo dropdown menu which should not be displayed at all. Or have some better UI for this menu.
i can confirm this, moreover even if you install a dictionary the dropdown grows in length but not in height, it appears as a clickable bar. You have to click and select the installed dictionary, while probably it should automatically select the only dictionary installed
Scott, could this be an issue with the widget?
Here's what I think the first run experience should look like when there are no dictionaries installed: Options Dialog: Language Menu list should be disabled and it should have a fixed width like the directory server in the addressing tab. Bonus points if we insert the word 'None' or something similar like we do for the directory server field. Compose Window: The menu drop down for the Spell Check toolbar button should have a menu item for downloading more dictionaries. Standalone Spell Check Dialog: Right now the Languages box is blank, there's a menu item option in that menu list for downloading more dictionaries but you have to open it and select it. I'm not sure what to do for this dialog.
Status: NEW → ASSIGNED
hijacking the summary since we aren't going to remove the psuedo drop down menu.
Summary: Remove the pseudo dropdown menu from the Prefs panel when there is no dictionary installed → Dictionary Options tab has issues when there is no dictionary
I think this makes the experience for the options dialog a lot better when there are no dictionaries installed and there is no value for spellchecker.dictionaries which is quite common for non en-US builds. 1) Add flex to the menu list to ensure a width when we have no dictionaries. 2) Disable the menu list when there are no dictionaries. 3) When we have one or more dictionaries installed, but we don't have a value for spellchecker.dictionary, forcibly set spellchecker.dictionary to have the value of the first dictionary in the list. This fixes the edge case where a user installs a dictionary, brings up the options dialog and sees the dictionary menu list without any selection. It has a side effect of forcing the first dictionary to be used in the compose window too (if you open the options dialog). We'll want to add compose window logic to do this as well. I'll tackle the compose window next but this gets us started. Asking Phil for a code review to see what he thinks.
To test this patch: 1) Remove all files in <path to exe>\dictionaries. 2) In about:config, make sure spellchecker.dictionary has no value. That's how you can simulate not having a dictionary installed. Verify that the menu list is disabled. 3) Now install a dictionary and restart. 4) Open up the spellcheck panel in the options UI and verify that your dictionary is selected. Hit OK 4) Now bring up the compose window and verify that the newly installed dictionary is used for inline spell check.
We're going to be more limited in what we can do for the compose window on the branch because we can't add any new strings so we can't do things like add a menu item in the toolbar popup for downloading dictionaries.
Comment on attachment 262793 [details] [diff] [review] fix for the options dialog I think the logic for this patch can change a bit once Bug 378795 lands.
Comment on attachment 262793 [details] [diff] [review] fix for the options dialog sorry for the bug noise, I still think this patch is valuable and would like to get Phil's thoughts on it.
ETA around six hours - the only thing I see offhand is that it took me some puzzling to realize that "// at this point we know we have at least one dictionary in the list" because this.mDictCount == count when they're both 0, but I'd like to actually build on Linux, where I think the <menulist> CSS is a little lacking, and the flex may not be enough to keep it from looking broken when it's empty.
The code looks good (better than what I had half-sketched, for sure), though as you say we should see where 378795 goes first. But, even though it's not your fault that an empty <menulist> doesn't have a min-height on Linux, could you give it one here, please? Even disabled and flexed wide, that still looks like something broken, not like something empty and disabled because I need to install a dictionary.
That Linux issue seems weird. I wonder if we suffer from it (or have worked around it) in other parts of the app like account settings...
The first two that occur to me are LDAP when you don't have any servers defined (which has a "None" element) and server junk headers when you've removed all your .sfd files (I'm away from my Linux machine, but I'm guessing that will collapse, too). Needs either a toolkit or widget bug (which won't help for 2.0.0.next), though I'm not sure which: pinstripe has an explicit 20px min-height in menulist.css, winstripe must be getting its from widget for moz-appearance: menulist, and not sharing with gnomestripe.
Comment on attachment 262793 [details] [diff] [review] fix for the options dialog Meh, no sense holding this hostage because Gnomestripe is broken. r=philringnalda
Attachment #262793 - Flags: review?(philringnalda) → review+
I can't find any other menulists where we are applying a min height for the linux issue, I was hoping to use the same height value if we've worked around this problem elsewhere. I'll keep digging.
Phil, I made a minor tweak to the patch to use the setInitialSelection method on a menulist. This routine does the work for us for setting the selection based on the value of the menu list or, if no item with that value was found, it selects the first item in the list.
Comment on attachment 263296 [details] [diff] [review] updated fix to use setInitialSelection So, the magic in that is that the preference binding has set the menulist value, while it didn't have any menuitems to select, but it's either a bug that appendItem doesn't check to see whether the appended item should be selected because of the menulist value, or a totally undocumented feature that when you dynamically create a menulist, you're responsible for calling setInitialSelection? Learn something new every day :) r=philringnalda
Attachment #263296 - Flags: review?(philringnalda) → review+
Comment on attachment 263296 [details] [diff] [review] updated fix to use setInitialSelection This effects just Thunderbird and makes the experience better when you run without any dictionaries installed.
Attachment #263296 - Flags: approval126.96.36.199?
Comment on attachment 263296 [details] [diff] [review] updated fix to use setInitialSelection approved for 188.8.131.52, a=dveditz for release-drivers, must land within the next couple of hours
Attachment #263296 - Flags: approval184.108.40.206? → approval220.127.116.11+
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
verified fixed 18.104.22.168 using Mozilla/5.0 (Windows; U; Windows NT 6.0; de; rv:22.214.171.124pre) Gecko/20070507 Thunderbird/126.96.36.199pre ID:2007050703 - looks good on this version of Thunderbird, much better than before the patch.
This is back. Found again in the localized 188.8.131.52 builds: Mozilla/5.0 (Windows; U; Windows NT 6.0; nl; rv:184.108.40.206) Gecko/2007072817 Thunderbird/220.127.116.11
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
False alarm. Fixed after all.
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Marking verified again.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.