105297 - unchecking "Allow documents to use other fonts" works wrong

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Nikita Borisov]

As illustrated by the web page example.  "This sentence should be in a
sans-serif font, not cursive" is actually rendered in a serif font.  The style
sheet contains the following directive;

.six { font-family: sans-serif,cursive; } 

which appears to confuse Mozilla.

- Nikita

Comment 1

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Hmm.  Works OK for me.  Are the other sans-serif fonts OK?

Comment 2

[Nikita Borisov]

Sorry, some more info.  This only happens when I uncheck "Allow the document to
use other fonts" box in Preferences/Appearance/Fonts.  It's still incorrect,
since the sans-serif and cursive fonts are both defined, so there's no reason to
ignore the directive.

- Nikita

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Hmmm, I didn't realize the meaning of that pref was that subtle.

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I think if we want to make unchecking "Allow documents to use other fonts" work
some way similar to the way it does now, it should use the first generic font in
the author's list, whether or not it is preceded by non-generic fonts.  The
alternative is to ignore all author font preferences...

Comment 5

[rbs]

> This only happens when I uncheck "Allow the document to use other fonts" box
> in Preferences/Appearance/Fonts.  

With the uncheck, Mozilla then uses what you have selected as your default font
(as seen on the same dialog box -- by default it is serif). Seems there is no
bug here -- unless the issue is to redesign it to work as per dbaron's comments
above.

Comment 6

[Nikita Borisov]

But the check box does *not* say: ignore author-specified fonts, and it
shouldn't act that way.  It seems inconsistent to render 
"font-family: sans-serif" as sans-serif, but "font-family: sans-serif,cursive"
as the default font.  I think that dbaron's redesign is the correct way to
implement the checkbox.

- Nikita

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

rbs: It already kind of works the way I described, except it only works when
there's a single font specified, not multiple generic fonts.

Comment 8

[rbs]

Ah, I remember now. Not parsing the multiple font stuff is a perf consideration.

Comment 9

[kill this account]

Erik van der Poel said to me on several occasions that the CSS spec states
that once a generic font is seen in the font list that is the effective end 
of the list as the generic must work and must be used.

Comment 10

[rbs]

The underlying catch is that an equality test is used during style resolution
(c.f. for example bug 78517). Hence, "font-family: sans-serif,cursive" is not
treated as if the generic font was "sans-serif" _during style resolution_. I
once had the thought of adding an API, say nsIFontMetrics::EnumerateFamilies(),
to let the font code enumerates the fonts that it has. There is another similar
API in nsFont, but the drawback in directly using that API would be that it
would parse and reparse at each call. Whereas if there was another
nsIFontMetrics::EnumerateFamilies(), it could just return what is already parsed
and cached there. Plus, there could be a flag to instruct the API to only return
the generic font in one stretch.

Comment 11

[Philip flip Kromer]

This may be obvious, but I'll also point out that if a specific face is given
it, too, kicks the text back to the default (serif) face.

For example, if I specify <FONT face="sans-serif"> the text will appear,
correctly, in sans-serif, regardless of the "Allow ... other fonts" setting.  If
I specify <FONT face="Trebuchet MS, sans-serif"> the text will incorrectly
appear in the default face when I uncheck the box.

See testcase at http://mrflip.com/MozBug/ButtonFont.html

Comment 12

[rbs]

same problem: it is due to the fact that an equality test is used as I explained 
above.

Comment 13

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

So would it be better to change the meaning of the pref so it completely ignores
fonts from documents, or can we fix it easily to do what was currently intended?

Comment 14

[rbs]

#2 has perf side-effects. I can get #1 relatively easily if people like that, 
programatically, it amounts to just use the default variable anytime.

(PS: I suspect similar queries will arise again when the ability to set 
different font-sizes is exposed on the UI. It is based on an equality test too 
-- so "font-family: sans-serif,cursive" isn't treated as "sans-serif")

Comment 15

[rbs]

...although the handling of the "sans-serif,cursive" case can be improved if an
n-compare was used (which might still fail if the generic name is a legitimate 
prefix of another font, or if users quote the " generic" name and have some 
leading spaces there to make the matter worse).

Comment 16

[rbs]

Attachment: fix to completely ignore document fonts

Comment 17

[rbs]

BTW, while fixing this, I saw once more how tricky -moz-fixed is. Indeed the 
pref puts monospace, serif, sans-serif, etc, in the same basket. Without 
special-casing -moz-fixed, <pre>, etc, would not render as fixed-width font with 
the pref unchecked.

Comment 18

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Does this pref affect the fonts in form controls or in user stylesheets, or is
there a check for levels of the cascade?  Also, should we slightly rename the
pref text?

Comment 19

[rbs]

Form controls (and generally, system fonts for which family.GetUnit() == enum)
come further down the chain, and so they fortuitously escape the pref.

Comment 20

[Nikita Borisov]

Re: patch from rbs --

From my point of view, the patch just makes things worse (if I understand it
correctly).  I really want Mozilla to only use generic fonts, and the pref seems
to specify exactly that, only now it misunderstands style sheets some of the time.  
The patch seems to ignore stylesheet font-specifications completely; I don't
understand why this is a desirable behavior.  It certainly doesn't agree with my
interpretation of what the checkbox says -- a more appropriate text would be
"ignore author-specified fonts."

Comment 21

[rbs]

> the patch just makes things worse
It bring Mozilla back to where it initially was. I had earlier did some changes 
but they led to this bug, so my fix is to go back to square one where people 
seemed to have been happy. On further thoughts, I think it is the most 
consistent and easier to get. And the other approach that you are advocating, if 
implemented, could be labelled as another pref.

Comment 22

[rbs]

I would like to put back the simpler option. The code for the subtle
option is still there, and could be exposed with another pref, so... 
dbaron, r? attinasi, sr?

Comment 23

[Pierre Saslawsky]

If you do it that way, you should change the pref name from "Allow documents to 
use other fonts" to "Allow documents to specify fonts" otherwise people will 
think that documents can still use any of the 5 generic fonts.

Comment 24

[Marc Attinasi]

Comment on attachment 54198 [details] [diff] [review]
fix to completely ignore document fonts

sr=attinasi, and I like Pierre's idea for the wording, but my sr is not contingent upon that.

Comment 25

[rbs]

[mid-air collision, re-submitting, I typed this without seeing Marc's comments]

pierre, my patch is reverting to the previous way -- i.e., it is a return to
what the pref and its description have been doing all along. The other, newer
and subtle way came with my changes and it is proving more difficult and
expensive to get right. So I am putting back the behavior that used to be there.
But as I said earlier, I suspect that the need to check the right generic -- at
least in the case of "generic1, generic2" -- will surface again when the ability
to set different sizes is exposed. I am deferring the vodoo needed for that
until then... And if people like the newer subtle way of allowing document
generic fonts, I don't mind that either, and it will benefit from any work to
get the check of the generic right. Also, I don't mind letting this subtle way
supersede the older way if prefs have to be kept simple and a single pref has to
be considered between the two. So really, I am okay with any option that people
like so long as they understand the issues. Just let me know what you guys like
the most...

Comment 26

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

->rbs, since he has a patch

Comment 27

[rbs]

Accepting and asking r= on attachment 54198 [details] [diff] [review].

[Reminder: c.f. comment #17, -moz-fixed is needed to make <pre>, etc, to work.
Without the special casing, all fixed-width browser's elements would erroneously
render in the variable width font as well.]

Reporter, please file another bug (likely to be Future'd) if you would like the
existing approach to be consolidated instead (comment #20). The patch is undoing
that for now.

Comment 28

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 54198 [details] [diff] [review]
fix to completely ignore document fonts

r=dbaron

Comment 29

[rbs]

Fix checked in.

Comment 30

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

A request for the other behavior is filed as bug 155026.  (I though there was
another such bug, but I can't find it.)