31816 - Not able to override page font defaults

Status: VERIFIED FIXED

(Core :: Internationalization, defect, P2)

Description

[Allan Masri]

with Netscape6 2000031012, all platforms.
Steps to reproduce:
1. Open fonts preferences panel.
2. Select any encoding and any font in the list.
3. Select override page fonts (so you can see the changes, if any)
4. Switch the serif / sans serif radio list to the opposite button
5. close the prefs panel and reload the page.
Result: fonts change. Now do steps 1-5 over again. This time fonts don't change.

Comment 1

[[e-mail] (away - not reading bugmail)]

Alan, Pierre,

after a very helpful clarification from Erik it seems that the "override page 
fonts" preference setting is not implemented on the backend. I'm tentatively 
assigning this as a tracking bug to Pierre and changing its title to reflect 
the missing functionality.

To clarify Alan's and my observation further: if you do the steps Alan outlined 
for Netcenter, you will not be able to set the font default to anything else 
then sans-serif, because the document is asking for it in the <font face> tag 
and we are not able to override the page default yet. 

In contrast, if you go to the Mozilla.org page, it works as expected since the 
document doesn't have any fonts defaults.

Comment 2

[rickg]

non-essential for m16

Comment 3

[Pierre Saslawsky]

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

Comment 4

[Pierre Saslawsky]

Adding the 'nsbeta3' and 'polish' keywords from bug 46286, but not 'pp'

Bug 46286 contains a testcase.

Comment 5

[Akkana Peck]

Attachment: Here's the test case from the other bug

Comment 6

[ekrock's old account (dead)]

Easy fix per pierre; [nsbeta3+].

Comment 7

[Phil Peterson]

PDT downgrading to P2, and only that high due to concerns about visually
impaired users.

Comment 8

[Pierre Saslawsky]

Note to self: the pref should only be active in the content area. After finding 
how to do that, share the info with Marc in bug 32372.

Comment 9

[Marc Attinasi]

From Daivd Hyatt:

ben says you need to know how to tell if you're in chrome... 

This info is on the docshell... you can see an example of the check in 
nsDocumentViewer.cpp where I check to see whether or not I want to use chrome or 
content user stylesheets.... 

  nsCOMPtr<nsIDocShellTreeItem> docShell = (get it from whereever you are...) 
  PRInt32 shellType; 
  docShell->GetItemType(&shellType); 
  PRBool isChrome = (shellType == nsIDocShellTreeItem::typeChrome); 

Comment 10

[karnaze (gone)]

Reassigning to Marc. This is related to bug 22963.

Comment 11

[Marc Attinasi]

This bug is similar to bug 22963: it is a pref that has never worked and that
cannot be implemented within the current style system.

Essentially, we need to either subvert the style system (that is, make sure that
fonts specified in style rules are ignored and the ones specified in the pref
are always used) or we need to insert style rules into the CSSOM to make the
style system enforce the font. Problems with the current implemenation of the
cascade make the latter seem unlikely, and the former requires a lot of coding
in the style system to know about the default fonts and use them exclusively
(leaving the chrom alone, presumably).

Like bug 22963, I think that this is a large change and it might be better to do
this after RTM rather than rush the solution now. Of course, if we chose not to
fix this for RTM, we need to remove the pref  "Always use my font settings...".

When we do decide to attack this bug, we really need to design how it should
work (if this has already been done I would love to see the design spec). Nav
enforces the font-face, for example, but not the size (font sizes can be changed
by using style rules and <font> tags, but the font face cannot be). For
accessabilty reasons, we might want to respect the size of the users chosen
fonts, too. Also, should this affect the chrome too (I doubt it, people can get
skins with larger fonts, right)?

Comment 12

[Erik van der Poel]

I'm not sure that it's that hard to fix this. (Not that I have a lot of time to
do this myself, but...) Pierre's suggestion a while ago was to make
nsFont::EnumerateFamilies() skip over the leading non-generic fonts, and only
process the first generic font (where "generic" is the CSS serif, sans-serif,
etc). The font prefs code is all based on the generics, so this would
effectively limit the display to the user's chosen fonts.

Comment 13

[Marc Attinasi]

Thanks Erik. I didn't realize that Pierre had already put some thought into this
one.

I agree that it does not sound that hard, bearing in mind that we have to be
careful to restrict this font filtering to the non-chrome docshell. The change
is not without risk, although I can envision the risk being relatively low in
the best case.

The remaining question is, how important is it to get this in for RTM? The PDT
will make that decision, I guess.

Comment 14

[Akkana Peck]

Adding access keyword -- it's an accessibility issue due to different font faces
being different sizes (e.g. see bug 46415).

Why restrict it to non-chrome?  If that's what makes this difficult, perhaps we
can let it affect chrome as well in the first release, then fix that part
later.  (Some people would argue that letting the user choose a chrome font is a
good thing.)  Then at least people who need this pref have a usable browser in
the meantime.

Comment 15

[Marc Attinasi]

I would guess that the UI would be pretty much of a mess if the user's choice of
fonts was honored in the chrome (just a guess, mind you). I can just imagine
those 24pt fonts in the prefs dialog...

Anyway, I'm not sure that keeping the font out of the chrome is that hard. But
before we spend much time on it I'd like to make sure the PDT will even accept
it. To that end, nominating for RTM to see what the PDT thinks.

Comment 16

[Phil Peterson]

Not holding PR3 for this -- marking nsbeta3-. 

Will let the rtm nomination run its course, but I think the interesting analysis 
is to figure out where this bug fits within the priority list of other layout 
bugs. Sure, it would be nice to fix this, but if it isn't high up on the list, 
or is too hard/risky, I'm not opposed to removing the feature.

Comment 17

[karnaze (gone)]

Adding rtm+.

Comment 18

[Phil Peterson]

PDT marking [rtm need info] since there isn't a patch or code review yet. PDT 
would like to have a safe fix for this for RTM.

Comment 19

[Pierre Saslawsky]

Akkana, I made it run once and that's how I figured out that the pref should only 
be active in the content area.

Marc, It was a very simple patch, something like Erik wrote on [2000-09-25 
15:53]. Look for "FirstExistingFont" in Layout and you will know what to do.

Comment 20

[don]

Added myself to "cc" list.

Comment 21

[Pierre Saslawsky]

Reassigned to myself. Unlike [karnaze 2000-09-21 11:36], I don't think it is 
related to bug 22963.

Comment 22

[Marc Attinasi]

pierre, I already have fixed this... it is in my local tree and I'll be getting
reviews today.

Comment 23

[Marc Attinasi]

Taking back: Just to re-confirm, I implemented this as you suggested, Pierre,
using C++ code to fall back to the default fonts instead of matching the
specified font (not implemented using custom style rules, I couldn't find rules
to make that work).

Patch coming...

Comment 24

[karnaze (gone)]

Removing rtm, since this is covered by bug 40340.

Comment 25

[Marc Attinasi]

Fixed by checkin for bug 40340

Comment 26

[Andreas Becker]

Changing QA contact to [e-mail].

Comment 27

[Yuying Long]

Not reproducible on recently trunk build, mark it as verified.