Closed
Bug 1344990
Opened 8 years ago
Closed 8 years ago
Needs "auto" pseudo font family to specify default font in font setting dialog
Categories
(Core :: Graphics: Text, enhancement)
Core
Graphics: Text
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: masayuki, Assigned: masayuki)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(4 files, 4 obsolete files)
Currently, we cannot specify some fonts as default font even if the most preferred font is not installed in all OS versions which we support.
E.g., Starting Windows 8.1, "Yu Gothic" and "Yu Mincho" are installed and they have good glyph. However, if we specify them as default font, Win7 user (perhaps) will see blank dropdown list in the font dialog in Options and the default font will be selected dynamically with "font.name-list.*" at rendering.
We're discussing about new Japanese default font in bug 548311. And even if we will take "Meiryo" which is installed Win7, Win8.1 and Win10 for Japanese, we need a way to specify multiple fonts to default font of Japanese Serif font setting and default font of non-Japanese Win10 (non-Japanese Win10 has only "Yu Gothic").
Therefore, I'd like to suggest "auto" pseudo font family in font dialog of Options. This is NOT available in CSS even in chrome. Detail of my idea is:
> 1. When "font.name.(serif|sans-serif|monospace|cursive).*" is empty string, we should treat it as auto and make all of them empty.
> 2. Add "font.auto-list.(serif|sans-serif|monospace|cursive).*" which includes one or more font families as comma separated list.
> 3. gfx should find default font with following steps:
> 3.1 If "font.name.(serif|sans-serif|monospace|cursive).*" is not empty, use it.
> 3.2 If "font.name.(serif|sans-serif|monospace|cursive).*" is empty, use first available font in "font.auto-list.(serif|sans-serif|monospace|cursive).*"
> 3.3 Otherwise, use "font.name-list.(serif|sans-serif|monospace|cursive).*"
> 4. The font dialog should display font family name if "font.name.(serif|sans-serif|monospace|cursive).*" is not empty.
> 5. Otherwise, display it as |"Auto (" + actual font name + ")"|.
With these new mechanism, any location can specify multiple fonts as default font.
Any ideas or objections?
Updated•8 years ago
|
Whiteboard: [gfx-noted]
Comment 2•8 years ago
|
||
This sounds reasonable to me, provided it can be implemented with minimal overhead in the font-matching process, as that can be a pretty hot codepath.
Would we include the "Auto (...)" entry in the Options font popup if there's a font.auto-list.* pref defined, but it's not the current setting? If not, I think that means a user who changes the default will not be able to restore that setting (except by manually clearing the font.name.* pref in about:config), but perhaps that's OK.
If we do include the "Auto" entry in the popup, that means the list we display will be specific to the language group that we're displaying it for, rather than always being the same list of fonts. So that will require a little extra care, but probably isn't too hard to implement.
Flags: needinfo?(jfkthame)
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #2)
> Would we include the "Auto (...)" entry in the Options font popup if there's
> a font.auto-list.* pref defined, but it's not the current setting? If not, I
> think that means a user who changes the default will not be able to restore
> that setting (except by manually clearing the font.name.* pref in
> about:config), but perhaps that's OK.
My idea is, dropdown will have "Auto (...)" in its top. If the font family available in font.auto-list.* is "Foo", the drop down will be:
* Auto (Foo)
* Bar
* Buzz
* Foo
* ...
So, selecting "Auto (...)" resets the font.name preference (then, actual font is changed automatically when "font.auto-list.*" is changed or user upgrade his/her OS and better font becomes available.
# If "Foo" is selected explicitly, even if they are changed, the actual default font won't be changed automatically.
Comment 4•8 years ago
|
||
(In reply to Masayuki Nakano [:masayuki] from comment #0)
> > 3. gfx should find default font with following steps:
> > 3.1 If "font.name.(serif|sans-serif|monospace|cursive).*" is not empty, use it.
> > 3.2 If "font.name.(serif|sans-serif|monospace|cursive).*" is empty, use first available font in "font.auto-list.(serif|sans-serif|monospace|cursive).*"
> > 3.3 Otherwise, use "font.name-list.(serif|sans-serif|monospace|cursive).*"
What happens if the user has configured a particular font (so "font.name.(...).*" is not empty), and it is not available? I assume we fall back to the list in "font.auto-list...." ? In that case, what's the benefit of leaving the font.name.(...).* value empty?
I wonder if a simpler implementation would put the "preferred-most" font in the default value for the "font.name.(...).*" pref (instead of leaving it empty). So effectively, the first element of what you're suggesting we put in "auto-list" would be the default for "font.name.(...).*". That would improve backward/forward profile migration between versions with/without this feature, and it would otherwise, AIUI, have the same consequences.
For the UI, I think we should then simply display the first available font on the machine (and write that value to the "font.name.(...).*" pref if the dialog is saved). That avoids having an explicit 'automatic' value. The downside is that we would need an efficient way of determining if the font is available. It wouldn't do, when opening the dialog, to have to query the disk or OS APIs for all fonts... though that dialog is already a bit of a performance disaster, so maybe it wouldn't be worse... And in any case, we would need to do that to display "Auto (font name)", as suggested in comment #2, too.
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to :Gijs from comment #4)
> (In reply to Masayuki Nakano [:masayuki] from comment #0)
> > > 3. gfx should find default font with following steps:
> > > 3.1 If "font.name.(serif|sans-serif|monospace|cursive).*" is not empty, use it.
> > > 3.2 If "font.name.(serif|sans-serif|monospace|cursive).*" is empty, use first available font in "font.auto-list.(serif|sans-serif|monospace|cursive).*"
> > > 3.3 Otherwise, use "font.name-list.(serif|sans-serif|monospace|cursive).*"
>
> What happens if the user has configured a particular font (so
> "font.name.(...).*" is not empty), and it is not available? I assume we fall
> back to the list in "font.auto-list...." ?
I think that just falling back to 3.3 as same as current behavior.
> In that case, what's the benefit
> of leaving the font.name.(...).* value empty?
So, empty string is just a constant value which means user doesn't specify a specific font to it.
> I wonder if a simpler implementation would put the "preferred-most" font in
> the default value for the "font.name.(...).*" pref (instead of leaving it
> empty). So effectively, the first element of what you're suggesting we put
> in "auto-list" would be the default for "font.name.(...).*". That would
> improve backward/forward profile migration between versions with/without
> this feature, and it would otherwise, AIUI, have the same consequences.
If we set font.name.(...).*, following 2 users have different font settings even though they haven't changed the font settings:
1. First preferred font is chosen because of using the latest OS and/or having installed proper language pack to the OS.
2. Second preferred font is chosen due to older OS or not having installed language pack yet but upgraded to new OS and/or installed language pack after that.
I think that this is very bad thing for QA cost and checking users environment when they report some bugs. (If reporter changed font settings explicitly, they may comment about that or quickly reply to developers at asking about it.
Comment 6•8 years ago
|
||
(In reply to Masayuki Nakano [:masayuki] from comment #5)
> (In reply to :Gijs from comment #4)
> > I wonder if a simpler implementation would put the "preferred-most" font in
> > the default value for the "font.name.(...).*" pref (instead of leaving it
> > empty). So effectively, the first element of what you're suggesting we put
> > in "auto-list" would be the default for "font.name.(...).*". That would
> > improve backward/forward profile migration between versions with/without
> > this feature, and it would otherwise, AIUI, have the same consequences.
>
> If we set font.name.(...).*, following 2 users have different font settings
> even though they haven't changed the font settings:
>
> 1. First preferred font is chosen because of using the latest OS and/or
> having installed proper language pack to the OS.
> 2. Second preferred font is chosen due to older OS or not having installed
> language pack yet but upgraded to new OS and/or installed language pack
> after that.
>
> I think that this is very bad thing for QA cost and checking users
> environment when they report some bugs. (If reporter changed font settings
> explicitly, they may comment about that or quickly reply to developers at
> asking about it.
I don't understand why leaving font.name.(...).* empty makes this case better. The same thing will happen, right?
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to :Gijs from comment #6)
> (In reply to Masayuki Nakano [:masayuki] from comment #5)
> > (In reply to :Gijs from comment #4)
> > > I wonder if a simpler implementation would put the "preferred-most" font in
> > > the default value for the "font.name.(...).*" pref (instead of leaving it
> > > empty). So effectively, the first element of what you're suggesting we put
> > > in "auto-list" would be the default for "font.name.(...).*". That would
> > > improve backward/forward profile migration between versions with/without
> > > this feature, and it would otherwise, AIUI, have the same consequences.
> >
> > If we set font.name.(...).*, following 2 users have different font settings
> > even though they haven't changed the font settings:
> >
> > 1. First preferred font is chosen because of using the latest OS and/or
> > having installed proper language pack to the OS.
> > 2. Second preferred font is chosen due to older OS or not having installed
> > language pack yet but upgraded to new OS and/or installed language pack
> > after that.
> >
> > I think that this is very bad thing for QA cost and checking users
> > environment when they report some bugs. (If reporter changed font settings
> > explicitly, they may comment about that or quickly reply to developers at
> > asking about it.
>
> I don't understand why leaving font.name.(...).* empty makes this case
> better. The same thing will happen, right?
Because it resolves empty string to preferred font name at every run time. Therefore, if installed font is changed, new instance resolves with new fonts. I don't know when it runs, though. I guess when gfx needs to refer the pref first time for each language? If it's not so, there may be performance issue with my idea because we don't set font names for some languages at least on Windows. E.g., Devanagari's Serif, Ethiopic's any fonts, etc.
Assignee | ||
Comment 8•8 years ago
|
||
Wow! Current font setting UI in Options sets font settings when the specified font is not available without user's change, sigh.
Assignee | ||
Comment 9•8 years ago
|
||
Looks like that there is "default font" feature but not used currently. We can use this, probably.
nsIFontEnumerator is implemented as nsThebesFontEnumerator, but its GetDefaultFont() returns always nullptr and it's not overridden by anybody. Additionally, it's referred from FontBuilder of pref UI which exposes it as "Default (%s)" into the dropdown list. This is what I suggested.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Assignee | ||
Comment 10•8 years ago
|
||
Assignee | ||
Comment 11•8 years ago
|
||
Assignee | ||
Comment 12•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 17•8 years ago
|
||
IIUC, this would leave us with three related sets of prefs for each language / CSS-generic combination:
font.name.<generic>.<lang>
font.name-auto-list.<generic>.<lang>
font.name-list.<generic>.<lang>
That seems perhaps overly complicated. Can't we achieve essentially the same result if we simply move the current font.name.* prefs into their sibling font.name-list.* prefs, and let an empty font.name.* value mean "default to the first available font from the name-list pref"?
Which is largely how things would currently work, I think, except for providing the "Default (...)" entry for the UI; and we can populate that by just implementing nsIFontEnumerator::GetDefaultFont() as returning the first available font from font.name-list.*.
Is there something more that I'm missing here? What would we gain by having two prefs that can both hold a list of fonts?
Assignee | ||
Comment 18•8 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #17)
> IIUC, this would leave us with three related sets of prefs for each language
> / CSS-generic combination:
>
> font.name.<generic>.<lang>
> font.name-auto-list.<generic>.<lang>
> font.name-list.<generic>.<lang>
>
> That seems perhaps overly complicated.
Unfortunately, I agree with it.
> Can't we achieve essentially the same
> result if we simply move the current font.name.* prefs into their sibling
> font.name-list.* prefs, and let an empty font.name.* value mean "default to
> the first available font from the name-list pref"?
Hmm, I was thinking so, however, I worried about the risk. For fixing bug 548311 at 57, I'd like to fix this ASAP. Therefore, I took current approach. But if you'd like to change the meaning of font.name-list.*, I'll rewrite the patches soon.
> Which is largely how things would currently work, I think, except for
> providing the "Default (...)" entry for the UI; and we can populate that by
> just implementing nsIFontEnumerator::GetDefaultFont() as returning the first
> available font from font.name-list.*.
Yes.
> Is there something more that I'm missing here? What would we gain by having
> two prefs that can both hold a list of fonts?
So, the reason why I took the approach is just for risk management. So,not problem to rewrite them as so if you like it better.
Comment 19•8 years ago
|
||
mozreview-review |
Comment on attachment 8852362 [details]
Bug 1344990 part.4 FontBuilder.readFontSelection() should return empty string when saved font isn't available
https://reviewboard.mozilla.org/r/124264/#review127198
I have to admit I don't follow:
- how this is handling the new "auto" value mentioned in the commit message.
- if we are ensuring that all the entries in the UI are present on the system somehow. If not, it seems like setting to "" will flip the dropdown field to whatever the automatic default font is, which in turn will cause a confusing UI for the user (the user sets font to font X, font dropdown automatically flips to font Y). Not sure if there's anything we can do about that, and/or if I'm just misunderstanding the patch.
However, I also think that in any case, these reviews should probably go to :jaws, because of the preferences forking, unless he thinks otherwise.
Attachment #8852362 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 20•8 years ago
|
||
(In reply to :Gijs from comment #19)
> - how this is handling the new "auto" value mentioned in the commit message.
Fortunately, font dialog code already has the feature, it calls "default". So, without any change, it works about this feature. See here:
http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/toolkit/mozapps/preferences/fontbuilder.js#19,28,32,42,47
> - if we are ensuring that all the entries in the UI are present on the
> system somehow. If not, it seems like setting to "" will flip the dropdown
> field to whatever the automatic default font is, which in turn will cause a
> confusing UI for the user (the user sets font to font X, font dropdown
> automatically flips to font Y). Not sure if there's anything we can do about
> that, and/or if I'm just misunderstanding the patch.
If user chose font Y explicitly, it won't be automatically changed to the others unless it's uninstalled from the system. If it's uninstalled, the pref value will be reset to "" ("default", I called "auto").
> However, I also think that in any case, these reviews should probably go to
> :jaws, because of the preferences forking, unless he thinks otherwise.
Thanks!
Assignee | ||
Updated•8 years ago
|
Attachment #8852359 -
Flags: review?(jfkthame)
Attachment #8852360 -
Flags: review?(jfkthame)
Attachment #8852361 -
Flags: review?(jfkthame)
Assignee | ||
Comment 21•8 years ago
|
||
Okay, I'm rewriting the patches with the another approach, just falling back to font.name-list.*. That makes the patches simpler except part.1.
Assignee | ||
Updated•8 years ago
|
Attachment #8852359 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8852360 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8852361 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8852362 -
Attachment is obsolete: true
Assignee | ||
Comment 22•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 27•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 32•8 years ago
|
||
mozreview-review |
Comment on attachment 8852539 [details]
Bug 1344990 part.4 FontBuilder.readFontSelection() should return empty string when saved font isn't available
https://reviewboard.mozilla.org/r/124730/#review127674
r=me but you will have to rebase now that bug 1335907 has landed, which moved Fonts from the "content" pane to the "general" pane (towards the bottom). Only the test within "in-content" will need updating. The test within "in-content-old" can stay as-is.
Attachment #8852539 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 33•8 years ago
|
||
Comment 34•8 years ago
|
||
mozreview-review |
Comment on attachment 8852536 [details]
Bug 1344990 part.2 Set each "font.name.*" to empty string which means "default" and move the values to first item of "font.name-list.*" on any platforms
https://reviewboard.mozilla.org/r/124724/#review128456
Looks good, just a couple tiny things I noticed.
This should probably be landed after patch 2, or merged with it, otherwise I suspect we'd have a lot of test breakage if anyone ever bisects and ends up at this changeset where all the font.name.*.* prefs are empty but the code hasn't changed yet.
::: modules/libpref/init/all.js:3424
(Diff revision 2)
> // When false, the prefs will be used for all mouse events.
> pref("ui.mouse.radius.inputSource.touchOnly", true);
>
> #ifdef XP_WIN
>
> -pref("font.name.serif.ar", "Times New Roman");
> +pref("font.name-list.serif.ar", "Times New Roman, Segoe UI");
I don't think Segoe UI should be in the serif list.
::: modules/libpref/init/all.js:3434
(Diff revision 2)
> -pref("font.name.serif.el", "Times New Roman");
> +pref("font.name-list.sans-serif.he", "Arial");
> -pref("font.name.sans-serif.el", "Arial");
> -pref("font.name.monospace.el", "Courier New");
> -pref("font.name.cursive.el", "Comic Sans MS");
> -
> -pref("font.name.serif.he", "Narkisim");
> -pref("font.name.sans-serif.he", "Arial");
> -pref("font.name.monospace.he", "Fixed Miriam Transparent");
> -pref("font.name.cursive.he", "Guttman Yad");
> pref("font.name-list.serif.he", "Narkisim, David");
Please swap these lines, so that we have a consistent order (serif, sans-serif) for each lang's prefs.
Attachment #8852536 -
Flags: review?(jfkthame) → review+
Comment 35•8 years ago
|
||
mozreview-review |
Comment on attachment 8852537 [details]
Bug 1344990 part.1 gfx and layout should refer "font.name-list.*" when "font.name.*" is empty
https://reviewboard.mozilla.org/r/124726/#review128462
::: layout/base/StaticPresData.cpp:194
(Diff revision 2)
> nsFont* font = fontTypes[eType];
>
> // set the default variable font (the other fonts are seen as 'generic' fonts
> // in GFX and will be queried there when hunting for alternative fonts)
> if (eType == eDefaultFont_Variable) {
> + // XXX "font.name.variable."? There is no such pref...
Interesting... maybe file a followup to check on the history of this, and remove it if it's completely obsolete?
Attachment #8852537 -
Flags: review?(jfkthame) → review+
Comment 36•8 years ago
|
||
mozreview-review |
Comment on attachment 8852538 [details]
Bug 1344990 part.3 Implement nsIFontEnumerator::GetDefaultFont() as returning first available font in font.name-list.*
https://reviewboard.mozilla.org/r/124728/#review128464
::: gfx/src/nsThebesFontEnumerator.cpp:99
(Diff revision 2)
> + if (NS_WARN_IF(!aLangGroup) || NS_WARN_IF(!aGeneric)) {
> + return NS_ERROR_INVALID_ARG;
> + }
Might as well combine all the tests into a single `if (...) return NS_ERROR_INVALID_ARG` at the start of the method.
::: gfx/thebes/gfxPlatformFontList.cpp:806
(Diff revision 2)
> + nsAutoCString prefName("font.name-list.");
> + prefName.Append(aGenericFamily);
> + prefName.Append('.');
> + prefName.Append(aLangGroup);
This little snippet to build the pref name occurs a number of times; I wonder if it's worth encapsulating in a helper somewhere, so that it becomes a one-liner each time we need to do it?
Attachment #8852538 -
Flags: review?(jfkthame) → review+
Assignee | ||
Comment 37•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8852536 [details]
Bug 1344990 part.2 Set each "font.name.*" to empty string which means "default" and move the values to first item of "font.name-list.*" on any platforms
https://reviewboard.mozilla.org/r/124724/#review128456
Okay, I'll just reorder the patches (2 -> 1 -> 3 -> 4) due to review flag management in MozReview.
> I don't think Segoe UI should be in the serif list.
Oh, right.
> Please swap these lines, so that we have a consistent order (serif, sans-serif) for each lang's prefs.
Thanks, I checked other prefs too.
Assignee | ||
Comment 38•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8852537 [details]
Bug 1344990 part.1 gfx and layout should refer "font.name-list.*" when "font.name.*" is empty
https://reviewboard.mozilla.org/r/124726/#review128462
> Interesting... maybe file a followup to check on the history of this, and remove it if it's completely obsolete?
Filed bug 1352975.
Assignee | ||
Comment 39•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8852538 [details]
Bug 1344990 part.3 Implement nsIFontEnumerator::GetDefaultFont() as returning first available font in font.name-list.*
https://reviewboard.mozilla.org/r/124728/#review128464
> Might as well combine all the tests into a single `if (...) return NS_ERROR_INVALID_ARG` at the start of the method.
Okay. But *aResult won't be cleared if aResult is not nullptr but another one is nullptr.
> This little snippet to build the pref name occurs a number of times; I wonder if it's worth encapsulating in a helper somewhere, so that it becomes a one-liner each time we need to do it?
Indeed. I filed bug 1352977.
Assignee | ||
Comment 40•8 years ago
|
||
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 45•8 years ago
|
||
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/46e6d8c9341f
part.1 gfx and layout should refer "font.name-list.*" when "font.name.*" is empty r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/2b99ea907d3d
part.2 Set each "font.name.*" to empty string which means "default" and move the values to first item of "font.name-list.*" on any platforms r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/74fa1a2c6240
part.3 Implement nsIFontEnumerator::GetDefaultFont() as returning first available font in font.name-list.* r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/0b45ef5f7702
part.4 FontBuilder.readFontSelection() should return empty string when saved font isn't available r=jaws
Comment 46•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/46e6d8c9341f
https://hg.mozilla.org/mozilla-central/rev/2b99ea907d3d
https://hg.mozilla.org/mozilla-central/rev/74fa1a2c6240
https://hg.mozilla.org/mozilla-central/rev/0b45ef5f7702
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•