142511 - Font prefs shouldn't limit choices

Status: RESOLVED FIXED

(SeaMonkey :: Preferences, defect)

Description

[Oliver Klee]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0.0+)
Gecko/20020504
BuildID:    2002050408

In the font preferences, there are only unreadable fonts (like Dingbats) selectable.

Reproducible: Always
Steps to Reproduce:
1. Edit/Preferences
2. Appearance/Fonts
3. Open the "Fantasy" drop-down list.

Actual Results:  Only unreadable, widely different fonts like "Dingbats" or
"Woodtype Ornaments 1" are listed.

Expected Results:  Readable fonts should be listed.

This makes the use of this fonts pointless as the results are unpredictable
(will I get a hand, a flower or a half-circle when using that font on the letter
"A"?).

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is not back end.

What fonts should be listed under "Fantasy" in your opinion?

Comment 2

[Oliver Klee]

I have no idea - just some font that produces at least remotely predictable results.

Are there any specs about that?

Comment 3

[Oliver Klee]

Ok, I looked it up in the CSS2 specs at
http://www.w3.org/TR/REC-CSS2/fonts.html#generic-font-families :

----
Fantasy fonts, as used in CSS, are primarily decorative while still containing
representations of characters (as opposed to Pi or Picture fonts, which do not
represent characters). Examples include:
Latin fonts Alpha Geometrique, Critter, Cottonwood, FB Reactor, Studz
----

So picture fonts like Dingbats or Woodtype Ornaments don't belong into that
category.

Upping severity to Normal as this makes pages that use that Fantasy font family
completely unreadable.

Comment 4

[Matthew T (active 1999-2002)]

*** Bug 159250 has been marked as a duplicate of this bug. ***

Comment 5

[Matthew T (active 1999-2002)]

I don't see any fixed bug which introduced this limitation. CCing rbs and bstell.

Unlike Serif, Sans-serif, and Monospace, the Fantasy family is vague enough that
I don't think a computer program can possibly decide whether a font is Fantastic
or not. (Certainly Mozilla is completely failing at the moment.) Even if it
could, many Mozilla installations -- such as the one I'm using right now -- will
be on computers where no Fantastic fonts are installed, so a merely Mildly
Wonderful font will need to be chosen instead. So, I suggest two changes be made:
1.  The Fantasy menu should always show *all* installed fonts.
2.  If the winnowing of the menu for any font family results in zero fonts being
    available, the menu should show all installed fonts.

Comment 6

[rbs]

Changing title from "Fantasy menu in Font prefs shouldn't limit choices"
"Font prefs shouldn't limit choices".

Suggesting to push mpt's suggestion even further. 

Always list all fonts as two groups:

[ Serif   ]
  font1
  ...         /* Group1: Serif fonts  [depending on the generic option] */
  fontp
  ---------   /* separator */
  fontp+1
  ...         /* Group2: The rest */
[ fontn   ]

Of note:
- Opera lists all fonts, it doesn't even bother to sort in those two groups
- On Unix/Linux, Mozilla is alreay lists all fonts, irrespective of the generic
  option.
- Trying to do the generic selection is unreliable as shown in bug 110655, which
  has a screenshot of the worst case scenario. No fonts at all:
  http://bugzilla.mozilla.org/attachment.cgi?id=58343&action=view

This is entirely fixable via JS, starting points for someone interested:
http://lxr.mozilla.org/seamonkey/source/xpfe/components/prefwindow/resources/content/pref-fonts.js
http://lxr.mozilla.org/seamonkey/source/xpfe/components/prefwindow/resources/content/pref-fonts.xul

Comment 7

[rbs]

-> taking, I have this working. Some tidyings and it will be ready.

Comment 8

[rbs]

Attachment: screenshot - how it looks like

While here I made the lists to load lazily -- as a work-around to the
interminable wait of the font panel (bug 63731). Also, even added an extra, a
spinning loading icon while the load is in progress.

Comment 9

[rbs]

Attachment: patch

Comment 10

[rbs]

ready for comments/feedback/testing/reviews... maybe the spinning loading icon
is too ahowy?

Comment 11

[Christopher Blizzard (:blizzard)]

Maybe a little? :)

Comment 12

[rbs]

Attachment: updated patch (no spinning loading icon anymore)

Did an iteration to remove the spinning loading icon which was bit showy (I
made a typo earlier re:ahowy). With this version, there is no new elements in
the UI. 

Summary of what the patch does:

- list all fonts and implement the grouping described in comment 6. (It is
essential to be able to list all fonts for cases such as bug 110655).

- make the font drop down lists load lazily. Hence no matter the amount of
fonts (as on my system), the font panel is displayed upfront, even when
building all fonts now. At least, the user sees the lists being updated and
doesn't easily get bored waiting for the font panel to show up (bug 63731).

- improve the code in certain areas. For example, the sizes used to be
repeatedly set in a |for| loop. Another example is replacing |strDefaultFont|
which the firt menuitem which is readily available -- rather than doing a
getEltByAttr just to re-find that.

Comment 13

[rbs]

Comment on attachment 122285 [details] [diff] [review]
updated patch (no spinning loading icon anymore)

Asking r/sr to those who bonsai shows as last reviewers to this file:
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/xpfe/components/prefwindow/re
sources/content/pref-fonts.js

Comment 14

[Christopher Blizzard (:blizzard)]

rbs, did you want to include the patch that's in bug 172779 here?  It's a good
time to try to get it in.

Comment 15

[rbs]

Attachment: iteration to include the [System Default]

Comment 16

[rbs]

Attachment: screenshot: how it looks like with [System Default] per bug 172779

Comment 17

[rbs]

Comment on attachment 122345 [details] [diff] [review]
iteration to include the [System Default]

ready for r=/sr= before heading to a=.

Comment 18

[Christopher Blizzard (:blizzard)]

Comment on attachment 122345 [details] [diff] [review]
iteration to include the [System Default]

looks good to me.  sr=blizzard

Comment 19

[rbs]

Attachment: alternate patch (attempt to shorten the drop down lists)

This patch is a slight variation of the earlier one in that it attempts to
shorten the font lists. 

The first patch uses |EnumerateAllFonts| which enumerates everything in the
system. This patch uses EnumerateFonts(aLanguage, null/*aGeneric*/) to only
enumerate the fonts for the current language. On my system, this cut the
drop-down lists from 199 items to 166 items.

Here is the code-section of relevance for this:

+ // get all the fonts for this language to complete the font lists
+ if (!globalFonts)
+   {
+     try
+	{
+	  // this asserts on platforms that don't support a generic |null| yet
+	  globalFonts = getFontEnumerator().EnumerateFonts( aLanguage, null,
count );
+	}
+     catch(e)
+	{
+	  globalFonts = null;
+	}
+     if (!globalFonts || !globalFonts.length)
+	{
+	  // fallback to all fonts on the system rather than nothing (bug
110655)
+	  globalFonts = getFontEnumerator().EnumerateAllFonts( count );
+	}
+    }

Unfortunately, this patch has some ramifications back in Gfx since some
implementations of the font enumerator (e.g., on GfxWin) fail when the CSS
generic parameter is null. So I had to touch the platform-specific code to
allow the CSS generic parameter to be null. Because there are zillion versions
of Gfx, it is not easy to complete all the platforms, and so the JS has a
fallback to the entire list.

Worthy of note is just that the mysterious font "6x13" on attachment 122347 [details]
goes away. It is hard to tell whether the savings from 199 items down to 166
items is worth all the troubles, but there doesn't seem to be any other elegant
way to avoid catastrophic situations such as bug 110655. I am okay with
whichever version the reviewers prefer.

Comment 20

[Gervase Markham [:gerv]]

Comment on attachment 122727 [details] [diff] [review]
alternate patch (attempt to shorten the drop down lists)

[Sorry for the delay. I'm assuming it's the second patch you actually want
reviewed.]

>Index: themes/classic/communicator/prefpanels.css
>===================================================================
>+
>+.prefpanel-font-list {
>+  -moz-box-flex: 1;
>+}

This CSS rule replaces the 'flex="1"' attribute on XUL tags; however, in some
cases, it also replaces 'style="width: 0px"'. Should that not also be reflected
in the rule? 

>Index: xpfe/components/prefwindow/resources/content/pref-fonts.js
>===================================================================
>+          // always put the default system font at the front of the list

On Red Hat Linux 8 with KDE, my fonts now say [System Default] for Cursive and
Fantasy. I had no idea that my system had defaults for these font families -
does it? :-) If not, this is somewhat misleading.

>+          // since the lists are sorted, we can get unique entries by just walking
>+          // both lists linearly side-by-side, skipping those values arealdy in

"already".

>+function lazyAppendFontNames( i )
>+  {
>+     //schedule the build of the next font list
>+     if (i+1 < fontTypes.length)
>+       {
>+         window.setTimeout(lazyAppendFontNames, 100, i+1);

The use of a third and subsequent parameter to window.setTimeout() to pass
parameters to the function called doesn't seem to be standardised or documented
anywhere; you might want to add a comment explaining that you are
lazily-loading successive font lists at 100ms intervals.

>-                catch(e) {
>-                    //font size lists can simply deafult to the first entry

Nit: missing space before comment (also in other places), and "default".

Gerv

Comment 21

[rbs]

OK, let's go for the second one. It might pay off further down the track on
other platforms as well.

>>Index: themes/classic/communicator/prefpanels.css
>>===================================================================
>>+
>>+.prefpanel-font-list {
>>+  -moz-box-flex: 1;
>>+}
> 
> This CSS rule replaces the 'flex="1"' attribute on XUL tags; however, in some
> cases, it also replaces 'style="width: 0px"'. Should that not also be reflected
> in the rule? 

Since 'style="width: 0px"' was present on certain lists and not present on
others, I removed it and didn't see any effect. Since you didn't see any effect
either, I think it might be unnecessary. No font list has a width=0. Let's
simply remove it.

>>Index: xpfe/components/prefwindow/resources/content/pref-fonts.js
>>===================================================================
>>+          // always put the default system font at the front of the list
> 
> 
> On Red Hat Linux 8 with KDE, my fonts now say [System Default] for Cursive and
> Fantasy.

That means they were blank before ("no fonts for this language"), right? Now at
least, you can pick another font on your system -- which why this patch is
useful :-)

> I had no idea that my system had defaults for these font families -
> does it? :-) If not, this is somewhat misleading.

This is a request from the sr in comment 14. It helps Xft. It is understood as
"don't care" -- where the user let Mozilla to decide. What really happens is
documented somewhere else in the patch:

// A font name can't be blank. The special blank means [System Default].
// Unset the pref entirely, letting Gfx to decide. GfxXft will use what
// Xft says, whereas GfxWin and others will use the built-in settings
// that are shipped for font.name and font.name-list.

Updated patch with the other corrections coming up.

Comment 22

[rbs]

Attachment: updated patch to include review comments

Comment 23

[rbs]

Comment on attachment 123654 [details] [diff] [review]
updated patch to include review comments

asking the r/sr stamps again.

Comment 24

[Christopher Blizzard (:blizzard)]

Comment on attachment 123654 [details] [diff] [review]
updated patch to include review comments

sr=blizzard

Comment 25

[Gervase Markham [:gerv]]

Comment on attachment 123654 [details] [diff] [review]
updated patch to include review comments

> That means they were blank before ("no fonts for this language"), right? Now at
> least, you can pick another font on your system -- which why this patch is
> useful :-)

Actually, no - it was a monospace font of some sort. But never mind. This seems
OK to me.

Gerv

Comment 26

[rbs]

Comment on attachment 123654 [details] [diff] [review]
updated patch to include review comments

asking a=, please see comment 12 for a quick summary of what the patch does.

Comment 27

[rbs]

Got a look at the recent Opera, and they have improve upon what they add
earlier. screenshot dated 2001-09-30:
http://bugzilla.mozilla.org/attachment.cgi?id=51495&action=view
Fast forward to now, and they have made their own iterations... (no screenshot
however :-)

Going back to gerv's question:
> I had no idea that my system had defaults for these font families -
> does it? :-) If not, this is somewhat misleading.

The newish Opera uses 'Automatic (fontname1)' where fontname1 is resolved
explicitly. What about switching from '[System Default]' to '[Automatic]' which
might reflect better what is done?

Comment 28

[Asa Dotzler [:asa]]

Comment on attachment 123654 [details] [diff] [review]
updated patch to include review comments

a=asa (on behalf of drivers) for checkin to 1.4

Comment 29

[Gervase Markham [:gerv]]

We should use the text which best reassures the user that everything is taken
care of. Even if it's a bit misleading, [System Default] is probably that.

Gerv

Comment 30

[rbs]

Checked in. After further insights, I let the global fonts be all fonts again
(i.e., globalFonts init'ed with all fonts).

Comment 31

[Scott MacGregor]

I think this bug introduced a really bad performance regression when laying out
the font prefs panel. It takes 3 to 4 seconds on my 2 GHz Win XP box to layout
the font prefs panel now. I think it's because of the huge size of the font
combo boxes now. You can see it painting each individual font combo box. 

I'm getting duplicate reports of this same performance regression in the latest
thunderbird build which just went out with this change in it. 

Comment 32

[Scott MacGregor]

On top of the performance regression these drop down boxes are now so huge they
look really bad and aren't very usable. Opening up one of the font combo boxes
gives me a drop down list that runs from the point of the combox box all the way
down to the bottom of my screen =(

Comment 33

[rbs]

There is an (intentional) setTimeOut which delays things (which, interestingly,
might be interpreted as slow as you pointed out...). Try looking for
'lazyAppendFontNames' in the code in 
http://lxr.mozilla.org/seamonkey/source/xpfe/components/prefwindow/resources/content/pref-fonts.js

Mind playing with the values and telling what you see? E.g., keep 100ms for the
first call (which affects when the initial panel shows up), and use '0' for the
other call (which affects when other drop down lists show up).

The long drop down list was a very difficult trade-off decision between two
evils (unless somebody has a better suggestion). Either the fonts were listed,
or much worse, there could be castatrophic situations where no fonts would
appear, leaving the clueless user with no alternative other than switching to
another browser. It was felt that, for the time being, the person will all fonts
to at least choose from is better off than the one with no fonts at all:
screenshots: http://bugzilla.mozilla.org/attachment.cgi?id=58343&action=view
             http://bugzilla.mozilla.org/attachment.cgi?id=92895&action=view
             http://bugzilla.mozilla.org/attachment.cgi?id=111028&action=view

Comment 34

[(not reading, please use [e-mail] instead)]

bringing jag (and shuehan) into this.

thye recently added some font UI to editor and the pref UI, and I don't remember
it having this type of perf hit.

maybe they can help offer suggestions?

Comment 35

[Scott MacGregor]

I did the following:
         if (i == 0)
           window.setTimeout(lazyAppendFontNames, 100, i+1);
         else
           window.setTimeout(lazyAppendFontNames, 0, i+1);

and it didn't help all that much. Maybe a small improvement but I didn't really
notice. 

Comment 36

[Alec Flett]

on my 1ghz win2k box, this page loads relatively quickly, but you definitely
watch it paint. I remember when WinXP was released we had almost identical
performance problems w.r.t. this font pref pane... my guess is that we're
hitting the same thing, and that maybe WinXP's font APIs are slower than win2k.

Comment 37

[rbs]

Another reason why the panel is slow goes back to bug 63731.
http://bugzilla.mozilla.org/show_bug.cgi?id=63731