1140720 - Error reading font prefs in the Slovenian locale

Status: VERIFIED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[peter_klofutar]

Attachment: Chat window with active chat.PNG

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 20150305021524

Steps to reproduce:

Installed Thunderbird Aurora 38


Actual results:

When starting a chat with a contact, the bottom bar for entering text doesn't appear.


Expected results:

Box should have appeared, so entering text is possible.  The problems appears in Slovenian locale.  I tested English-US and chat worked fine.

Comment 1

[Patrick Cloke [:clokep]]

The screenshot you show is when you're clicked on a group, which we wouldn't expect to have the box there. If you double click on a contact do you see the conversation box show up?

(I can't read the text there so I'm unsure exactly what state the chat window is in.)

Comment 2

[peter_klofutar]

When I double click on a contact, I get a bar on the bottom, that says "Start a conversation with <contact>".  When I click on it, the text box doesn't show up.

Comment 3

[aleth [:aleth]]

Can you take a look in the error console and see if something relevant appears there when you click the bar?

Comment 4

[peter_klofutar]

I get the following errors:

Časovni žig: 9.3.2015 8:56:15
Napaka: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getIntPref]
Izvorna datoteka: resource://gre/modules/imTextboxUtils.jsm
Vrstica: 33

Časovni žig: 9.3.2015 8:56:17
Napaka: TypeError: item.convView is null
Izvorna datoteka: chrome://messenger/content/chat/chat-messenger-overlay.js
Vrstica: 536

Comment 5

[aleth [:aleth]]

OK, so it fails trying to fetch the appropriate font.size.variable.* pref.

What's the value of the font.language.group pref in this locale?

Comment 6

[aleth [:aleth]]

Also, is this a regression, i.e. did this work in a previous version in the Slovenian locale?

Comment 7

[peter_klofutar]

The value of that pref. is x-central-euro.  This worked in Aurora 36, problems started, when it updated to 37.

Comment 8

[aleth [:aleth]]

That's strange, font.size.variable.x-central-euro should be set by default.

Comment 9

[aleth [:aleth]]

Can you check the value of that pref? 
Does the problem happen with all protocols or just one (XMPP, IRC, ...)?

What's puzzling me is that it's not clear to me why that pref value should be accessed at all in TB.

Comment 10

[:aceman]

(In reply to aleth [:aleth] from comment #8)
> That's strange, font.size.variable.x-central-euro should be set by default.

It is not set for me either (can't be seen via TB's config editor).

And Options->Display->Formatting->Default font field is empty (and also the size). I think that is reported somewhere else. The Preferences dialog allows me to set up the fonts and sizes (also the serif and monospace are missing). And then the pref font.size.variable.x-central-euro is set to the non-default value I have chosen.

Comment 11

[aleth [:aleth]]

(In reply to :aceman from comment #10)
> (In reply to aleth [:aleth] from comment #8)
> > That's strange, font.size.variable.x-central-euro should be set by default.
> 
> It is not set for me either (can't be seen via TB's config editor).
> 
> And Options->Display->Formatting->Default font field is empty (and also the
> size). I think that is reported somewhere else. The Preferences dialog
> allows me to set up the fonts and sizes (also the serif and monospace are
> missing). And then the pref font.size.variable.x-central-euro is set to the
> non-default value I have chosen.

So it looks like the problem is that the default values are currently only set for OSX: 

https://dxr.mozilla.org/comm-central/source/mail/app/profile/all-thunderbird.js#590

And x-central-euro doesn't seem to exist at all for Firefox.

Do you know someone knowledgeable about l10n/fonts who would know what the correct fix would be here?

Comment 12

[peter_klofutar]

It happens also on IRC.

Comment 13

[aleth [:aleth]]

(In reply to peter_klofutar from comment #12)
> It happens also on IRC.

Right, since comment 11 it's clear why it's happening now, registerTextbox calls getValues in imTextboxUtils and that tries to get the nonexistent default value of the pref.

Comment 14

[aleth [:aleth]]

Can you tell me what value font.language.group has in Slovenian Firefox? (I hope it matches, but it's a bit surprising that Firefox seems to be missing some default values too).

Comment 15

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #14)
> Can you tell me what value font.language.group has in Slovenian Firefox? (I
> hope it matches, but it's a bit surprising that Firefox seems to be missing
> some default values too).

In fact x-central-euro no longer exists in Firefox since bug 756022, which was not ported to c-c at the time.

Comment 16

[aleth [:aleth]]

Attachment: Minimal possible temporary fix - adds back some prefs removed from gecko

Bug 756022 removed three language groups, this patch restores the missing font.size prefs as a workaround, which should be enough for the narrow cause of this bug.

Of course that's not really the correct fix, which would be to fully port bug 756022, which is nontrivial in that it's likely easy to miss something unless one is familiar with the way TB handles charsets etc, which I am not. However, there's probably a whole set of other bugs caused by mismatches due to those language groups being removed from gecko but not TB (as hinted at by aceman above). The question is whether it isn't too late to do this for TB38.

Needinfoing rkent to raise awareness of this issue.

Comment 17

[peter_klofutar]

(In reply to aleth [:aleth] from comment #14)
> Can you tell me what value font.language.group has in Slovenian Firefox? (I
> hope it matches, but it's a bit surprising that Firefox seems to be missing
> some default values too).
It's the same.

Comment 18

[aleth [:aleth]]

(In reply to peter_klofutar from comment #17)
> (In reply to aleth [:aleth] from comment #14)
> > Can you tell me what value font.language.group has in Slovenian Firefox? (I
> > hope it matches, but it's a bit surprising that Firefox seems to be missing
> > some default values too).
> It's the same.

If this is true for new profiles, that's a bug in the Firefox Slovenian l10n, since https://dxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/intl.properties#35 clearly states it must be one of the values in https://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/fonts.xul#50 and x-central-euro no longer exists there.

Comment 19

[Kent James (:rkent)]

I'm really not the right guy to discuss character set issues. Joshua and Magnus, is this likely to be a serious issue in TB 38? Can we backport a change, or does it affect localization?

Comment 20

[Henri Sivonen (:hsivonen) (not reading bugmail until 2025-02-06)]

(In reply to Kent James (:rkent) from comment #19)
> Can we backport a
> change, or does it affect localization?

https://hg.mozilla.org/mozilla-central/rev/2a9dfb605be4 introduced new strings. Still, c-c really needs to backport this.

Comment 21

[Kent James (:rkent)]

The proposed patch could be added to TB 38 with no ramifications AFAICT, so we'll consider taking it when reviewed.

Comment 22

[peter_klofutar]

(In reply to aleth [:aleth] from comment #18)
>
> If this is true for new profiles, that's a bug in the Firefox Slovenian
> l10n, since
> https://dxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/
> global/intl.properties#35 clearly states it must be one of the values in
> https://dxr.mozilla.org/mozilla-central/source/browser/components/
> preferences/fonts.xul#50 and x-central-euro no longer exists there.

It's the same for new profiles.

Comment 23

[Joshua Cranmer [:jcranmer]]

(In reply to Kent James (:rkent) from comment #19)
> I'm really not the right guy to discuss character set issues. Joshua and
> Magnus, is this likely to be a serious issue in TB 38? Can we backport a
> change, or does it affect localization?

I think hsivonen answered this question. If they're changing how some of the fonts setting works, we need to change our dialog to conform to those changes, not paper it over with pref changes that will mean nothing internally.

Comment 24

[Kent James (:rkent)]

Sure, let's backport this to c-c, but do we have to add a late string at this point, or can we just accept the pref?

Comment 25

[aleth [:aleth]]

Attachment: Port bug 756022

Port of bug 756022.

On its own this is not enough - it's missing the charset aspects of the removal of x-central-euro/x-baltic/tr. I don't know enough about how charsets relate to email encodings to be confident in doing this part.

There's also bug 910192 which should likely be ported at the same time for consistency.

If we take this for 38, we will have to tell localizers about the late strings and the langgroup changes.

Comment 26

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #25)
> Created attachment 8577400 [details] [diff] [review]
> Port bug 756022
> 
> Port of bug 756022.
> 
> On its own this is not enough - it's missing the charset aspects of the
> removal of x-central-euro/x-baltic/tr. I don't know enough about how
> charsets relate to email encodings to be confident in doing this part.

i.e. is naively replacing the removed groups with x-western in mailnews/intl/charsetData.properties the right thing to do?

Comment 27

[aleth [:aleth]]

Attachment: Port bug 756022 v2

On second thoughts, I added the naive changes. I suppose I'm just concerned I'm missing non-obvious consequences of this shift.

Comment 28

[aleth [:aleth]]

Attachment: Port bug 756022 v3

I looked at porting bug 910192, but discovered that while TB's fonts.xul references the obsolete intl.charset.default, it doesn't actually use it.

So it appears there is nothing to port. Sorry for the bugspam!

Instead of intl.charset.* and the associated Text Encoding groupbox, TB has its own charset prefs. Please check that the recent gecko changes in the linked bugs don't require some sort of parallel changes there.

Comment 29

[aleth [:aleth]]

Attachment: Port bug 756022 v4

Fix missed entity name change.

Comment 30

[aleth [:aleth]]

Attachment: Port bug 756022 v5

Typo fix. Sorry, clearly not my best day for patch QA...

Comment 31

[aleth [:aleth]]

(In reply to peter_klofutar from comment #22)
> (In reply to aleth [:aleth] from comment #18)
> >
> > If this is true for new profiles, that's a bug in the Firefox Slovenian
> > l10n, since
> > https://dxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/
> > global/intl.properties#35 clearly states it must be one of the values in
> > https://dxr.mozilla.org/mozilla-central/source/browser/components/
> > preferences/fonts.xul#50 and x-central-euro no longer exists there.
> 
> It's the same for new profiles.

And indeed, despite this being a change in gecko 34, http://hg.mozilla.org/l10n-central/sk/file/4887034a4291/toolkit/chrome/global/intl.properties#l41 has not been updated.

This might have happened becaue the pref name did not change, so localizers were not aware they had to update this. Hsivonen, it may be worth reaching out to the affected locales?

Comment 32

[Magnus Melin [:mkmelin]]

Comment on attachment 8577459 [details] [diff] [review]
Port bug 756022 v5

Review of attachment 8577459 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good afikt, r=mkmelin

Comment 33

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/58ace5521e3d

Comment 34

[aleth [:aleth]]

Comment on attachment 8577459 [details] [diff] [review]
Port bug 756022 v5

[Approval Request Comment]
Regression caused by (bug #): 756022
Risk to taking this patch (and alternatives if risky): see earlier discussion in the bug

Comment 35

[aleth [:aleth]]

Note the bug mentioned in the bug description won't actually be fixed by this patch unless the m-c localizers for the affected locales all fix the font.language.group pref value.

smontagu: As you wrote the original patch, and as the incorrect pref values affect Firefox too and the next merge date is drawing close, I thought I'd better needinfo you about this as well.

Comment 37

[:aceman]

(In reply to aleth [:aleth] from comment #26)
> > On its own this is not enough - it's missing the charset aspects of the
> > removal of x-central-euro/x-baltic/tr. I don't know enough about how
> > charsets relate to email encodings to be confident in doing this part.
> 
> i.e. is naively replacing the removed groups with x-western in
> mailnews/intl/charsetData.properties the right thing to do?

It seems my central european language also got replaced with "x-western", when I look in FF 36.

Comment 38

[Simon Montagu :smontagu]

(In reply to aleth [:aleth] from comment #26)
> > On its own this is not enough - it's missing the charset aspects of the
> > removal of x-central-euro/x-baltic/tr. I don't know enough about how
> > charsets relate to email encodings to be confident in doing this part.
> 
> i.e. is naively replacing the removed groups with x-western in
> mailnews/intl/charsetData.properties the right thing to do?

That was the goal of bug 756022 -- to merge all the removed groups into x-western

Comment 39

[aleth [:aleth]]

(In reply to Simon Montagu :smontagu from comment #38)
> (In reply to aleth [:aleth] from comment #26)
> > > On its own this is not enough - it's missing the charset aspects of the
> > > removal of x-central-euro/x-baltic/tr. I don't know enough about how
> > > charsets relate to email encodings to be confident in doing this part.
> > 
> > i.e. is naively replacing the removed groups with x-western in
> > mailnews/intl/charsetData.properties the right thing to do?
> 
> That was the goal of bug 756022 -- to merge all the removed groups into
> x-western

Right, that wasn't why I needinfo'd you though. The problem is that some of the locales affected by this change (e.g. sk, ro, there may be more) never changed the value of the font.language.group pref accordingly. They should be given a heads-up or someone with the requisite privileges should make the change for them.

Comment 40

[Simon Montagu :smontagu]

Ah, OK. Opened bugs against the relevant l10ns:

http://mxr.mozilla.org/l10n-central/search?string=x-central-euro&tree=l10n-central
cs - Bug 1146172 
hu - Bug 1146173
pl - Bug 1146174
ro - Bug 1146176
sk - Bug 1146177
sl - Bug 1146178 
sq - Bug 1146179

http://mxr.mozilla.org/l10n-central/search?string=x-baltic&tree=l10n-central
lt - Bug 1146180
lv - Bug 1146181 

(tr already uses x-western)

Comment 41

[aleth [:aleth]]

smontagu: Sorry to bother you again, but I was wondering what the process would be to get the changes made in the bugs you filed in comment #40 uplifted to aurora (38)? This would be very valuable as the next ESR release will be based off 38 (for Thunderbird, which only has ESR releases, but also for Firefox).

Comment 42

[aleth [:aleth]]

Ah, looking at some of those bugs that have been fixed, it looks like the localizers have already taken care of it :-)

Comment 43

[Kent James (:rkent)]

Comment on attachment 8577459 [details] [diff] [review]
Port bug 756022 v5

https://hg.mozilla.org/releases/comm-aurora/rev/db9997946ebd

Comment 44

[aleth [:aleth]]

Peter, could you verify this is fixed next time you get an update of Slovenian TB38, please? Thanks!

Comment 45

[Francesco Lodolo [:flod]]

I don't really work on Thunderbird (nor I localize it for my language), but breaking string freeze on the last week of the Aurora cycle is far from acceptable. 

Even more considering that 38 is the ESR release, and some locales will have to stick with these untranslated strings, in a visible part of the UI, for 6 months.

Comment 46

[Henri Sivonen (:hsivonen) (not reading bugmail until 2025-02-06)]

While at it, I suggest implementing a Thunderbird/SeaMonkey equivalent of bug 1147311.

Comment 47

[aleth [:aleth]]

(In reply to Henri Sivonen (:hsivonen) from comment #46)
> While at it, I suggest implementing a Thunderbird/SeaMonkey equivalent of
> bug 1147311.

Thanks, good idea!

Comment 48

[peter_klofutar]

(In reply to aleth [:aleth] from comment #44)
> Peter, could you verify this is fixed next time you get an update of
> Slovenian TB38, please? Thanks!

It's working now, thanks (TB 39).