159440 - [Pref Stylesheet]"allow docs to use other fonts" causes pre to use variable width font family

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Joe Bayes]

Steps to reproduce:
1. Run Mozilla 1.0
2. Uncheck "Allow documents to use other fonts" prefs->appearance->fonts.
3. Load a page with a style spec of pre {font-family: monospace} and some pre text.

Actual Results: 
The text is rendered in a variable-width font. 

Expected Results: 
The text should be rendered in a monospace font. When I say to ignore
document-specified fonts, Moz should default to a monospaced font for <pre> text
no matter what the document says.

Comment 1

[Joe Bayes]

Attachment: Test case to demonstrate bug. Try commenting out the pre style line to see proper behavior.

Comment 2

[Andrew Schultz]

confirming with linux build 20020725

it looks like it goes overboard in ignoring the document-specified font and
ignores the font spec for <pre>

a standard <pre> tag (without specifying a font-family) works fine.

this bug did not exist in 0.9.7

marking NEW

Comment 3

[Andrew Schultz]

this is not a DOM problem
==> StyleSystem (?)

Comment 4

[Jeff Epler]

This bug still exists in RedHat 9's mozilla-1.2.1 and ????'s mozilla-1.3.1

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

duplicate of bug 79074?

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: incomplete patch

If the pre element(or TT, CODE, xml:space="preserve") has no font-family
spcifiy,
the content is displayed by monospace font.
But _this_ is a bug for the current code.

If we will implement "allow docs to use other fonts" completely,
we need the information that is displaied by default variable font or default
fixed font on every element.

This patch is incompletely.
Because this patch is not setting the infomation whitch elements should be
displaied fixed font.
I don't have idea for this initialization.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Test cases.
text/html
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2271&action=view
application/xhtml+xml
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2272&action=view

Comment 9

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

Comment on attachment 150438 [details] [diff] [review]
incomplete patch

You clearly didn't include the whole patch here, since mDefaultFontIsFixedPitch
is never set.

That said, it would probably be a good idea to explain what it is you're trying
to do, since from the look of the partial patch here I'm don't think I like the
approach you're taking.

Your changes to the IsChrome() callers almost seem to be undoing changes I made
a while back.  Is this a merging problem, or intentional?

Please use |diff -pu12| to show function names and more context.

Comment 10

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

I'd prefer fixing this by fixing bug 79074, and I'm not quite sure how you'd fix
it without doing that.  So the most important of the above comments is to
explain the logic of what you're trying to do.

Comment 11

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

Oops, I apologize for not reading all of comment 7.

Anyway, I still don't see what it is you're trying to do.  I think the right
thing to do is to fix bug 79074.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: incomplete patch(diff -pu12 ver.)

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> Your changes to the IsChrome() callers almost seem to be undoing changes I made
> a while back.  Is this a merging problem, or intentional?

It is intentional.
I think that I wrote more simply than the present thing.
The to get the font information and to get |useDocumentFonts| value are
intermingled in the current code.
I separated these codes.
1. Get the value of |useDocumentFonts|.
2. Get the font name from the value of |useDocumentFonts|.

If you suppose the problem that |IsChrome()| is called twice,
I will write "PRBool IsChromeContext = IsChrome(mContext);".

> I still don't see what it is you're trying to do.

I think that the current code ALWAYS get the value of font-family that is
|eCSSUnit_String|.
It is bug.
The current code uses the font-family value before decideing the value of
|useDocumentFonts|.

Then, I think as following.
1. If |useDocumentFonts| is PR_TRUE, we can use specified value of 'font-family'.
2. If |useDocumentFonts| is PR_FALSE, we must ignore specified value of
'font-family'. And we must use "default font".
3. However, the current code has two default font(variable and fixed), but we
cannot know that in the current element, which should be used.

Thus, I think that the element have to have the information of to use which
default font.

# I will sleep...(now is 03:00-JST)

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 150452 [details] [diff] [review]
incomplete patch(diff -pu12 ver.)

My idea is wrong too.
In following case, this patch is not working.

<pre>fixed <em style="font-family: 'hoge';">non-fixed!</em> fixed</pre>

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 16

[Markus Lindström]

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

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 18

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

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

Comment 19

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

Attachment: patch (diff -u)

Comment 20

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

Attachment: patch (diff -ub)

This fixes the bug by essentially fixing bug 79074, except only for the default vs. monospace case.  I'm not sure the rest should be fixed, so I'm attaching it here instead.

It pulls the ignoring of the font name out of SetFont into ComputeFontData.  I cleaned up ComputeFontData a little by always getting useDocumentFonts (which is really cheap now).  This allows the second font-family-related bit to be moved inside the first, since that's the only time when generic != kGenericFont_NONE can happen.

There are really three important changes here:
 * change just checking for _moz_fixed to also check for monospace
 * extract a generic from a family name string rather than requiring that the font be only a generic
 * move ignoring the name out, as mentioned above

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 211164 [details] [diff] [review]
patch (diff -ub)

>Index: nsRuleNode.cpp

> nsRuleNode::SetFont(nsPresContext* aPresContext, 

>+      if (aFont->mFont.name.Length() != 0)

"!aFont->mFont.name.IsEmpty()" would make more sense to me, I think.

r+sr=bzbarsky either way.

Comment 22

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

Checked in to trunk.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

-> v.

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060212 Firefox/1.6a1 ID:2006021206