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)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(4 files, 4 obsolete files)

59 bytes, text/x-review-board-request
jfkthame
: review+
Details
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
59 bytes, text/x-review-board-request
jaws
: review+
Details
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?
:jfkthame, can you comment to the bug?
Flags: needinfo?(jfkthame)
Whiteboard: [gfx-noted]
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)
(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.
(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.
(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.
(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?
(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.
Wow! Current font setting UI in Options sets font settings when the specified font is not available without user's change, sigh.
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: nobody → masayuki
Status: NEW → ASSIGNED
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?
(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 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)
(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!
Attachment #8852359 - Flags: review?(jfkthame)
Attachment #8852360 - Flags: review?(jfkthame)
Attachment #8852361 - Flags: review?(jfkthame)
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.
Attachment #8852359 - Attachment is obsolete: true
Attachment #8852360 - Attachment is obsolete: true
Attachment #8852361 - Attachment is obsolete: true
Attachment #8852362 - Attachment is obsolete: true
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+
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 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 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+
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.
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.
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.
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
Depends on: 1353278
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: