Closed
Bug 367533
Opened 18 years ago
Closed 17 years ago
Dictionary Options tab has issues when there is no dictionary
Categories
(Thunderbird :: Preferences, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
Thunderbird 3
People
(Reporter: LpSolit, Assigned: mscott)
References
(Blocks 1 open bug)
Details
(Keywords: verified1.8.1.4)
Attachments
(5 files, 1 obsolete file)
(deleted),
image/png
|
Details | |
(deleted),
image/jpeg
|
Details | |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
philor
:
review+
dveditz
:
approval1.8.1.4+
|
Details | Diff | Splinter Review |
(deleted),
image/jpeg
|
Details |
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.
Updated•18 years ago
|
OS: Linux → All
Hardware: PC → All
Assignee | ||
Updated•18 years ago
|
Target Milestone: --- → Thunderbird 3
Comment 3•18 years ago
|
||
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
Comment 4•18 years ago
|
||
Scott, could this be an issue with the widget?
Assignee | ||
Comment 5•18 years ago
|
||
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
Assignee | ||
Comment 6•18 years ago
|
||
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
Assignee | ||
Comment 7•18 years ago
|
||
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.
Attachment #262793 -
Flags: review?(philringnalda)
Assignee | ||
Comment 8•18 years ago
|
||
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.
Assignee | ||
Comment 9•18 years ago
|
||
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.
Assignee | ||
Comment 10•18 years ago
|
||
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.
Attachment #262793 -
Attachment is obsolete: true
Attachment #262793 -
Flags: review?(philringnalda)
Assignee | ||
Comment 11•18 years ago
|
||
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.
Attachment #262793 -
Attachment is obsolete: false
Attachment #262793 -
Flags: review?(philringnalda)
Comment 12•18 years ago
|
||
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.
Comment 13•18 years ago
|
||
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.
Assignee | ||
Comment 14•18 years ago
|
||
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...
Comment 15•18 years ago
|
||
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 16•18 years ago
|
||
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+
Assignee | ||
Comment 17•18 years ago
|
||
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.
Assignee | ||
Comment 18•18 years ago
|
||
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.
Attachment #262793 -
Attachment is obsolete: true
Attachment #263296 -
Flags: review?(philringnalda)
Comment 19•18 years ago
|
||
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+
Assignee | ||
Comment 20•18 years ago
|
||
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: approval1.8.1.4?
Comment 21•18 years ago
|
||
Comment on attachment 263296 [details] [diff] [review]
updated fix to use setInitialSelection
approved for 1.8.1.4, a=dveditz for release-drivers, must land within the next couple of hours
Attachment #263296 -
Flags: approval1.8.1.4? → approval1.8.1.4+
Assignee | ||
Updated•18 years ago
|
Comment 22•18 years ago
|
||
verified fixed 1.8.1.4 using Mozilla/5.0 (Windows; U; Windows NT 6.0; de; rv:1.8.1.4pre) Gecko/20070507 Thunderbird/2.0.0.4pre ID:2007050703 - looks good on this version of Thunderbird, much better than before the patch.
Keywords: fixed1.8.1.4 → verified1.8.1.4
Comment 24•17 years ago
|
||
This is back.
Found again in the localized 2.0.0.6 builds: Mozilla/5.0 (Windows; U; Windows NT 6.0; nl; rv:1.8.1.6) Gecko/2007072817 Thunderbird/2.0.0.6
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 25•17 years ago
|
||
Comment 26•17 years ago
|
||
False alarm. Fixed after all.
Status: REOPENED → RESOLVED
Closed: 18 years ago → 17 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•