1348259 - Default line height was changed on Nightly55.0a1

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P1)

Description

[Alice0775 White]

Attachment: Screenshot 53.0b3 VS Nightly55.0a1 VS Edge

Build Identifier:
https://hg.mozilla.org/mozilla-central/rev/ff04d410e74b69acfab17ef7e73e7397602d5a68
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0 ID:20170316030211

It seems to be affected to web layout compatibility.


Steps To Reproduce:
1. Windows10 Home x64 Japanese Edition (I do not know whether it is specific to the language edition)
2. Launch Firefox en-US build with clean profile
3. Open https://jsfiddle.net/d_toybox/Lr4hdrto/1/ (testcase of Bug 548311)

Actual Results:
Nightly55.0a1 increases default line height than Beta53.0

Expected Results:
No reason to change default line height

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b1c8b28b9fa2a8424db940d4b657eb59b3f01ff3&tochange=b45d664c0a7b10d6a54ceae884f2f8956f10bbec

Suspect: Bug 1346674

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Taking.

Thank you for reporting it and apologize for the regression.

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Ok, so here's an interesting question.

The STR list that the testing environment is a Japanese Windows, with *en-US Firefox*.

Before bug 1346674 landed, we would take the language from the OS ("ja") and ignore the Firefox language ("en-US").

After the change, we follow the app locale, not the OS locale.

First of all, this is an edge case scenario and I think it's important to recognize that. I do not have data that would indicate that users tend to diverge their OS locale from their app locale.

Once we establish that, we can talk about how such edge case should be handled.
I don't believe there's a benefit of ignoring app locale over OS locale.

The comparison with Edge is also not fair I think because I assume that Edge in this setup is "ja", not "en-US".

My conclusion is that in such a scenario we should behave the way we do in Fx 55 (after patch) and follow the app locale choice irrelevant of the OS locale choice.

If my conclusion is wrong, we can easily switch the call in gfx/thebes/gfxPlatformFontList.cpp to use OSPreferences instead of LocaleService.

:m_kato, what should we do here?

Comment 3

[Alice0775 White]

(Yes, it is an edge case, and in case of no lang attribute in the web document. So, To be honest, I can mark this as WONTFIX)

Comment 4

[Axel Hecht [:Pike]]

My suspicion is that this is a real bug that didn't show before we changed the conventions on how to get the locale.

I'd not close this without some deeper investigation.

Given this is fonts, I'd look for Jonathan here more than m_kato

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

Adding :jfkthame.

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

I'm a bit lost.

I launched Windows 10 en-US with:

 - Edge
 - Firefox 54 ja (nightly without the patch)
 - Firefox 55 ja (nightly with the patch)

And there is no difference.

What's more, in both setups the line heights in Edge (once again, en-US Edge in en-US Windows) is different than both Firefoxes.

I suspect that in order to produce a difference between Firefoxes and see the regression I'd need a ja version of Windows.

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1348259 - Switch nsLanguageAtomService to use OSPreferences::GetSystemLocale.

Review commit: https://reviewboard.mozilla.org/r/122526/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122526/

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

:Alice0775, can you verify if the patch fixes your issue?

Comment 9

[Makoto Kato [:m_kato]]

Comment on attachment 8849760 [details]
Bug 1348259 - Switch nsLanguageAtomService to use OSPreferences::GetSystemLocale.

https://reviewboard.mozilla.org/r/122526/#review124764

::: intl/locale/nsLanguageAtomService.cpp:17
(Diff revision 1)
> -#include "mozilla/intl/LocaleService.h"
> +#include "mozilla/intl/OSPreferences.h"
>  #include "nsServiceManagerUtils.h"
>  #include "mozilla/dom/EncodingUtils.h"
>  
>  using namespace mozilla;
>  using mozilla::intl::LocaleService;

Replace LocaleSerivce with OSPreferences since LocaleSerivce is unused after this fix.

Comment 10

[Makoto Kato [:m_kato]]

clear ni due to discussion by bug 1348535

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8849760 [details]
Bug 1348259 - Switch nsLanguageAtomService to use OSPreferences::GetSystemLocale.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122526/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9e5904971244
Switch nsLanguageAtomService to use OSPreferences::GetSystemLocale. r=m_kato

Comment 13

[Alice0775 White]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #8)
> :Alice0775, can you verify if the patch fixes your issue?

I verified that the problem was no longer reproduced on the latest autoland build[1].

[1]https://hg.mozilla.org/integration/autoland/rev/9e59049712448824a3717ce2d9dcbe1cf07da941
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0 ID:20170322001035

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9e5904971244