367533 - Dictionary Options tab has issues when there is no dictionary

Status: VERIFIED FIXED

(Thunderbird :: Preferences, defect)

Description

[Frédéric Buclin]

Attachment: screenshot of the Prefs panel

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.

Comment 3

[Marco Bonardo [:mak]]

Attachment: after installing a dictionary

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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Scott, could this be an issue with the widget?

Comment 5

[Scott MacGregor]

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.

Comment 6

[Scott MacGregor]

hijacking the summary since we aren't going to remove the psuedo drop down menu.

Comment 7

[Scott MacGregor]

Attachment: fix for the options dialog

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.

Comment 8

[Scott MacGregor]

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.

Comment 9

[Scott MacGregor]

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 10

[Scott MacGregor]

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 11

[Scott MacGregor]

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.

Comment 12

[Phil Ringnalda (:philor)]

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

[Phil Ringnalda (:philor)]

Attachment: Linux, disabled

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.

Comment 14

[Scott MacGregor]

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

[Phil Ringnalda (:philor)]

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

[Phil Ringnalda (:philor)]

Comment on attachment 262793 [details] [diff] [review]
fix for the options dialog

Meh, no sense holding this hostage because Gnomestripe is broken. r=philringnalda

Comment 17

[Scott MacGregor]

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.

Comment 18

[Scott MacGregor]

Attachment: updated fix to use setInitialSelection

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 19

[Phil Ringnalda (:philor)]

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

Comment 20

[Scott MacGregor]

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.

Comment 21

[Daniel Veditz [:dveditz]]

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

Comment 22

[Carsten Book [:Tomcat]]

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.

Comment 24

[Al Billings [:abillings - ex-MoCo]]

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

Comment 25

[Al Billings [:abillings - ex-MoCo]]

Attachment: 2.0.0.6 with Vista in Dutch (nl)

Comment 26

[Al Billings [:abillings - ex-MoCo]]

False alarm. Fixed after all.

Comment 27

[Al Billings [:abillings - ex-MoCo]]

Marking verified again.