216456 - 'font' with system font keyword and 'font-family' with generic font family keyword behaving incorrectly

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Stanimir Stamenkov]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b) Gecko/20030815 Netscape/7.1 (ax; svg)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b) Gecko/20030815

Read the notes in the test case for more info.

Reproducible: Always

Steps to Reproduce:
1. Open the test case.
2. Observe the results.
3. Check with the descriptions.

Actual Results:  
Using: 

{
  font: message-box;
  font-family: sans-serif;
}

The 'font-family' statement resets all other font properties which should(!) be
set by the 'font' shorthand property. This is a case with system font keyword
used with 'font' and later generic font family keyword applied to the
'font-family' property.

Expected Results:  
The 'font' shorthand property should set all other font properties in compliance
with the CSS spec therefore the above test case should not to fail.

Comment 1

[Stanimir Stamenkov]

Attachment: A test case showing the problem.

Comment 2

[Stanimir Stamenkov]

Attachment: Mozilla screen shot

Mozilla screen shot of the browser window on my system.

Comment 3

[Stanimir Stamenkov]

Attachment: IE screen shot

Internet Explorer screen shot of the browser window on my system. Here, IE
shows the test case fine unlike Mozilla.

Comment 4

[Stanimir Stamenkov]

Could it be related to Bug 120903?

Comment 5

[Boris Zbarsky [:bzbarsky]]

To do this per CSS spec we'd probably need to resolve the system fonts into a
family/size/etc in the CSS parser instead of whenever we actually end up doing it...

Comment 6

[Ryan McCue]

Attachment: Simplified Test Page (INVALID)

Comment 7

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

Bug 377947 was supposed to fix this, but it looks like it didn't.

Comment 8

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

Comment on attachment 204104 [details]
Simplified Test Page (INVALID)

This testcase is invalid: system fonts are used in the font shorthand as only a standalone value; they can't be followed by a font-family value.

Comment 9

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

If I delete the second item of the testcase, the third one is corrected.  That suggests there's a problem with nsRuleNode::ComputeFontData in the case where aStartStruct is non-null.

Comment 10

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

Attachment: patch

This patch works, and also fixes bug 199755 fully.  I'd like to think about it a little more before requesting review.

This code goes back to the beginning of these functions:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/html/style/src/nsRuleNode.cpp&rev=3.22&mark=1913,1918-1920,1923-1925,1966-1968#1912

Comment 11

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

Comment on attachment 271007 [details] [diff] [review]
patch

We can't stomp on aFont as temporary storage because it's sometimes the only copy of aStartStruct we have, and we're not allowed to modify it.

I'm comfortable passing parentFont twice, since the COMPUTE_START_* macros often set up the parent struct as the same as the struct, so we have to be able to handle that in general.  And the code looks like it handles it fine.

This code still scares me a little, but I think this is right.

Comment 12

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

Comment on attachment 271007 [details] [diff] [review]
patch

This seems to break generic fonts entirely, though.

Comment 13

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

I think the nsRuleNode code added in bug 74186/bug 99010 is fundamentally incompatible with the aStartStruct parameter; fixing this probably just depends on bug 380915.

Comment 14

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

Attachment: patch

...but I can effectively skip the optimization by changing SetGenericFont, which fixes the major problems with this interaction (although I think some remain; see the XXXldb comment being added).

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 271263 [details] [diff] [review]
patch

>+    if (i != 0)
>+      fontData.mFamily.Reset(); // avoid unnecessary operations in SetFont()

Document that this is because i == 0 is our own context and we do care about the font-family then?

r+sr=bzbarsky.  Sorry for the delay; I have an irrational aversion to reviewing code I don't think I fully understand, so this took awhile.  :(

Comment 16

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

Fix checked in to trunk.