Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-inline] A question for the procedure to compute the resolved value of "line-height" #3749

Open
joonghunpark opened this issue Mar 20, 2019 · 43 comments

Comments

@joonghunpark
Copy link

commented Mar 20, 2019

I have a question about the procedure to compute the used value of "line-height".
I'm currently making a change on blink in
https://chromium-review.googlesource.com/c/chromium/src/+/1522053/3.
This change is for "line-height" to return used value instead of "normal keyword"
as resolved value.

In case of WebKit, fontMetrics().lineSpacing() value is used instead of 'normal' as below.

int RenderStyle::computedLineHeight() const
{
    const Length& lh = lineHeight();
    // Negative value means the line height is not set. Use the font's built-in spacing.
    if (lh.isNegative())
        return fontMetrics().lineSpacing();
    if (lh.isPercentOrCalculated())
        return minimumValueForLength(lh, computedFontPixelSize());
    return clampTo<int>(lh.value());
}

And Firefox returns used value "px" instead of "normal" also.

But I'm not sure what is the exact procedure to compute the used value for "line-height",
because I coundn't find that in spec.

Is it ok to use font metircs linespacing value like WebKit, or is there a exact procedure to compute the used value?

@frivoal

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2019

The logic for line-height described in css2.1 is known to be buggy.

It has been quite thoroughly reviewed and fixed, and css2.2 was made reliable. There remained some unsolved questions forline-height: normal, but it is a lot less wrong than what came before it.

However, for unrelated reasons (insufficiently careful edits had been made throughout the spec, so various parts were wrong, and which was which had become intractable), all changes to css2.2 had to be reverted to css2.1. We still haven't been able to re-apply all the known-to-be good changes.

This means the better definition of line height is currently in no spec at all. We should fix that, but until that happens, I recommend reading form the PR that did fix it at some point: #1993, while keeping an eye for possible issues. I'm aware of at least these:

  • #1254 (This one is partly solved, so you may want to skip the first half of the issue, but as the comments after #1254 (comment) show, there are still some unresolved points)
  • #2418
  • #1551

Based on all that, I am not sure that what you're trying to do is possible: unlike the other values of the property, line-height: normal does not result in single a height per element. Each text run may end up having a different height. To me, that means that returning 'normal' as a keyword makes more sense. Unless there's a compat constraint, I think CSSOM should be fixed accordingly (non-normal values should still be absolutized, but normal should stay as a keyword).

@joonghunpark

This comment has been minimized.

Copy link
Author

commented Mar 20, 2019

@frivoal

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2019

Agenda+ to check if we should change CSSOM to say that the resolved value of line-height: normal is normal

@fantasai fantasai changed the title [css-inline] A question for the procedure to compute the used value of "line-height" [css-inline] A question for the procedure to compute the resolved value of "line-height" Mar 20, 2019

@dbaron

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

I filed bug 1536871 to change this behavior in Gecko.

@css-meeting-bot

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

The CSS Working Group just discussed #3749.

The full IRC log of that discussion <dael> Topic: #3749
<dael> github: https://github.com//issues/3749
<dael> dbaron: Isn't this talking about value for GCS which is another term?
<dael> florian: Resolved value, you're right
<dael> florian: This is raised by chromium b/c cssom spec says resolved value is used value and they were trying to find a way to return number of pixels but line-height:normal doesn't resolve into a single height
<AmeliaBR> s/#3749/The resolved value of "line-height"/
<dael> florian: line-height has 2 sets of values. All values other than normal resolve to a single height, normal resolves in different value per text run. You ask for gCS for element, not for text run so we can't retunr
<dael> dbaron: Chromium returns normal as a keyword?
<dael> florian: Yes
<dael> nigel: Reviewed this and was confused by spec with resolved value b/c line-height just takes you to css2 line ehight and doesn't way what resolved value is for line-height. Doens't seem spec says what it should be
<dael> florian: Kind of does depending on how you read it. For line-height it's in the sublist where it says resolved value is used value. It's unclear for line-height:normal that can be returned
<dael> nigel: I think you're expalining how to read the sublists. THat prob needs reformatting. Thank you
<dael> florian: line-height:normal can only be preserved as normal if we want something meaningful
<dael> nigel: You want to move line-height to computed value category?
<dael> florian: Not necessarily. There is another subgrouping of values for line-height: length and % are turned into absolute values at computed but numbers are numbers and computed. At used numbers become lengths. Do same thing but inherit differently
<dael> florian: Normal, regardless of computed or used time can't become a single number.
<dael> AmeliaBR: Are you asking us to say normal exists at used value time or are you asking to create a special cat to figure out resolve value for line-height being normal if computed value is normal?
<dael> florian: I think difference between those 2 is editorial b/c used can't be observed unless we say gCS returns it. In terms of what' observable it's same. I don't care how we phrase, just don't turn normal into a number
<dael> nigel: Happy to have a unitless number for line-height converted to a used value?
<dael> florian: Unitless stay as unitless at computed. At used value I suppose it's cat. that way b/c web compat requires it. That's usually why prop whose resolved is mapped to used value instead of computed mapping. I'm not trying to change values other than normal, want to avoid normal being cast to a length when it can't be
<dael> florian: Who is CSSOM editor?
<dael> florian: Emilio
<dael> Rossen_: Right
<dael> Rossen_: You can make a PR and emilio can merge
<dael> florian: Happy with a resolution we keep normal and work with emilio on phrasing
<dael> nigel: You talked about what is returned when it's normal. Do we have data on UAs for unitless numbers?
<dael> florian: I have not looked. I assumed spec wasn't wrong, but can look separately.
<dael> AmeliaBR: Working on a test case. Chrome does what florian is asking for
<dael> myles: We're talking about return on gCS?
<dael> florian: Yes
<dael> myles: That's scary, line-height has been around forever
<dael> florian: Changing spec to match chrome
<dael> myles: FF and webkit return pixel value
<dael> florian: Can you define what value you return?
<dael> myles: THis issue has a snippit of code that desc how we compute
<dael> florian: [looks at it]
<dael> florian: I'm not terrified about web compat given chrome does it already
<AmeliaBR> Test case: https://codepen.io/AmeliaBR/pen/bZmgqJ?editors=1011
<dael> myles: otoh FF and webkit don't return keyword
<dael> florian: You think this is kind of code websites could have specialized on to deal with browser difference?
<dael> Rossen_: I think the assertion made was line-ehight has been there forever and many sites depend on it. Predicting what will be broken or not is difficult. I agree Chrome market shre prob dictates some behavior on desktop, but not sure on mobile
<dael> florian: I wouldn't say chrome dictates behavior, but chrome doing it this way is strong evidence that doing it this was doesn't break the web
<dael> florian: And length value you would return doens't match layout so why return that?
<dael> fantasai: Also computed value is the normal keyword. We use used value where there's a compat reason to do so
<dael> myles: I won't object but we wouldn't be able to make this change w/o more research into how this would break
<dael> Rossen_: Can always defer to next week if you want to get some time and come back to use myles
<dael> florian: I guess...the question is what's alternative? Spec being wrong is bad. Chrome returning a length that's unrelated to actually used length seems weird
<dael> AmeliaBR: Is it really wrong? It's based on font of element. THe text spans inside might have different fonts but doesn't the parent element have a line height even if it's not used for anything?
<dael> florian: From the code snippit I don't know which font metrics we're talking about
<dael> myles: Primary
<dael> florian: Which metric of that font?
<dael> myles: Ascent+descent+line gap
<fantasai> AmeliaBR, the line height for normal is calculated per fragment, not per element
<dael> florian: I'm not good enough at fonts to phase this, but I believe in the past we discussed there isn't a single ascent or descent and the one we use aren't always the same
<dael> myles: FOr a particular font there are different values, but every browser picks one and uses it throughout. At least for the browsers I know
<dael> florian: You return line height of the strut?
<dael> florian: strut is the css2.1 term for the thing that keeps the line height fromcollaping to 0
<dael> myles: Sounds right to me
<dael> fantasai: line height used is calc per fragment of element so it's not nec the line height from first avail font. Can also include fallback
<dael> myles: Correct, also includes things like child elements
<dael> fantasai: THat's about height on line-box. THere's difference between line-box and fragment we're looking at. For an element when calc line-height which is what's used when calc height of linebox in most cases it's definite nmber, but varies by fragment with normal
<dael> fantasai: THe position of that length on the line is used in combination with other elements on the line to get the line box. We're trying to calc the line-height value that factors into line box calc and that varies by fragment if you have normal. You can't get that answer w/o doing layout
<dael> florian: I guess this boils down to that number from webkit isn't entirely meaningless, but if we're saying this is the used value it actually isn't
<dael> fantasai: Example: have line-height:normal and all text is using second font in list that's what would set the line height, but you'd returnt he first font. Given the intention of gCS is reach the computed value and we don't have a web compat reason to not it seems to me returning normal makes a lot of sense.
<dbaron> I think there's value in allowing the discussion to proceed a little further in the issue before bringing it to the call; might be more likely to take less than 20 minutes.
<dael> Rossen_: We're at the top of the hour. I don't want to force a resolution. What's captured is compelling enough for WK and FF team to noodle on this for a week and we'll put it in next week's agenda.
<dael> florian: Sounds reasonable.
@MatsPalmgren

This comment has been minimized.

Copy link

commented Mar 20, 2019

I don't really understand why the line-height property is singled out. Pretty much all of the properties where the resolved value = used value in this list may have different used values on each fragment. E.g. padding-top:10% on a block that is fragmented has 0px on all fragments except the first, unless ofc box-decoration-break:clone was specified, and if so there's no guarantee that the percentage basis is the same for all fragments so they may have a different used value in that case too.

IIRC, Gecko returns the used value for the first fragment for all these properties. It seems inconsistent to me to single out line-height as an exception when other properties have the same issue. For that reason it seems better that Blink adopts the WebKit/Gecko behavior. A pixel-value also seems more useful to authors than normal, even if the value can potentially be different in other fragments if they exist, but as I said, that's no different from padding/border/margin etc.

(Ideally, there should be an API for getting the values of all fragments, but that's a separate issue.)

@emilio

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2019

Also, Gecko does have internal line-height values for form controls that right now are unobservable via the computed style (-moz-block-height) thanks to this property returning the used value. We'd need to figure out what to return for those, which is unfortunate.

@kojiishi

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

Happy to discuss what we can do for better web compat. The context was that, @joonghunpark wanted to contribute the change to Blink, but we didn't understand what the desired behavior is, hence he came here to ask.

Do other browsers match to WebKit? I read @litherum's explanation, and also @joonghunpark showed me the WebKit code. If this is what other browsers do, this is a simple fix, and fast to compute. We can try to see if we can safely change. This is not a used value though, so it maybe nice to update the spec.

If other browsers do it differently, we may need to discuss what is the desired behavior. I wish it be simple and fast since we need to compute it for getComputedStyle regardless author needs it or not, but I don't have other strong opinions.

@kojiishi

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

@MatsPalmgren

IIRC, Gecko returns the used value for the first fragment for all these properties. It seems inconsistent to me to single out line-height as an exception when other properties have the same issue.

Blink does that too, taking the first block fragment. This is the first case for Blink to handle inline fragment though, and there are more special cases to consider than other properties, so I'd like to understand what other browsers do.

@frivoal

This comment has been minimized.

Copy link
Collaborator

commented Mar 21, 2019

I don't really understand why the line-height property is singled out. Pretty much all of the properties where the resolved value = used value in this list may have different used values on each fragment

  • We're not talking about fragments here, but about text runs, or even parts of text runs. This may be considered sorta similar if you want to, but no fragmentation (either in the multicol/pagination sense, or in the line break sense) needs to occur here; we just need font fallback to happen. We may want to deal with these chunks the same way we deal with Fragments. Or not. I'd lean towards not, because I don't think it is specified anywhere if and how we're supposed to slice text runs into multiple chunks based on font fallback (and/or other things?), so it's hard to talk about the height of the first one until we agree on what's supposed to be in the first one.

  • Assuming I understand it correctly the cold snippet that was shared for webkit without looking up the functions it calls, and assuming it was described correctly, the behavior described as the webkit behavior does not give the height of the not the first chunk: If gives the height of a hypothetical chunk with text from the first available font. You may not have one of these, and even if you do, it may not be the first one. Let's say your font stack is "times new roman", "hiragino mincho", serif, that both times new roman and hiragino mincho are available, and that all the text in the element is in Japanese. The actual line height (of all text runs / fragments / chunks) will be based on hiragino mincho, but the getComputedStyle as described above would return the height of times new roman alone.

I think we could do a number of variants:

  1. return normal as a keyword.

  2. return the theoretical height based on the first available font, regardless of whether it is actually used.
    Possible nuance: the latest version of the spec text includes (based I believe on @dbaron saying that this was needed):

    User Agents may use different sets of font metrics when determining A and D
    depending on whether the 'line-height' property
    is 'normal' or some other value,
    for instance taking external leading metrics into account
    only in the 'normal' case.

    Since we're not returning something based on actual layout, which of the possible font metrics are we using? presumably the one that gets used for the normal case, but given that the code snippet in the first comment talks about "the metrics" of "the font" when there are multiple fonts each having multiple metrics, I am not sure this nuance is taken into account, so maybe not.

  3. return the actual height of the first inline box (i.e. from the beginning until a line break, or the start of a different inline, whichever comes sooner), enclosing all glyphs actually used, going from the tallest ascender to the deepest descender, including the strut.

    Variants:

    1. if the element starts with an atomic/replaced inline, count the height of the first inline box after that
    2. if the element starts with an atomic/replaced inline, return the height of the strut, i.e. the theoretical height based on the first available font.

    Further nuances:

    • If the anchor point of a float or an abspos comes in the middle of that box, does that count a cutting that box in two or not?
    • If we have some other formatting changes, within the line (say, a color change), does that count as splitting the box or not? If we have <p>aあ<span style=color:red>अ</span>, and we're asking for the line height of the <p>, we we work with the union of the Latin and Japanese fonts only, or do we include the Devanagari one as well? How about if the span is unstyled?
    • If the element starts with a block level descendent, do we look at the first inline box after that, or to we go pretend there's an empty inline at the start, and therefore return the height of the strut?
  4. return the actual height of the first non empty inline box chunk: based on the height of first glyph actually used (or equivalently, on the height of the first run of glyphs that all come form the same font).

(1) is what Blink does now. (2) Is what I believe webkit does now. If Firefox does something based "on the first fragment", I suppose it does one of the other variants, but I'm not sure which, or maybe it does (2) as well.

Doing anything other than (1) needs us to dig into the details of the various variants. Given that no use case has been brought up so far, and that Chrome shipping variant (1) is fairly strong evidence that this behavior is web compatible, I'd rather do that.

@emilio

This comment has been minimized.

Copy link
Collaborator

commented Mar 21, 2019

FWIW, what Gecko ends up doing is here:

https://searchfox.org/mozilla-central/rev/7c20ad925005fbad7b8e08813115f1ec7fa1c248/layout/generic/ReflowInput.cpp#2819

Using the font metrics of the first available font as specified by the style of the element. The default value for what to do is 2, which is eCompensateLeading, so:

https://searchfox.org/mozilla-central/rev/7c20ad925005fbad7b8e08813115f1ec7fa1c248/layout/generic/ReflowInput.cpp#2831

https://searchfox.org/mozilla-central/rev/7c20ad925005fbad7b8e08813115f1ec7fa1c248/modules/libpref/init/all.js#305

Then we adjust for the minimum font-size specified by the user (so that it's not reflected in the computed style), and for zoom here, though those are less interesting special-cases:

https://searchfox.org/mozilla-central/rev/7c20ad925005fbad7b8e08813115f1ec7fa1c248/layout/style/nsComputedDOMStyle.cpp#2578

So no post-layout-dependent value for normal.

@frivoal

This comment has been minimized.

Copy link
Collaborator

commented Mar 25, 2019

Thanks @emilio. Unless I misunderstand something, it seems that getComputedStyle on line-height: normal does not, unlike what @MatsPalmgren suggested it might do, return the height of the first fragment (regardless of how you define fragment).

So I still think that returning normal as a keyword, rather than some artificial measurement of what the line-height might end up being, is preferable. getComputedStyle is generally attempting to return the computed value, and mostly only returns post layout used values when compatibility requires. In this case, compat doesn't seem to require it (since chrome can without issue do something else), and what webkit and gecko are returning aren't actually post-layout used values.

If we're actually trying to expose font-derived metrics so that people can do useful things with them, I think we should rather look into that on the houdini side, rather than try and shoehorn them through an API that doesn't have sufficient granularity anyway.

@kojiishi

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

Thank you @emilio from me too, great to know Gecko doesn't return the used value.

Given Gecko and WebKit return somewhat similar logic and it's not expensive, I'm open to either option. Returning normal as proposed by @frivoal is fine. We can also try to match to WebKit if doing so is desired and see if the change can stick.

@css-meeting-bot

This comment has been minimized.

Copy link
Member

commented Mar 27, 2019

The CSS Working Group just discussed A question for the procedure to compute the resolved value of "line-height", and agreed to the following:

  • RESOLVED: return 'normal' for line-height
The full IRC log of that discussion <dael> Topic: A question for the procedure to compute the resolved value of "line-height"
<myles> by the way, everyone should remember to re-join the newly rechartered SVG WG
<dael> github: https://github.com//issues/3749
<dael> florian: Bit of discussion in GH was first a suggestion maybe line-height isn't special b/c many types of boxes that can frag and we always return first. Two buts, we're talking contiguous runs of text which is poorly defined to say first. This is not what Gecko or WK return, both return more theoretical rather than actual layout
<dael> florian: But it is similar.
<myles> q+
<dael> florian: I maintain given that we're not providing a post layout usable value we may as well preserve as keyword as Chrome does. BUt alternative isn't hard to spec so if people feel strongly, why not. I'm not sure there's a use case for it.
<dael> florian: If we want to drill down to font metrics it's more houdini space.
<astearns> ack myles
<dael> myles: I was reading convo from last week and I think I didn't make something clear. Philosophically I think you're right. There is no px value that will always be correct. My concern was web compat. If we do keyword and not number and line of JS tries to parse it could cause exceptions
<dael> myles: I brought up we should do analysis. Did investigate this week into how we can do.
<dael> florian: Queston on compat concern. Anything we change from any engine could break amount of web. The fact that Chrome ships other behavior tends to make me think it's prob okay. DO you have a specific reason to worry about this one or is it a general case of changing might break
<dael> myles: Why this might be different is line-height and gCS have been around forever. CHanging behavior that's been consistent in engine for decades is scary
<dael> fantasai: If we want interop someone needs to change. Either Chrome to return px or other engines return 'normal'
<dael> fantasai: Is that not correct?
<dael> TabAtkins: myles , given we have real life compat data that Chrome returns keyword, what information do you want obtained.
<dael> myles: A blunt tool is how many times gCS called where result is 'normal'
<dael> TabAtkins: Potentially obtainable, but a fairly invasive use counter
<dael> florian: Would that data from Chrome help? We know Chrome is fine. If there's a problem it's b/c websites are build differently perhaps on mobile where Chrome has less market share. So would it help?
<Rossen_> https://developer.microsoft.com/en-us/microsoft-edge/platform/usage/css/line-height/
<emilio> q+
<Rossen_> in case this helps a bit ^
<astearns> ack emilio
<nigel> q+ to suggest that although resolved value is not always the same as computed value, we should bias in favour of making resolved value closer to the computed value as a tie-breaker
<fantasai> http://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0Ax%0A%3Cscript%3E%0Aw(window.getComputedStyle(document.body).lineHeight)%3B%0A%3C%2Fscript%3E
<dael> emilio: I'm somewhat concerned...result of gCS on form controls have particularly weird computation. WK and Blink mangle line-height to give computed line-height. I suspect there are cases where if you have a select element if this change impl in Gecko it could return keywork and Blink returns pixel. I recall something where like if the height is fixed it becomes a fixed height value.
<astearns> ack nigel
<Zakim> nigel, you wanted to suggest that although resolved value is not always the same as computed value, we should bias in favour of making resolved value closer to the computed value
<dael> emilio: Willing to try and change, but that's my concern
<Zakim> ... as a tie-breaker
<dael> nigel: Observing this seems a bit of a tie here. Current behavior is return 'normal' and that'scloser to computed. Resolved isn't always computed, but being as close as possible seems a reasonable rule to use to break the tie.
<dael> fantasai: I think if Gecko is willing to try that should resolve web compat concerns. I agree that's prob direction to try and go in.
<dael> myles: If Gecko makes this change and there's no prob that's sufficient. I agree with fantasai . Don't need a use counter in that case
<dael> astearns: Way forward is spec we return 'normal' have Gecko try it out and hope for the best
<dael> fantasai: And if problem open issue
<dael> astearns: Obj to return 'normal' for line-height ?
<dael> RESOLVED: return 'normal' for line-height
<dael> astearns: Thanks emilio for being the guenia pig
<dael> emilio: I'll add a use counter and not blindly change, but happy to give a shot
@emilio

This comment has been minimized.

Copy link
Collaborator

commented Mar 27, 2019

Hmm, as I wrote the patch to do this in Gecko, I realized that returning the computed value for line-height is not as easy, since line-height: <number> is defined to compute to a number, but Blink returns a px for <number> line-height.

Is Blink willing to try changing this @kojiishi?

Should line-height compute to <length> instead? That seems somewhat undesirable though, as in Gecko we account for min-font-size for numeric line height, but not for Ems.

@MatsPalmgren

This comment has been minimized.

Copy link

commented Mar 28, 2019

@frivoal I don't see any discrepancy in what @emilio and I said about how the value is calculated above. He was just a lot more detailed and pointed to the actual code (and if you look at that code, mFrame in there is the first fragment I was referring to). Anyway, the only layout-dependent part of that code is -moz-block-height which we are unshipping. The remaining part is a fairly straightforward calculation from the font metrics of the first available font from the style, as specified in CSS2. I really don't see why this is controversial or complicated to implement (and as our code shows - it's not). Perhaps you misunderstood and thought I was referring to the line height of the first line-box or something? That's a separate concept and not what I meant. Sorry for any confusion.

@MatsPalmgren

This comment has been minimized.

Copy link

commented Mar 28, 2019

@frivoal said:

Chrome shipping variant (1) is fairly strong evidence that this behavior is web compatible

Well, Firefox and Safari is shipping the resolved pixel-value, which is fairly strong evidence that that behavior is web compatible too.

@MatsPalmgren

This comment has been minimized.

Copy link

commented Mar 28, 2019

I don't think we should return the computed value, given that no browser implements it like that. Given that Chrome is resolving <number> to a pixel-value, I don't see why it can't do the same for normal. It's fairly straight-forward to implement if you look at the links @emilio posted above. I don't see why it would be more complicated to implement in Blink.

I don't see any strong reason why Firefox/Safari should change their decades-old behavior that implements literally what the CSS2 spec says. I'd prefer that Chrome implement the spec instead.

@kojiishi

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

@emilio:

Is Blink willing to try changing this @kojiishi?

Created a test@jsbin, it looks like normal is the only case we differ. I can try, but it means all other browsers also need to change. We should probably not try to change behaviors where all browsers are interoperable today. Does this sound reasonable?

@MatsPalmgren:

I'd prefer that Chrome implement the spec instead.

We can try it out, but because no one implements the spec (used value) today, for Blink doing so will create another new interop problem to all of us. I would rather prefer to change the spec to allow Gecko and WebKit behaviors, and to try to change Blink to match to WebKit.

To recap, for normal, Edge and Blink return normal. Gecko and WebKit return an estimated value from font metrics (and some other info in Gecko), but the logic differ by large.

@litherum is concerned about the breakage, which is understandable. We know a lot of pages already detect browsers and run different code, so Blink and Edge returning normal today does not prove it's safe to change. With @joonghunpark motivated to work on this (thanks!), Blink can try to return an estimated value if WG is happy to change the spec.

IIUC @frivoal's point is that, since the returned length are not reliable nor interoperable, he prefers not to return any length. It has a point, and I'm fine with it too.

I'm also fine to allow both as I've not seen complaints from authors yet, but I guess we prefer not to do it.

@emilio

This comment has been minimized.

Copy link
Collaborator

commented Mar 29, 2019

Created a test@jsbin, it looks like normal is the only case we differ. I can try, but it means all other browsers also need to change. We should probably not try to change behaviors where all browsers are interoperable today. Does this sound reasonable?

Sure, but that means that either the css-inline definition needs to be changed to make numbers compute to length, or that CSSOM still needs to mention that line-height has a special resolved value, and define it properly. @frivoal, opinions?

@emilio

This comment has been minimized.

Copy link
Collaborator

commented Mar 29, 2019

Hmm, and the css-inline definition cannot change, since the computation is visible in terms of inheritance, so...

@emilio

This comment has been minimized.

Copy link
Collaborator

commented Mar 29, 2019

That is, the following should show 64px 64px 32px 32px if we show "resolved" values, 2 2 32px 32px if we show computed values:

<!doctype html>
<div style="line-height: 2; font-size: 16px">
  My line-height should be 32px.
  <div style="font-size: 32px">
    Mine should be 64px
  </div>
</div>
<div style="line-height: 2em; font-size: 16px">
  My line-height should be 32px.
  <div style="font-size: 32px">
    Mine should still be 32px
  </div>
</div>
<pre id="log"><script>
  for (const d of document.querySelectorAll("div"))
    document.writeln(getComputedStyle(d).lineHeight);
</script></pre>

I think resolving some values but not others is quite inconsistent.

@kojiishi

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

I agree it's inconsistent. I think what Gecko and WebKit returns today is not a used value, and that it's been inconsistent already. Do you agree what Gecko returns today is not a used value? If our understanding on this point is different, I think we should try to sync better first.

@kojiishi

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

This test@jsbin should show the same value 3 times if the resolved value is "the used value of the first fragment". No browsers do it today.

I'm fine with any of these options:

  1. The resolved value is normal, matching to Blink and Edge.
  2. The resolved value is "an approximate estimated value based only on the first available font of the element", matching to Gecko and WebKit.
  3. Allow both of above. No browsers need to change.

Either way we go, I think we should update the spec.

Does this explain my opinion better?

@emilio

This comment has been minimized.

Copy link
Collaborator

commented Mar 29, 2019

I disagree, what we return is the value for line-height that is used during layout. That may end up not being the actual height of the line, but the used value of the line-height property is what we return.

@emilio

This comment has been minimized.

Copy link
Collaborator

commented Mar 29, 2019

I guess there can be a difference in what the used value of the line-height property means... But yeah, I agree with you.

@MatsPalmgren

This comment has been minimized.

Copy link

commented Mar 30, 2019

@kojiishi I think the way you present the options is slightly misleading though. Edge will soon be based on Blink, so there's only one UA doing option 1, IMHO. So here's the list of options as I would present them:

  1. The resolved value is normal, matching Blink.
  2. The resolved value is the used value as specified by CSS2 for over a decade, matching Gecko and WebKit.
  3. Allow both of above. No browsers need to change.
@MatsPalmgren

This comment has been minimized.

Copy link

commented Mar 30, 2019

@emilio wrote:

I disagree, what we return is the value for line-height that is used during layout.

Indeed, me too. There seems to be some confusion in this issue about what the used value of the line-height property is. That's odd, because the spec is utterly clear on this:

normal
Tells user agents to set the used value to a "reasonable" value based on the font of the element. The value has the same meaning as <number>. We recommend a used value for 'normal' between 1.0 to 1.2.
<number>
The used value of the property is this number multiplied by the element's font size.

Given those definitions and the fact that Blink returns a pixel-value for <number>, it seems it could trivially implement what the spec says.

To highlight exactly how Gecko does it, here's the key lines.

@kojiishi

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2019

@MatsPalmgren I'm just asking the logic to be described more clearer in the spec, because our engineers could not read the desired logic from the spec. Thanks to you and @emilio pointing to Gecko's code, I think I can understand that. I also looked at WebKit's, which looks a bit different from Gecko's.

I'm fine with whichever options, or with the one @frivoal proposed, or with something else. My request is just to describe the logic more clearer to the spec so that we can implement. Did this explain better?

To explain our problem better, we were reading the Resolved Values spec, which says "The resolved value is the used value" for the line-height property. The Used Values spec says:

The used value is the result of taking the computed value and completing any remaining calculations to make it the absolute theoretical value used in the layout of the document. If the property does not apply to this element, then the element has no used value for that property.

We could not read this paragraph to the logic you quoted, hence asking better clarification in the spec.

@kojiishi

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

@frivoal @fantasai are you ok for Blink to try to match to WebKit? IIUC you want Gecko and WebKit to change to return normal, correct?

@frivoal

This comment has been minimized.

Copy link
Collaborator

commented Apr 15, 2019

@kojiishi I agree with your logic (in comment #3749 (comment)), and prefer option 1. I wouldn't object to option 2, but then I'd hope it would be defined precisely, which I don't think is the case today. option 3 is not terrible, but it just pushes the problem down the line.

@MatsPalmgren The bit of spec you quoted in #3749 (comment) is known to be wrong, as no browser implements normal as being equivalent to some number in the 1.0 to 1.2 range, and instead all browsers implement it as a union of the ascenders and descenders of fonts actually in use. We have a WG resolution and spec text to fix that (#1993), but unfortunately, not active CSS2.1 editor to apply the fix at the moment :( Either way, this is about the computed value, not the value returned by getComputedStyle, which may or may not be the same (and in this case, isn't).

@kojiishi

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

@frivoal

I agree with your logic...

I think your proposal is very reasonable, but when @joonghunpark is willing to try to change Blink, @litherum showed a mild concern on the change, and @MatsPalmgren is very strongly against, I think choosing easier option for all of us might be worth considering, as long as you and @fantasai can live with it.

@frivoal

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

I don't prefer returning a numeric value, but I don't oppose it either. If we chose to go that way, I would hope that we define how it's calculated though.

@kojiishi

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2019

Thank you @frivoal for allowing our lives easier. @fantasai can you live with this? I could not read your opinion from the IRC log.

I would hope that we define how it's calculated though.

I prefer that too. A proposed change to CSSOM Resolved Value would be moving line-height to its own group, saying like:

The resolved value is approximately estimated value from the first available font, even when the box does not have non-empty line box.

@MatsPalmgren are you ok with this?

@kojiishi

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

gentle ping, any idea to move this forward?

@dbaron

This comment has been minimized.

Copy link
Member

commented Jun 2, 2019

So I think one of the things that's difficult here is that different people in the conversation have different understandings of how line-height works at layout-time. This is partly a result of the extended discussions we had a few years ago at the Tokyo face-to-face meeting (at Keio University's Mita campus), which are not currently reflected in any spec. These discussions included the questions of how line-height works on elements that include multiple fonts within their text, and what answers to that problem produced better results for baseline consistency if the font fallback only occurred within some lines of a block or within some elements within the lines of a block, and what normal means in the context of these conclusions and in the presence of different internal and external leading metrics (and perhaps whether those metrics should be used for line-height: normal) across the fonts used. I think there were some other issues as well. If there's at least a clear reference describing those topics and our conclusions that we can point the participants in this conversation to, that might help clear things up a bit.

Then there's additional the question of whether the things that we decided at that meeting are actually web-compatible, and also the other questions of what to do about the non-interoperable cases that we didn't decide about at that meeting. Getting things written in a spec sooner rather than later would also help answer this... and may even affect the answer (since changes that are web-compatible now might not be five years from now).

@css-meeting-bot

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

The CSS Working Group just discussed resolved value of line-height.

The full IRC log of that discussion <emilio> Topic: resolved value of line-height
<Rossen_> github: https://github.com//issues/3749
<emilio> github: https://github.com//issues/3749
<emilio> AmeliaBR: what's the question to be resolved?
<emilio> florian: this is about the resolved value of `line-height: normal`
<emilio> koji: right now Blink behaves differently from Gecko / WebKit
<heycam> koji: one of our contributors is willing to try to change Blink
<heycam> koji: seems smoother for Blink to change, want to confirm that with WG
<fantasai> ScribeNick: fantasai
<fantasai> scribenick: heycam
<heycam> emilio: Gecko and WebKit already have this behavior since CSS 2
<heycam> ... CSS2 defines the used value of line-height as something based on the primary font of the element
<heycam> ... I did try to switch Gecko to return normal, and it's probably not impossible, but it's a bunch of work
<heycam> ... also mats disagreed with it
<heycam> florian: what CSS 2 has to say is fuzzy
<heycam> ... different editions
<heycam> ... all browsers are interoperable about what line-height:normal does, to an approximation
<heycam> ... that is different from what CSS 2.1 says
<heycam> ... 2.1 claims normal is equivalent ot a number
<heycam> ... that's not what everyone does
<heycam> ... therefore it would make sense that for resolved value, you return the word "normal"
<heycam> emilio: there is
<heycam> florian: there isn't
<heycam> ... there is only if the element is empty
<heycam> dbaron: part of the problem is we had a long discussion at Tokyo F2F about line height
<heycam> ... and there's nothing written down that reflects the results of that
<heycam> ... in a readable format
<heycam> ... some edits made to CSS 2 and reverted
<heycam> ... there's no usable reference for the results of that discussion, therefore some people involved here inlc emilio and mats, who don't know what's discussed there
<heycam> florian: I'm frustrated about these edits being reverted
<heycam> ... I'll take those edits and put them into css-inline
<heycam> ... skipping oer the detalis, the behavior of line-height:normal is not euivalnet to a number
<heycam> emilio: you copmute a number at the start of the element (and the element itself), but once you get that number, which is what Gekco and WK return from gCS, it's the same as if you use that px value ...
<heycam> dbaron: it's not
<heycam> ... there are 2 ways it's different at least
<heycam> koji: I think dbaron and florian are talking about how line-height:normal is laid out
<heycam> ... emilio is talking about how line-height is computed
<heycam> ... and that is interop between Gecko and WK
<heycam> dbaron: what florian and I are saying is that there does not exist a number or a px value that is equivalent to normal
<heycam> ... because depending on the text you have in the element, adn what font fallbcak occurs, you'll get different results
<heycam> koji: that's for layout
<heycam> florian: the computed value is normal as a keyword
<heycam> dbaron: I don't think the point you're making is relevant here, I think florian's argument is that we're looking at situations where the computed value is normal, from a style perspective
<heycam> ... and we're asking what resolved value to report there
<heycam> ... florian's point is that in most cases when we have a reoslved value, putting that resolved value back in the computed value is basically euqivalent
<heycam> ... so there are cases where the computed value is a % and resolved value is px
<heycam> ... but if you take the resolved value and put it back in computed value, you would get the same result (at least for that element, not necessarily for inheritance etc.)
<heycam> ... florian's point is that in this case, there is not a single number that leads to that result, because what normal does around say font fallback, looking at metrics of possibly one possibly multiple fonts, is different from what any particulra number does
<heycam> ... that's the stuff that is not well documented because the CSS 2.1 edits were reverted
<heycam> florian: the right value to return from gCS would be normal
<heycam> ... but if there's broad interop everywhere but Chrome, to return what that number would be if the element is empty, then that's unfortuate but not unfortunate enough to object to that
<heycam> emilio: I was going through the Gecko code, we have various bits of if line-height is normal, update the line
<tantek> reverting the 2.1 edits (there were a bunch more) was the right thing to do because none of it was based in minuted discussions nor even test cases, and that kind of editing for a stable document like 2.1 is irresponsible
<heycam> dbaron: there are a bunch of things in Gecko code based on line-height normal, a bunch are wrong
<heycam> ... some were discussed at that previous meeting,
<heycam> dbaron: a bunch of the conditions in the gecko code are in the wrong place
<heycam> ... there's a bunch of design decisions important for interactions with other features, due to hte lack of interop, we probaly still have the freedom to make
<heycam> emilio: after seeing our usage of line-height:normal in our layout engine, I agree that given normal is more special, the right thing to do would be to return normal
<heycam> ... I can try again to change Gecko, and if I can't I can report back
<heycam> florian: getting interop would be nice
<Rossen_> q?
<heycam> ... if the only way for that is to return a number, that's OK, otherwise let's try normal
<tantek> from my recollection, line-height normal was a way used (by implementation) to capture the legacy Netscape behavior of inline line layout, as opposed to what Håkon & Bert wanted line-height to do
<heycam> myles_: philosophically returning normal would be great
<heycam> ... might break the web
<heycam> florian: chrome has been returning normal for a long time, so maybe wouldn't?
<heycam> ... maybe there is UA sniffing that would maek it break
<heycam> emilio: given this conversation I can try to convince mats
<heycam> florian: I will try to take the CSS 2.1 fixes and put them in Level 3
<heycam> emilio: notes in the spec about how that not only affects resolved value, but other stuff, would be great
<heycam> koji: dbaron's explanation I finally understand why we want normal, it makes sense
<heycam> ... but I also want interop. is WK willing to change?
<heycam> myles_: I don't think I would chane this before Gecko does
<heycam> ... if Gecko succeeds we'd probably be willing to do it
<heycam> dbaron: my memory of the discussions we had in Tokyo, I'm not 100% sure we all agreed that we want to stick to a behavior that normal cannot be mapped to a number
<heycam> ... that number might depend on something in the first available font, it almost certianly would, but I'm not sure we agreed that we do not want hte behavior that you could'nt convert normal to an equivalent number based on available font metrics
<heycam> ... that is using font metrics but not particular character availability
<heycam> florian: how about we reopen that issue as necessary, based on the Tokyo text, which describes ~reality
<heycam> ... and do that based on the text
<heycam> myles_: are you suggesting you want to change hte behavior to make normal not magic? or numbers be more magic?
<heycam> dbaron: change normal so that its magic is a bit different
<heycam> ... e.g. how font fallback interacts with line rhythm
<heycam> ... one piece there is that maybe it is better if the box that normal gives you is only a function of hte first available font, and not hte fallback fonts
<heycam> ... then when you have multiple lines/elements, some of which trigger fallbcak, you don't get unstable line rhythm
<heycam> ... I'm not sure we sorted that out fully
<heycam> myles_: I have opinions on this but maybe that's a different topic
<heycam> dbaron: the reason I'm bringing it up, is it relates to the conclusion that there does not exist an equivalent number
<heycam> ... but that's a full day long discussion
<heycam> florian: I agree there's room for discussion
<heycam> ... I propose we first get back the text, and go from there
<heycam> dbaron: sure
@zcorpan

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

WebKit bug for the line-height change: https://bugs.webkit.org/show_bug.cgi?id=201296

@joonghunpark

This comment has been minimized.

Copy link
Author

commented Sep 6, 2019

I've read the reached conclusion here now, and happy to see that.
Gecko has https://bugzilla.mozilla.org/show_bug.cgi?id=1536871 for that,
and Chrome already has the behavior.
Then I will take https://bugs.webkit.org/show_bug.cgi?id=201296 on WebKit,
and address it.

@emilio

This comment has been minimized.

Copy link
Collaborator

commented Sep 7, 2019

Note that we still haven't shipped to release that change in Gecko. Though we haven't heard any compat issues since it landed, so we should probably try to do that.

@emilio emilio reopened this Sep 7, 2019

@emilio

This comment has been minimized.

Copy link
Collaborator

commented Sep 7, 2019

https://bugzilla.mozilla.org/show_bug.cgi?id=1579624 should enable it for all users starting with Firefox 71. If we get no reports after a while I guess closing this should be fine.

@joonghunpark

This comment has been minimized.

Copy link
Author

commented Sep 10, 2019

With https://bugs.webkit.org/show_bug.cgi?id=201296,
now WebKit has the same behavior with Blink and Gecko, FYI.

zdobersek pushed a commit to Igalia/webkit that referenced this issue Sep 10, 2019
jh718.park@samsung.com
getComputedStyle for line-height: normal should return the keyword in…
…stead of a length

https://bugs.webkit.org/show_bug.cgi?id=201296

LayoutTests/imported/w3c:

Reviewed by Ryosuke Niwa.

Per w3c/csswg-drafts#3749,
Gecko and Blink has this behavior already.

This patch makes WebKit has the same behavior with them.

* web-platform-tests/css/css-inline/parsing/line-height-computed-expected.txt: Added.
* web-platform-tests/css/css-inline/parsing/line-height-computed.html: Added.
* web-platform-tests/css/cssom/getComputedStyle-line-height-expected.txt: Added.
* web-platform-tests/css/cssom/getComputedStyle-line-height.html: Added.
* web-platform-tests/html/rendering/replaced-elements/the-select-element/select-1-line-height-expected.html: Added.
* web-platform-tests/html/rendering/replaced-elements/the-select-element/select-1-line-height.html: Added.

Source/WebCore:

Reviewed by Ryosuke Niwa.

Per w3c/csswg-drafts#3749,
Gecko and Blink has this behavior already.

This patch makes WebKit has the same behavior with them.

Tests: imported/w3c/web-platform-tests/css/css-inline/parsing/line-height-computed.html
       imported/w3c/web-platform-tests/css/cssom/getComputedStyle-line-height.html
       imported/w3c/web-platform-tests/html/rendering/replaced-elements/the-select-element/select-1-line-height.html

* css/CSSComputedStyleDeclaration.cpp:
(WebCore::lineHeightFromStyle):

LayoutTests:

Reviewed by Ryosuke Niwa.

Per w3c/csswg-drafts#3749,
Gecko and Blink has this behavior already.

This patch makes WebKit has the same behavior with them.

* css3/calc/line-height-expected.txt:
* fast/css/font-calculated-value-expected.txt:
* fast/css/font-calculated-value.html:
* fast/css/font-shorthand-from-longhands-expected.txt:
* fast/css/font-shorthand-from-longhands.html:
* fast/css/font-shorthand-line-height-expected.txt:
* fast/css/font-shorthand-line-height.html:
* fast/css/getComputedStyle/computed-style-expected.txt:
* fast/css/getComputedStyle/computed-style-font-expected.txt:
* fast/css/getComputedStyle/computed-style-font.html:
* fast/css/getComputedStyle/computed-style-without-renderer-expected.txt:
* fast/ruby/ruby-line-height-expected.txt:
* fast/ruby/ruby-line-height.html:
* fast/text-autosizing/ios/idempotentmode/line-height-boosting-expected.txt:
* fast/text-autosizing/ios/idempotentmode/line-height-boosting.html:
* fast/text-autosizing/ios/line-height-text-autosizing-expected.txt:
* fast/text-autosizing/ios/line-height-text-autosizing.html:
* media/track/track-cue-rendering-on-resize-expected.txt:
* media/track/track-cue-rendering-on-resize.html:
* platform/gtk/fast/css/css2-system-fonts-expected.txt:
* platform/gtk/fast/css/getComputedStyle/computed-style-expected.txt:
* platform/gtk/fast/css/getComputedStyle/computed-style-without-renderer-expected.txt:
* platform/gtk/svg/css/getComputedStyle-basic-expected.txt:
* platform/ios/TestExpectations:
* platform/ios/css3/calc/line-height-expected.txt:
* platform/ios/fast/css/css2-system-fonts-expected.txt:
* platform/ios/fast/css/getComputedStyle/computed-style-expected.txt:
* platform/ios/fast/css/getComputedStyle/computed-style-without-renderer-expected.txt:
* platform/ios/svg/css/getComputedStyle-basic-expected.txt:
* platform/mac-sierra/fast/css/getComputedStyle/computed-style-expected.txt:
* platform/mac-sierra/fast/css/getComputedStyle/computed-style-without-renderer-expected.txt:
* platform/mac-sierra/svg/css/getComputedStyle-basic-expected.txt:
* platform/mac/fast/css/css2-system-fonts-expected.txt:
* platform/mac/fast/css/getComputedStyle/computed-style-expected.txt:
* platform/mac/fast/css/getComputedStyle/computed-style-without-renderer-expected.txt:
* platform/mac/fast/text-autosizing/ios/line-height-text-autosizing-expected.txt:
* platform/mac/svg/css/getComputedStyle-basic-expected.txt:
* platform/wincairo/css3/calc/line-height-expected.txt:
* platform/wincairo/fast/css/css2-system-fonts-expected.txt:
* platform/wpe/fast/css/css2-system-fonts-expected.txt:
* platform/wpe/fast/css/getComputedStyle/computed-style-expected.txt:
* platform/wpe/fast/css/getComputedStyle/computed-style-without-renderer-expected.txt:
* platform/wpe/svg/css/getComputedStyle-basic-expected.txt:
* svg/css/getComputedStyle-basic-expected.txt:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@249686 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.