nsStyleFont::CalcDifference() too liberal

VERIFIED FIXED in mozilla1.0



CSS Parsing and Computation
17 years ago
15 years ago


(Reporter: Chris Waterson, Assigned: Chris Waterson)



Firefox Tracking Flags

(Not tracked)



(1 attachment)



17 years ago
While investigating bug 90205, I discovered that nsStyleFont::CalcDifference()
will return NS_STYLE_HINT_REFLOW if the font's mFlags differ.

This seems a bit too liberal, because at least two of the flags
(NS_STYLE_FONT_[FACE|SIZE]_EXPLICIT) are write-only. (The only code that uses
them is some #if 0'd form control code -- see nsFormControlHelper.cpp:626;
perhaps we ought remove these flags altogether?)

The last flag (NS_STYLE_FONT_USE_FIXED) simply blasts the fixed font into the
normal font slot AFAICT. So...

Do we care if the flags differ at all? Could we just remove the flag comparison
code in CalcDifference() and let the normal font comparison routines do their thing?

Comment 1

17 years ago
Created attachment 42860 [details] [diff] [review]
proposed change, diff -ub


17 years ago
Priority: -- → P4
Target Milestone: --- → mozilla1.0

Comment 2

17 years ago
If the flag NS_STYLE_FONT_USE_FIXED changes, I think we need to reflow since the
layout will change to use or not use the fixed font, right? 

Other than that, I think you are probably right about the other flags (in other
words, we should remove the unused flags and restrict the flag comparison to
just the USE_FIXED flag - unless that turns out to be unnecessary too).


Comment 3

17 years ago
But at nsRuleNode.cpp:1527 (where we actually fill in the nsStyleFont's data),
we explicitly reset mFont to mFixedFont. (This is the only place that the
NS_STYLE_FONT_USE_FIXED is read.) With that in mind,
nsStyleFont::CalcFontDifference() should do the right thing, no?

Comment 4

17 years ago
Yup, you're right. Since nsStyleFont::CalcDifference() computes the difference
between both fonts (fixed and non-fixed) it will correctly handle the swith made
in the ruleNode. 

Comment 5

17 years ago

Comment 6

17 years ago

Comment 7

17 years ago
In other words, I can get rid of mFlags from nsStyleFont in my patch at that 
other bug 84398, since I don't use NS_STYLE_FONT_USE_FIXED anymore. A little 
more savings.

Comment 8

17 years ago
Please get in touch with rods@netscape.com before removing these flags.

Comment 9

17 years ago
Rod is on sabbatical - he won't be back for 5 more weeks. If he really objects,
he can always add back the dead, unused and ancient code (he #if 0'd it on Jan
25, 2000).

Comment 10

17 years ago
Rod's on sabbatical? Cool! I'm going to re-enable the native widgets next week!

Comment 11

17 years ago
Changes checked in.
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 12

17 years ago
Adding this here for the record. As I was about to remove mFlags from
FONT_20011307_BRANCH, it occurred to me that one information carried
by these flags was to differentiate between the following two situations:

<span style="font-size:24pt">
  <span style="font-size:50%"> ... text at 12pt ... </span>

vs: (note: '50%' above could be something else, e.g., 'smaller' with a different 
parent font, and there could be an hierarchy of such elements)

<span style="font-size:24pt">
  <span style="font-size:12pt"> ... text at 12pt ... </span>

But the direct test on the final resolved font structs would tell if there is 
any difference and ensure that a reflow hint is returned. So I do not see why 
mFlags should be kept if it is write-only.

There is a subtelty as to whether the CalcLength()/CalPointSize() functions (the 
functions that maps the non-fixed values to the final fixed values) can lead to
edge cases that warrant having the test against mFlags in CalcFontDifference() 
somewhere in the StyleContext hierarchy. So far, I can't picture such edge 
cases that would not be detected by the direct test on the final resolved font 
structs. So off I go, I am going to remove mFlags in FONT_20011307_BRANCH 
because it is write-only there.

Comment 13

16 years ago
would the crash at http://dhtmlkitchen.com/dhtml/dom/extendingDOM.html be
related to this?

Comment 14

15 years ago
since the code was checked in more than a year and a half ago, marking the bug
You need to log in before you can comment on or make changes to this bug.