371041 - Make nsComputedDOMStyle::GetLineHeightCoord handle "normal"

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Right now, the computed style line-height is the string "normal" if the computed value is "normal".  We should probably try to return an actual px value instead.

See also bug 365932 comment 3.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So I'm trying to decide how computed line-height should interact with text zoom and min font size.

For example, right now if a node has a line-height specified as a length, doing:

  foo.style.lineHeight = doc.defaultView.getComputedStyle(foo, "").lineHeight;

changes the line-height if the document has been zoomed (because the size stored in the nsStyleStruct is already zoomed).

Is that the behavior we want from computed style?  Or should it try to undo the effects of zooming and min font size?

Comment 2

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

Hrm.  It's probably better if it undoes the effects of zooming and minimum font size.  (I think the spec used to have wording about the computed line-height depending on the actual font-size, but if it did, it's not there anymore.)  However, I think it's fine to be consistent with what we do for the other value types for now.

Did you mean <number> rather than <length>?

Comment 3

[Boris Zbarsky [:bzbarsky]]

No, I meant <length>.  It happens for <number> too, of course.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

I still need to write some tests, and the undoing for "normal" seems to have a 1px rounding thing at some font zooms, but I'm not sure I could avoid that even if I used font->mSize (since that's zoomed too), and this way I get to reuse our existing line-height code...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: tests

Comment 6

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

Could you provide a brief explanation of the patch?

Comment 7

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

Comment on attachment 256135 [details] [diff] [review]
Patch

Er, never mind, it's not that bad:

The aPresContext argument to nsHTMLReflowState::CalcLineHeight should
be removed.

nsComputedDOMStyle::GetLineHeightCoord should return void instead of
PRBool.

Have you tested that the adjustment you're doing in  
nsComputedDOMStyle::GetLineHeightCoord does the right thing for all of:
 * the normal case (where I'm hoping it even avoids rounding errors)
     ... would there be an advantage to parenthesizing
           float(font->mSize) / float(font->mFont.size)
         so that it would be 1.0 exactly in the normal case?
 * just text zoom
 * just min font size
 * both text zoom and min font size
?  (The expression makes my head hurt.)

r+sr=dbaron

Comment 8

[Boris Zbarsky [:bzbarsky]]

> The aPresContext argument 

Removed.

> nsComputedDOMStyle::GetLineHeightCoord should return void 

It's being used as a percentage-base callback in one of the calls to SetValueToCoord (in GetVerticalAlign()), so it needs to return a PRBool, unfortunately.

>     ... would there be an advantage to parenthesizing

Yes, good idea.

And yes, I did test the adjustment in all four cases.  I really wish it were less sucky an adjustment... :(

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch with comments all addressed, including tests

Comment 10

[Boris Zbarsky [:bzbarsky]]

Checked in.

Not marking in-testsuite+, because we should really have those min font size and text zoom tests here too...