366138 - The line-height should refer to adjusted font size, not 'font-size'

Status: NEW

(Core :: CSS Parsing and Computation, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

The 'line-height' with 'font-size-adjust' doesn't work fine if the specified value is relevant units(<number>, <percentage> and em).

http://hixie.ch/tests/adhoc/css/fonts/size-adjust/004.xml
http://hixie.ch/tests/adhoc/css/fonts/size-adjust/005.xml
http://hixie.ch/tests/adhoc/css/fonts/size-adjust/006.xml
http://hixie.ch/tests/adhoc/css/fonts/size-adjust/009.xml

These testcases fail to calculated the line-height. (The lines are overlapped.)
And I think that if the unit is ex, the adjusted value may be used for inherit.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: testcase

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #0)
> And I think that if the unit is ex, the adjusted value may be used for inherit.

Please ignore this. The length doesn't inherit from specified value...

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

fix.

We should use actual font size for any relative sizes.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 259881 [details] [diff] [review]
Patch rv1.0

Sorry, we should use nsIRenderingContext::GetEmHeight instead of GetHeight.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.1

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.1

Sorry. The patch has a gfx code. We need some easy work in gfx after this.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

dbaron:

Would you review this?

Comment 8

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

I have mixed feelings about this.  I lean towards thinking that this bug should be WONTFIX, but I'd sort of like to fix it as well -- I just don't think we can.

We have three options for what should depend on actual font size rather than computed font size:
 1. 'em' units and numeric line-heights
 2. just numeric line-height
 3. neither

CSS 2.1 says that (1) is incorrect:
# The 'em' unit is equal to the computed value of the 'font-size'
# property of the element on which it is used. The exception is when
# 'em' occurs in the value of the 'font-size' property itself, in
# which case it refers to the font size of the parent element.
 -- http://www.w3.org/TR/CSS21/syndata.html#length-units
That said, CSS2.1 doesn't have font-size-adjust.  But I suspect authors (and tests) do depend on 'em' being a multiple of the computed font size.  Do our reftests even pass with this patch?

We used to do (2), I think, but we changed it because it was confusing to have 1.2 and 1.2em behave differently in ways other than how they inherited.

That said, (3) can lead to overlapping text when authors use numeric line-heights relatively close to 1.0, and 'normal' isn't very good for authors because it's too little spacing for most uses.

Comment 9

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

That said, the spec does support doing (2) based on
http://www.w3.org/TR/CSS21/visudet.html#propdef-line-height , which is also all Hixie's tests require.  But we've had problems with something similar in the past, when we made the factor a multiple of a number out of the font metrics rather than the computed font size (perhaps even what you're doing in this patch, excluding the handling of font-size-adjust).

Comment 10

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

The bug that (2) would regress is bug 196270.

Comment 11

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

Comment on attachment 260565 [details] [diff] [review]
Patch rv1.1

In any case, I'm not sure what we do want to do here, but I don't think this patch is what we want.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I'm resetting bugs which are assigned to me but I'm not working on them and I don't have plan for fixing them in near future.

Comment 13

[Michael Kohler [:mkohler]]

This is a mass change. Every comment has "assigned-to-new" in it.

I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.