Implement CSS property `line-height-step`

RESOLVED WONTFIX

Status

()

Core
Layout: Block and Inline
P3
normal
RESOLVED WONTFIX
a year ago
7 months ago

People

(Reporter: Tommy Kuo (away forever...), Assigned: Tommy Kuo (away forever...))

Tracking

(Blocks: 1 bug, {dev-doc-needed, DevAdvocacy})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DevRel:P2], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(10 attachments)

(Assignee)

Description

a year ago
This bug is for implementing `line-height-step` property.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 4

a year ago
Created attachment 8843887 [details]
testcase.html
(Assignee)

Comment 5

a year ago
Created attachment 8843889 [details]
result screenshot comparison
(Assignee)

Comment 6

a year ago
Currently, this patch can adjust the height of line boxes to multiples of the specified length. Please see the testcase and the attached screenshot.

And I also run the tests provided from the spec[1]. And the test result is here[2]. There are still some issues with ruby and vertical-align.

[1]: https://drafts.csswg.org/css-rhythm/
[2]: https://test.csswg.org/harness/results/css-rhythm-1_dev/grouped/

Comment 7

a year ago
Great, and thank you for testing the test in CSS WG.

FYI, Blink patch to align to the latest spec is in progress:
https://codereview.chromium.org/2704343003
The layout code is in RootInlineBox.cpp. The code looks almost identical, but from the test results, it looks like Gecko and Blink handles vertical-align somewhat differently?

I guess ruby is the same issue as vertical-align; so you need to extend the line height without changing the text-top position. I don't know Gecko code base, so apologies that I can't advise the code.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

a year ago
Created attachment 8851027 [details]
Screenshot of failed test case.png
(Assignee)

Comment 12

a year ago
After some investigation, I found the test case failure of vertical-align is caused by actual value of `line-height: normal`. In Gecko, `normal` value is 1.2[1]. But it's 1 in Blink. I add `line-height: 1` to the test case, the result is expected.

[1]: https://developer.mozilla.org/en/docs/Web/CSS/line-height
(Assignee)

Comment 13

a year ago
Created attachment 8852410 [details]
the screenshot of adding `line-height: 1` to <div class="test">
(Assignee)

Comment 14

a year ago
The test case of vertical-align would be fixed after this commit[1] is merged.

[1]: https://github.com/w3c/web-platform-tests/pull/5251
(Assignee)

Comment 15

a year ago
Created attachment 8852788 [details]
line-height-step-with-ruby-failed.png

Comment 16

a year ago
line-height-step-ruby-001.html should also be rendered with Ahem, can you check if your machine has Ahem installed?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 21

a year ago
Pass all test cases on spec[1], except 'line-height-step-valign-001' (please see comment 14).

[1]: https://www.w3.org/TR/css-rhythm-1/(In reply to Koji Ishii from comment #16)
(Assignee)

Comment 22

a year ago
Fix the reference link in comment 21.

[1]: https://www.w3.org/TR/css-rhythm-1/
(Assignee)

Updated

a year ago
Attachment #8852832 - Flags: review?(xidorn+moz)
Attachment #8843876 - Flags: review?(xidorn+moz)
Attachment #8843877 - Flags: review?(xidorn+moz)
Attachment #8843878 - Flags: review?(xidorn+moz)
(Assignee)

Comment 23

a year ago
(In reply to Koji Ishii from comment #16)
> line-height-step-ruby-001.html should also be rendered with Ahem, can you
> check if your machine has Ahem installed?

I found there is really a bug with ruby frame, and I fixed it now. Thank you! :)
(Assignee)

Comment 24

a year ago
mozreview-review
Comment on attachment 8843878 [details]
Bug 1343819 - (Part 4) Adjust the height of line box.

https://reviewboard.mozilla.org/r/117506/#review127598

::: layout/generic/nsLineLayout.cpp:1546
(Diff revision 3)
>    // participated in #1; for example: "top" and "botttom" aligned frames)
>    //
>    // [3] the minimum line height ("line-height" property set on the
>    // block frame)
> +  // 
> +  // [4] "line-height-step"

Oh, I found I forgot to finish the comment here. I'll leave the comment later.
Please add some tests. If there are already some in CSSWG's test repo, you can import them via layout/reftests/w3c-css/import-tests.py.
Comment hidden (mozreview-request)
Also please send an "Intent to implement" to dev-platform. (I thought you've sent one, but failed to find. Probably I was seeing some other feature.)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 33

a year ago
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #25)
> Please add some tests. If there are already some in CSSWG's test repo, you
> can import them via layout/reftests/w3c-css/import-tests.py.

Done!

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #27)
> Also please send an "Intent to implement" to dev-platform. (I thought you've
> sent one, but failed to find. Probably I was seeing some other feature.)

Done!

Comment 34

a year ago
mozreview-review
Comment on attachment 8852832 [details]
Bug 1343819 - (Part 1) Add line-height-step to property_database.js.

https://reviewboard.mozilla.org/r/124992/#review128000

::: layout/style/test/property_database.js:3184
(Diff revision 2)
> +  "line-height-step": {
> +    domProp: "lineHeightStep",
> +    inherited: true,
> +    type: CSS_TYPE_LONGHAND,
> +    initial_values: [ "none" ],
> +    other_values: [ "20px", "none", "40pt", "calc(30px)", "calc(3*25px)", "1em" ],
> +    invalid_values: [ "100%", "-10px", "calc(1 + 2px)", "calc(100% + 0.1)" ]
> +  },

You need to put this behind prefs. There are several near the end of the file.

::: layout/style/test/property_database.js:3188
(Diff revision 2)
> +    initial_values: [ "none" ],
> +    other_values: [ "20px", "none", "40pt", "calc(30px)", "calc(3*25px)", "1em" ],

`none` is initial value, so it is not "other value". Also please add `0` and `0px` to initial value.

I wonder whether we really need `none`. Probably the spec should just remove it.
Attachment #8852832 - Flags: review?(xidorn+moz)

Comment 35

a year ago
> I wonder whether we really need `none`. Probably the spec should just remove it.

Glad you found it. fantasai said it's necessary:
https://github.com/w3c/csswg-drafts/issues/938#issuecomment-275175638

but I don't understand why it's needed. It's marked as an issue in the spec:
> none and 0 are equivalent. Is this ok?

I personally think it's possible that it may be gone as the spec evolve. Since 'none' computes to 0, adding 'none' later is easy.

Comment 36

a year ago
mozreview-review
Comment on attachment 8843877 [details]
Bug 1343819 - (Part 3) Get computed value of line-height-step.

https://reviewboard.mozilla.org/r/117504/#review128012

This can probably be merged with part 2.
Attachment #8843877 - Flags: review?(xidorn+moz) → review+

Comment 37

a year ago
mozreview-review
Comment on attachment 8843876 [details]
Bug 1343819 - (Part 2) Add line-height-step prop in style system.

https://reviewboard.mozilla.org/r/117502/#review128002

::: dom/ipc/ContentPrefs.cpp
(Diff revision 4)
>  const char*  mozilla::dom::ContentPrefs::GetContentPref(size_t aIndex)
>  {
>    MOZ_ASSERT(aIndex < ArrayLength(ContentPrefs::gInitPrefs));
>    return gInitPrefs[aIndex];
>  }
> -

Please avoid include unrelated files.

::: layout/style/nsCSSPropList.h:2479
(Diff revision 4)
> +    LineHeightStep,
> +    CSS_PROPERTY_PARSE_VALUE |
> +        CSS_PROPERTY_VALUE_NONNEGATIVE |
> +        CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE |
> +        CSS_PROPERTY_APPLIES_TO_PLACEHOLDER |
> +        CSS_PROPERTY_GETCS_NEEDS_LAYOUT_FLUSH,

It doesn't seem to me getting the resolved value of this property needs layout flush. Please remove this flag. line-height needs that because an internal keyword value for form control relies on the height of its container, but this prooerty doesn't need such thing.

::: layout/style/nsCSSPropList.h:2480
(Diff revision 4)
> +    CSS_PROPERTY_PARSE_VALUE |
> +        CSS_PROPERTY_VALUE_NONNEGATIVE |
> +        CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE |
> +        CSS_PROPERTY_APPLIES_TO_PLACEHOLDER |
> +        CSS_PROPERTY_GETCS_NEEDS_LAYOUT_FLUSH,
> +    "layout.css.line-height-step.enabled",

This pref should be listed in modules/libpref/init/all.js.

::: layout/style/nsRuleNode.cpp:4801
(Diff revision 4)
> +  } else if (eCSSUnit_None == lineHeightStepValue->GetUnit() ||
> +             eCSSUnit_Initial == lineHeightStepValue->GetUnit()) {
> +    text->mLineHeightStep.SetCoordValue(0);
> +  } else if (eCSSUnit_Inherit == lineHeightStepValue->GetUnit() ||
> +             eCSSUnit_Unset == lineHeightStepValue->GetUnit()) {
> +    text->mLineHeightStep = parentText->mLineHeightStep;

You need `SetUncachable();` here, because it inherits.

::: layout/style/nsRuleNode.cpp:4809
(Diff revision 4)
> +    LengthNumberCalcObj obj = css::ComputeCalc(*lineHeightStepValue, ops);
> +    text->mLineHeightStep.SetCoordValue(NSToCoordRoundWithClamp(obj.mValue));
> +  } else if (lineHeightStepValue->IsLengthUnit()) {
> +    nscoord len = CalcLength(*lineHeightStepValue, aContext, mPresContext,
> +                             conditions);
> +    conditions.SetUncacheable();

I need to investigate a bit whether we need set uncacheable here.

::: layout/style/nsStyleStruct.h:1944
(Diff revision 4)
>  
>    nsStyleCoord mTabSize;                // [inherited] coord, factor, calc
>    nsStyleCoord mWordSpacing;            // [inherited] coord, percent, calc
>    nsStyleCoord mLetterSpacing;          // [inherited] coord, normal
>    nsStyleCoord mLineHeight;             // [inherited] coord, factor, normal
> +  nsStyleCoord mLineHeightStep;         // [inherited] none, coord, calc

It seems this property doesn't need anything other than absolute length for computed value (no number, percentage, etc). You should just store it as nscoord.
Attachment #8843876 - Flags: review?(xidorn+moz)

Comment 38

a year ago
mozreview-review
Comment on attachment 8843877 [details]
Bug 1343819 - (Part 3) Get computed value of line-height-step.

https://reviewboard.mozilla.org/r/117504/#review128022

::: layout/style/nsComputedDOMStyle.cpp:3823
(Diff revision 4)
>  
>  already_AddRefed<CSSValue>
> +nsComputedDOMStyle::DoGetLineHeightStep()
> +{
> +  RefPtr<nsROCSSPrimitiveValue> val = new nsROCSSPrimitiveValue;
> +  SetValueToCoord(val, StyleText()->mLineHeightStep, false);

Actually making the property stored as nscoord means you need to fix code here as well.

Comment 39

a year ago
mozreview-review
Comment on attachment 8843876 [details]
Bug 1343819 - (Part 2) Add line-height-step prop in style system.

https://reviewboard.mozilla.org/r/117502/#review128024

::: layout/style/nsRuleNode.cpp:4810
(Diff revision 4)
> +    text->mLineHeightStep.SetCoordValue(NSToCoordRoundWithClamp(obj.mValue));
> +  } else if (lineHeightStepValue->IsLengthUnit()) {
> +    nscoord len = CalcLength(*lineHeightStepValue, aContext, mPresContext,
> +                             conditions);
> +    conditions.SetUncacheable();
> +    text->mLineHeightStep.SetCoordValue(len);

You need to clamp the result value to nonnegative, because calc() can bring negative value here.
Keywords: DevAdvocacy
Whiteboard: [DevRel:P2]
Comment on attachment 8843876 [details]
Bug 1343819 - (Part 2) Add line-height-step prop in style system.

https://reviewboard.mozilla.org/r/117502/#review128046

One more comment in addition to Xidorn's:

::: layout/style/nsRuleNode.cpp:4792
(Diff revision 4)
> +  // line-height-step: none, length, calc
> +  const nsCSSValue* lineHeightStepValue = aRuleData->ValueForLineHeightStep();
> +  if (eCSSUnit_Null == lineHeightStepValue->GetUnit()) {
> +    // do nothing
> +  } else if (eCSSUnit_None == lineHeightStepValue->GetUnit() ||
> +             eCSSUnit_Initial == lineHeightStepValue->GetUnit()) {
> +    text->mLineHeightStep.SetCoordValue(0);
> +  } else if (eCSSUnit_Inherit == lineHeightStepValue->GetUnit() ||
> +             eCSSUnit_Unset == lineHeightStepValue->GetUnit()) {
> +    text->mLineHeightStep = parentText->mLineHeightStep;
> +  } else if (eCSSUnit_Calc == lineHeightStepValue->GetUnit()) {
> +    SetLineHeightCalcOps ops(aContext, mPresContext, conditions);
> +    LengthNumberCalcObj obj = css::ComputeCalc(*lineHeightStepValue, ops);
> +    text->mLineHeightStep.SetCoordValue(NSToCoordRoundWithClamp(obj.mValue));
> +  } else if (lineHeightStepValue->IsLengthUnit()) {
> +    nscoord len = CalcLength(*lineHeightStepValue, aContext, mPresContext,
> +                             conditions);
> +    conditions.SetUncacheable();
> +    text->mLineHeightStep.SetCoordValue(len);
> +  } else {
> +    MOZ_ASSERT_UNREACHABLE("unexpected unit");
> +  }

Much of this should be done by calling SetCoord rather than writing it out yourself.  (In fact, if you were to add a SETCOORD_NONE_ZERO I think you could do it all in SetCoord -- although it appears there's some question as to why 'none' exists at all.)

Note that you'd need SETCOORD_CALC_LENGTH_ONLY and SETCOORD_CALC_CLAMP_NONNEGATIVE.
Comment on attachment 8843876 [details]
Bug 1343819 - (Part 2) Add line-height-step prop in style system.

https://reviewboard.mozilla.org/r/117502/#review128054

::: layout/style/nsCSSPropList.h:2477
(Diff revision 4)
> +        CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE |
> +        CSS_PROPERTY_APPLIES_TO_PLACEHOLDER |

One more comment is that I think these two flags are wrong.  I think neither should be present.

(Remember that while 'line-height' applies to blocks and inlines, 'line-height-step' only applies to blocks.)
Comment on attachment 8843878 [details]
Bug 1343819 - (Part 4) Adjust the height of line box.

https://reviewboard.mozilla.org/r/117506/#review128058

::: layout/generic/nsLineLayout.cpp:1572
(Diff revision 5)
>    // Navigator 4 gives precedence to the first top/bottom aligned
>    // object.  We just let block end aligned objects win.
>    if (lineBSize < mMaxEndBoxBSize) {
>      // When the line is shorter than the maximum block start aligned box
>      nscoord extra = mMaxEndBoxBSize - lineBSize;
>      baselineBCoord += extra;
>      lineBSize = mMaxEndBoxBSize;
>    }
>    if (lineBSize < mMaxStartBoxBSize) {
>      lineBSize = mMaxStartBoxBSize;
>    }

I think this code may also need adjustment for line-height-step.  (It also probably needs to be adjusted to match a newer agreement on what the spec should be, which I *think* the group made at some point.)

In particular, I think the CSS WG did agree to a resolution on how to resolve the indeterminate cases in https://lists.w3.org/Archives/Public/www-style/1999Mar/0121.html .  (We definitely addresed the unsolvable cases much earlier by introducing the  "aligned subtree" concept in https://drafts.csswg.org/css2/visudet.html#propdef-vertical-align .)
Keywords: dev-doc-needed

Comment 44

a year ago
mozreview-review
Comment on attachment 8843878 [details]
Bug 1343819 - (Part 4) Adjust the height of line box.

https://reviewboard.mozilla.org/r/117506/#review128052

::: layout/generic/nsLineLayout.cpp:2511
(Diff revision 5)
> +  const nsStyleCoord& lhsCoord =
> +    psd->mFrame->mFrame->StyleContext()->StyleText()->mLineHeightStep;

You don't need `mFrame->StyleContext()->StyleText()`. `mFrame->StyleText()` should just work.

Actually, `line-height-step` applies to the block container, so you should use the value from `mStyleText` rather than `psd->mFrame`.

This also reminds me that, the spec should probably reset `line-height-step` to its initial value for `ruby`, `rt`, and `rtc`.

::: layout/generic/nsLineLayout.cpp:2525
(Diff revision 5)
> +  nscoord extra = mod ? lineHeightStep - mod : 0;
> +
> +  // get max start/end leading affected by emphasis or ruby frame
> +  nscoord maxStartLeading = 0;
> +  nscoord maxEndLeading = 0;
> +  for (auto pfd = psd->mFirstFrame; pfd; pfd = pfd->mNext) {

Please write down the actual type here rather than using `auto`.

::: layout/generic/nsLineLayout.cpp:2557
(Diff revision 5)
> +  // emphasis. So, the real extra space corresponding to the text should be the
> +  // sum of the extra space we calculated above, the height of the top ruby
> +  // frame (emphasis) and the height of the end ruby frame (emphasis).
> +  //
> +  // And, the baseline offset would be the following calculation.
> +  nscoord baselineOffset = (extra + maxStartLeading + maxEndLeading) / 2 - maxStartLeading;

This doesn't seem right to me. For example, if you only have ruby on the end side, say, start leading = 0, end leading = 100, and extra = 80, you would get baseline offset = 90, which pushes the baseline down even more than the total extra space, and potentially pushes the ruby to somewhere conflicts with line below.

Also if start leading = 100, end leading = 0, extra = 0, you would get baseline offset = -50... which doesn't make sense at all.

Based on your figure, I guess the calculation should probably be something like `extra / 2 - maxStartLeading`. You still need to handle negative baseline offset somehow. But existence of negative offset really makes it suspicious. I guess it's worth further thinking about how line-height-step should interact with ruby and emphasis marks. I don't have a clear mental model about this for now.

Probably the spec should show some examples for these cases.
Attachment #8843878 - Flags: review?(xidorn+moz) → review-

Comment 45

a year ago
mozreview-review
Comment on attachment 8853249 [details]
Bug 1343819 - (Part 5) Add reftests from CSSWG.

https://reviewboard.mozilla.org/r/125308/#review128758

You probably need to add your pref to layout/reftests/w3c-css/failures.list for these tests.
Attachment #8853249 - Flags: review?(xidorn+moz) → review+

Comment 46

a year ago
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #44)
> This also reminds me that, the spec should probably reset `line-height-step`
> to its initial value for `ruby`, `rt`, and `rtc`.

Jet gave the feedback on ruby when we met, but as far as I checked the spec after that, the ruby spec does not define to create line boxes for rt/rtc, and line-height-step applies to line boxes, not inline boxes, so it looked unnecessary to me.

On second thought now, does Gecko create line boxes for rt/rtc? Since the ruby spec does not prohibit to create line boxes, maybe we could say "if impl chose to create line boxes for rt/rtc, ..."?

Resetting for ruby looks unnecessary, or undesired (unlikely but) when the whole line is ruby, no?

Comment 47

a year ago
Blink is considering shipping this API: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/z0RI8seNuCs.  If folks here have any concerns over the stability of the API / spec (specific things which may cause non-trivial web-compat issues if we decide to change, please let us know, eg. on that thread).  If approved we'd have until at least June to make breaking changes or disable the feature before it hits Chrome stable.
(Assignee)

Comment 48

a year ago
Because there are still some concerns for the spec[1]. We decide to not ship this API for now. And Blink also decides to delay shipping this API[2].

[1]: https://groups.google.com/forum/#!searchin/mozilla.dev.platform/line-height-step%7Csort:relevance/mozilla.dev.platform/cnEN_sccXJY/-0W_P31cCwAJ
[2]: https://codereview.chromium.org/2833323002/

Updated

10 months ago
Priority: -- → P3

Updated

7 months ago
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → WONTFIX
Jet, could you add more context in terms of this resolution? Thanks.
Flags: needinfo?(bugs)

Comment 50

7 months ago
(In reply to Astley Chen [:astley] (UTC-6) from comment #49)
> Jet, could you add more context in terms of this resolution? Thanks.

Serious issues with this proposal have not been addressed despite multiple requests for resolution:
https://groups.google.com/d/msg/mozilla.dev.platform/cnEN_sccXJY/1ddo5XifAwAJ

Also, we have not seen sufficient author interest to justify shipping with those issues. Marking this bug WONTFIX clarifies our position on this proposal.
Flags: needinfo?(bugs)
You need to log in before you can comment on or make changes to this bug.