Bug 195180 - [css-values-4] Support font-relative lh and rlh unit
Summary: [css-values-4] Support font-relative lh and rlh unit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: http://w3c-test.org/css/css-values/lh...
Keywords: InRadar, W3CTest, WebExposed, WPTImpact
: 204277 (view as bug list)
Depends on: 211351
Blocks:
  Show dependency treegraph
 
Reported: 2019-02-28 12:17 PST by Gérard Talbot
Modified: 2020-06-15 14:43 PDT (History)
19 users (show)

See Also:


Attachments
Patch (42.57 KB, patch)
2020-04-04 15:14 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (42.74 KB, patch)
2020-04-04 15:42 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (45.63 KB, patch)
2020-04-04 20:29 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (45.62 KB, patch)
2020-04-04 21:44 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (45.64 KB, patch)
2020-04-04 23:00 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (45.66 KB, patch)
2020-04-06 22:03 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gérard Talbot 2019-02-28 12:17:42 PST
CSS4 Values and Units, section 6.1.1 Font-relative lengths: the lh, rlh units
https://www.w3.org/TR/css-values-4/#font-relative-lengths
states

"
lh unit
    Equal to the computed value of the line-height property of the element on which it is used, converting normal to an absolute length by using only the metrics of the first available font. 

rlh unit
    Equal to the computed value of line-height property on the root element, converting normal to an absolute length as above.
"

Tests
-----

http://w3c-test.org/css/css-values/lh-rlh-on-root-001.html

http://w3c-test.org/css/css-values/lh-unit-001.html

http://w3c-test.org/css/css-values/lh-unit-002.html


Test results
- - - - - -

https://wpt.fyi/results/css/css-values?label=master

So far, no browser pass these 3 tests.


Correspondent Bugzilla Mozilla bug report:
https://bugzilla.mozilla.org/show_bug.cgi?id=1310170
Comment 1 Radar WebKit Bug Importer 2019-03-01 21:02:06 PST
<rdar://problem/48532318>
Comment 2 Tyler Wilcock 2020-02-15 15:37:54 PST
Hi!  Looks like there is a duplicate of this -- https://bugs.webkit.org/show_bug.cgi?id=204277.
Comment 3 Simon Fraser (smfr) 2020-02-17 10:34:22 PST
*** Bug 204277 has been marked as a duplicate of this bug. ***
Comment 4 Tyler Wilcock 2020-04-04 15:14:36 PDT
Created attachment 395461 [details]
Patch
Comment 5 Tyler Wilcock 2020-04-04 15:18:09 PDT
Hello!  This is my first patch -- hope I've done everything correct.  I look forward to your review.
Comment 6 Tyler Wilcock 2020-04-04 15:42:46 PDT
Created attachment 395462 [details]
Patch
Comment 7 Tyler Wilcock 2020-04-04 20:29:10 PDT
Created attachment 395473 [details]
Patch
Comment 8 Tyler Wilcock 2020-04-04 21:44:26 PDT
Created attachment 395479 [details]
Patch
Comment 9 Tyler Wilcock 2020-04-04 23:00:52 PDT
Created attachment 395487 [details]
Patch
Comment 10 Tyler Wilcock 2020-04-05 10:12:12 PDT
Sorry for the churn — should be all set for review now.
Comment 11 Antti Koivisto 2020-04-06 10:41:13 PDT
Comment on attachment 395487 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395487&action=review

Looks good!

> Source/WebCore/css/CSSToLengthConversionData.h:44
> +enum class ComputingProperty {
> +    FontSize,
> +    LineHeight,
> +    OtherOrUnknown
> +};

An alternative approach would be to use Optional<CSSPropertyID> instead of this (passing CSSPropertyFontSize/CSSPropertyLineHeight where appropriate).

If you keep the enum it should move to CSSToLengthConversionData namespace. PropertyToCompute or just PropertyType might read better.

> Source/WebCore/css/CSSToLengthConversionData.h:48
> +    CSSToLengthConversionData(const RenderStyle* style, const RenderStyle* rootStyle, const RenderStyle* parentStyle, const RenderView* renderView, float zoom, ComputingProperty computingProperty = ComputingProperty::OtherOrUnknown)

'computingProperty' is an awkward name for variables and fields too
Comment 12 Tyler Wilcock 2020-04-06 22:03:07 PDT
Created attachment 395653 [details]
Patch
Comment 13 Tyler Wilcock 2020-04-06 23:14:19 PDT
Thanks for the suggestions, I've applied them in this new patch.
Comment 14 Antti Koivisto 2020-04-07 21:36:00 PDT
Thanks for the patch!
Comment 15 EWS 2020-04-07 21:45:50 PDT
Committed r259703: <https://trac.webkit.org/changeset/259703>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395653 [details].
Comment 16 Simon Fraser (smfr) 2020-06-15 14:43:26 PDT
This was moved behind an off-by-default flag:
https://bugs.webkit.org/show_bug.cgi?id=211356