Closed
Bug 94078
Opened 23 years ago
Closed 23 years ago
getComputedStyle adds "serif" to correct answer for 'font-family'
Categories
(Core :: DOM: CSS Object Model, defect, P2)
Core
DOM: CSS Object Model
Tracking
()
RESOLVED
FIXED
mozilla0.9.5
People
(Reporter: glazou, Assigned: dbaron)
References
Details
(Keywords: css1, dom2, regression)
Attachments
(2 files)
2.12 KB,
patch
|
Details | Diff | Splinter Review | |
1.26 KB,
text/html
|
Details |
if you query the value of the 'font-family' property using getComputedStyle, it appends ",serif" to the correct answer.
Reporter | ||
Comment 1•23 years ago
|
||
Adding blocking dependency to "CSS support in composer" tracking bug.
Blocks: 77705
Updated•23 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.4
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•23 years ago
|
||
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.
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.
and re-instate document fonts ...according to the CSS cascade at that point.
Reporter | ||
Updated•23 years ago
|
Whiteboard: regression,correctness,dom2,css1
Assignee | ||
Comment 6•23 years ago
|
||
Assignee | ||
Comment 7•23 years ago
|
||
I'm not sure about the fixed/non-fixed stuff. I also haven't tested any of the rest :-)
Reporter | ||
Comment 8•23 years ago
|
||
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•23 years ago
|
||
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?
Whiteboard: regression,correctness,dom2,css1 → correctnes
Comment 10•23 years ago
|
||
it's trivial to tokenize even with the spaces in. why take them out?
Reporter | ||
Comment 11•23 years ago
|
||
jst : I agree with the readability factor. hyatt: I did not take them out ; that's how getComputedStyle answers. It is trivial.
Comment 12•23 years ago
|
||
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•23 years ago
|
||
There should be spaces after the commas according to my c14n proposal: http://www.damowmow.com/mozilla/canon.txt
Keywords: qawanted
Updated•23 years ago
|
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Comment 14•23 years ago
|
||
--> dbaron, who supplied the patch. r=hyatt
Assignee: hyatt → dbaron
Status: ASSIGNED → NEW
Reporter | ||
Comment 15•23 years ago
|
||
dbaron : can you update patch in order to add the missing whitespace after commas _before_ check-in ? thanks !-)
Comment 16•23 years ago
|
||
sr=jst, but please fix the inconsistent space-before-and-after-equals in the patch.
Assignee | ||
Comment 17•23 years ago
|
||
glazman: That's a nontrivial change -- it's not part of this code. Could you file a separate bug?
Assignee | ||
Comment 18•23 years ago
|
||
This still needs to be tested before it's checked in. P2/0.9.5
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: correctnes
Reporter | ||
Comment 19•23 years ago
|
||
dbaron: sure !
Assignee | ||
Comment 20•23 years ago
|
||
Assignee | ||
Comment 21•23 years ago
|
||
Fix checked in 2001-09-06 19:28 PDT. If someone wants to start filing bugs based on Ian's c14n proposal, go ahead...
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 22•23 years ago
|
||
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?
Assignee | ||
Comment 23•23 years ago
|
||
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)).
You need to log in
before you can comment on or make changes to this bug.
Description
•