94078 - getComputedStyle adds "serif" to correct answer for 'font-family'

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect, P2)

Description

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

if you query the value of the 'font-family' property using getComputedStyle,
it appends ",serif" to the correct answer.

Comment 1

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Adding blocking dependency to "CSS support in composer" tracking bug. 

Comment 2

[rbs]

I have seen where this bug comes from. During style resolution, the default font 
(from the Fonts prefs dialog) is appended to the font-family list as a fallback 
font. Unfortunately, GFX has come to rely on this behavior so that the correct 
fix for this will involve changing the zillion versions of GFX on each platform. 
The correct fix would be to let GFX do the appending and exactly preserve the 
computed values during the CSS cascade.

Comment 3

[David Hyatt]

I think it's fine that CSS store the additional fallback font.  It just needs to
ensure that the fallback font is peeled off inside getComputedStyle.

Comment 4

[rbs]

It is a bit trickier because the fallback font can also overwrite the CSS font
(depending on whether the use-document-fonts pref is set). So getComputedStyle()
would have to tell if the current font is the result of an 'append' or an
'overwrite' and re-instate document fonts. The font sub-system does already a
lot of things w.r.t. to fallback fonts (e.g., the selection of fonts that
correspond to generic names). It would have been much cleaner to fold the
appending logic there. My guess is that it was added in the style system because
it was felt an easier shortcut at the time.

Comment 5

[rbs]

and re-instate document fonts ...according to the CSS cascade at that point.

Comment 6

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

Attachment: guess at a fix (untested)

Comment 7

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

I'm not sure about the fixed/non-fixed stuff.  I also haven't tested any of the
rest :-)

Comment 8

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

patch works fine, thanks!

Just one important comment : let's all agree that the commas between values
of font-family should not have whitespaces before and after in getComputedStyle's
answer. This is what I see right now.

Comment 9

[Johnny Stenback (:jst)]

I would argue that there should be a space after the comma in the list of font
names for consistency and readability, why would we prefer to not have space
after comma?

Comment 10

[David Hyatt]

it's trivial to tokenize even with the spaces in.  why take them out?

Comment 11

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

jst : I agree with the readability factor.
hyatt: I did not take them out ; that's how getComputedStyle answers. It is
       trivial.

Comment 12

[rbs]

Did you try to see what happens when the prefs to disable document fonts is set?
(need a JS test a la hixie)

Comment 13

[Hixie (not reading bugmail)]

There should be spaces after the commas according to my c14n proposal:
   http://www.damowmow.com/mozilla/canon.txt

Comment 14

[David Hyatt]

--> dbaron, who supplied the patch.

r=hyatt

Comment 15

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

dbaron : can you update patch in order to add the missing whitespace after
commas _before_ check-in ? thanks !-)

Comment 16

[Johnny Stenback (:jst)]

sr=jst, but please fix the inconsistent space-before-and-after-equals in the patch.

Comment 17

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

glazman:  That's a nontrivial change -- it's not part of this code.  Could you
file a separate bug?

Comment 18

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

This still needs to be tested before it's checked in.  P2/0.9.5

Comment 19

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

dbaron: sure !

Comment 20

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

Attachment: testcase

Comment 21

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

Fix checked in 2001-09-06 19:28 PDT.  If someone wants to start filing bugs
based on Ian's c14n proposal, go ahead...

Comment 22

[R.K.Aa.]

sometime the past 24 hours code was checked in that broke national fonts at
http://www.vg.no  æ,ø,å now all display as ?
Noticed this fist in an 8 hour old CVS build. Linux 2001090711 has the same bug.
Any idea?

Comment 23

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

I highly doubt it would be caused by this change.  Could you file a separate
bug?  You can also test whether it was this change by backing it out in your own
tree (cvs up -j1.27 -j1.26 nsComputedDOMStyle.cpp (undo with cvs diff and then
reverse-applying the diff), or even just cvs up -r1.26 nsComputedDOMStyle.cpp
(undo with cvs up -A nsComputedDOMStyle.cpp)).