58098 - plaintext compose uses variable-width font when 'always use my fonts' pref is set

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect, P3)

Description

[Akkana Peck]

One reason some users prefer plaintext compose is so that they can do ascii
graphics or otherwise make text line up.  (10/26 branch release build, classic
skin.)

The css for our plaintext compose window has regressed so that now it uses
variable width fonts.  It should be, and used to be, fixed-width.

Comment 1

[Akkana Peck]

Regression.

Comment 2

[Ben Bucksch (:BenB)]

Akk, is this since a few days? Might be Marc Attinasi's checkin for the "Always
use my fonts"-pref. Do you have that enabled? Does the bug appear with it disabled?

Deliberately reassigning to Marc.

Marc, I am using trunk from 2000-10-22 plus that one of the patches for bug
57234. I don't see the bug regardsless of the pref.

Comment 3

[Ben Bucksch (:BenB)]

This is a major regression (ai agree with akk that this is one of the largest
motivations for using plaintext compose). I assume we have a fix. rtm nomination.

Comment 4

[Akkana Peck]

Good intuition, Ben!  That's it.

- I see fixed-width in yesterday's trunk build; don't have today's, the build
failed. :-(
- I see variable width in today's branch, with "always use my fonts" = true.
- I see fixed width in today's branch with "always use my fonts" false.

Unfortunately, the set of people who say "always use my fonts" probably overlaps
heavily with the set of people who use plaintext compose and want fixed-width
there: Unix users (who have to override document fonts because of the Arial
problem) and people who want maximal control over what they see.

The "always use" fix was very important and it's great to have it in; but it
shouldn't apply to pre and moz-pre areas like plaintext compose.

Comment 5

[Marc Attinasi]

The font handling code is identical between the branch and the trunk as of last
night when I updated the trunk to match the branch. I cannot see how the
behavior could be different in todays builds.

The problem is this: when 'always use my fonts' is set to true, the font mapping
code in nsCSSStyleRule and nsHTMLFontElement look for the magical font-face
'-moz-fixed' to determine if the NS_STYLE_FONT_USE_FIXED flag is to be set. This
is specifically to support <pre>, <TTY> etc. and is exactly the same as the old
behavior, before the prefs were implemented.

I see the font-face coming through as 'monospace' when I use plain-text
composer, so it is never flicking the NS_STYLE_FONT_USE_FIXED bit. It worked
before because we always honored the font face, so 'monospace' was matched just
as if it had been 'courier'. Now, we ignore all of the font faces and only use
the fonts set in the pref. If the font-face for the style rules is -moz-fixed,
then we know to use the fixed font, otherwise we use the variable font.

We can do one one of two things: change the style sheet for the plaintext
compose window to use -moz-fixed, or change the code to look for special font
faces that it knows should be mapping to fixed fonts. I prefer the first option,
since the -moz-fixed is intended to be the 'one' special face that means 'always
use a fixed width font'. I could not find the CSS file that has the rules for
plaintext compose - if somebody knows which file it is please tell me and I'll
change it and test it out.

Updating summary to reflect the 'always use my font' pref, and removing
regression keyword since the 'always use my fonts' pref never even worked before.

Comment 6

[Marc Attinasi]

Changing the font face from 'monospace' to '-moz-fixed' in nsHTMLEditor.cpp line
5049 fixes this problem. As Akkana noticed, Viewsource has the same problem, as
will anyplace that specifies 'monospace' as a font-face to indicate that any
generic fixed width font is desired.

We can either change those occurrances of 'monospace' to '-moz-fixed' or test
for the font-face 'monospace' like we do for '-moz-fixed' to set the
NS_STYLE_FONT_USE_FIXED flag. As much as I don't like it, it may be the safer
approach...

Looking at the font handling code across the different platforms, it appears
that 'monospace' and '-moz-fixed' act the same. I am not very familiar with this
code, however, and this a cursory observation only. CC'ing Erik for his insight
into how 'monospace' and '-moz-fixed' may differ.

Comment 7

[Marc Attinasi]

BTW: Viewsource is fixed by changing 'monospace' to '-moz-fixed' in
nsViewSourceHTML.cpp, line 104.

Comment 8

[Marc Attinasi]

I'm now convinced that the correct fix is to change 'monospace' to '-moz-fixed'
only where we really do REQUIRE a monospace font. I say this because Nav4.x does
not render a font-family of 'monospace' as a fixed font when 'always use my
fonts' is set. If we treated 'monospace' just like '-moz-fixed' then web pages
that specify a font of 'monospace' would end up looking very different in
Mozilla than in Nav4.x when 'always use my fonts' is set.

Also, it appears that there are very few places where we set the font-family to
'monospace' so it should be easy to look at each case and decide if it should
stay 'monospace' or change to '-moz-fixed'. Sometimes we really do want to force
a fixed-width font (even if the user want to override fonts), but in others we
may not want to.

Currently, ViewSource and Composer need to change to '-moz-fixed' to make sure
that they render in a fixed font even if 'always use my fonts' is set. I'll
attach a patch to fix these two areas and request reviews and approval.

Comment 9

[Ben Bucksch (:BenB)]

Marc, I think, the override prefs should not apply to the UI at all. They are
intended to avoid badly styled webpages, not badly styled Mozilla skins (which
can easily be switched).

Comment 10

[Akkana Peck]

I tend to agree with BenB's comment.  But regardless, changing Composer (and
probably View Source) to use -moz-fixed sounds like a good idea (if 4.7 didn't
use the user's fixed-width font when "always use my fonts" is set, I suppose we
shouldn't either, though that makes me wonder what the plural in "fonts" refers
to since I always assumed it meant variable and fixed, the two I set in the
fonts dialog) and we should probably do that independant of whether we do
anything else later.

Comment 11

[Marc Attinasi]

I agree with Ben too. Implementation-wise we have limited choices since we build 
our content areas and chrome (app) using the same technology. All I have to go 
on is when the PresContext says it is 'chrome' or 'content' and sometimes pieces 
of chrome say they are content (some sidebar panels, for example, and 
ViewSource, and Composer). Viewsource, for example, is just an HTML window, as 
is composer. The only reason we don;t apply the pref to Composer is because we 
had a place to disable it, but that is not always the case. Maybe this is the 
price we pay for being so general-purpose ;)

Using -moz-fixed to trigger fixed-width fonts is one way we can make sure that 
the chrome does what we want while keeping the content consistent with Nav 
(since authors will never use -moz-fixed as a font).

akkana, Nav does use your fixed width font preference, however it only uses it 
for <pre>, <tt> and the like...

Comment 12

[Matthew T (active 1999-2002)]

Marc, your last comment suggests the real cause of this bug. Navigator should be 
using my Monospace font (as specified in prefs) not only for TT, PRE etc, but 
also for {font-family: monospace}, even when `Always use my fonts' is turned on. 
That's what the CSS families are for: to allow authors to give a general idea of 
what sort of font they want (by specifying a family), while letting the user have 
the final say over exactly what font is used for each family.

So bits of chrome should be able to use `monospace' rather than `-moz-fixed', and 
this should not be affected by the `Always use my fonts' pref.

Comment 13

[Ben Bucksch (:BenB)]

matthew,
I disagree. CSS is for style only. The 'Always use my style' prefs are for
overriding the author's styles. If the content inherently requires monospace,
<pre> or something like that should be used in the HTML source.

Comment 14

[Marc Attinasi]

Attachment: Patch to make font -moz-fixed instead of monospace for viewsource and plaintext composer

Comment 15

[Akkana Peck]

r=akkana

Comment 16

[Marc Attinasi]

I got a r=rickg and an sr=buster too, so this is going into the trunk.

Comment 17

[Marc Attinasi]

Fixes checked into trunk.
(nsViewSourceHTML.cpp, nsHTMLEditor.cpp)

Comment 18

[esther]

verified