Closed Bug 1343819 Opened 7 years ago Closed 6 years ago

Implement CSS property `line-height-step`

Categories

(Core :: Layout: Block and Inline, enhancement, P3)

Product:
Core
Component:
Layout: Block and Inline
Type:
enhancement

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: kuoe0.tw, Assigned: kuoe0.tw)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: dev-doc-needed, DevAdvocacy, Whiteboard: [DevRel:P2])

Attachments

(10 files)

Bug 1343819 - (Part 2) Add line-height-step prop in style system.
59 bytes, text/x-review-board-request
Details
Bug 1343819 - (Part 3) Get computed value of line-height-step.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1343819 - (Part 4) Adjust the height of line box.
59 bytes, text/x-review-board-request
xidorn
: review-
Details
testcase.html
2.67 KB, text/html
Details
result screenshot comparison
1.08 MB, image/jpeg
Details
Screenshot of failed test case.png
206.94 KB, image/png
Details
the screenshot of adding `line-height: 1` to <div class="test">
103.54 KB, image/png
Details
line-height-step-with-ruby-failed.png
114.25 KB, image/png
Details
Bug 1343819 - (Part 1) Add line-height-step to property_database.js.
59 bytes, text/x-review-board-request
Details
Bug 1343819 - (Part 5) Add reftests from CSSWG.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
This bug is for implementing `line-height-step` property.
Attached file testcase.html — 

    
    

    
  
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/

    
    

    
  
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.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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

    
    

    
  
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

    
    

    
  
line-height-step-ruby-001.html should also be rendered with Ahem, can you check if your machine has Ahem installed?

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
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)

    
    

    
  
Fix the reference link in comment 21.

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

    
  

        Attachment #8852832 -
        Flags: review?(xidorn+moz)

        Attachment #8843876 -
        Flags: review?(xidorn+moz)

        Attachment #8843877 -
        Flags: review?(xidorn+moz)

        Attachment #8843878 -
        Flags: review?(xidorn+moz)

    
    

    
  
(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! :)

    
    

    
  
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.

    
    

    
      
  

    
    

    
  
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.)

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(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 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)

    
    

    
  
> 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 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 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 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 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 .)

    
    

    
  
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 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+

    
    

    
  
(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?

    
    

    
  
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.

    
    

    
  
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/
Priority: -- → P3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX

    
    

    
  
Jet, could you add more context in terms of this resolution? Thanks.
Flags: needinfo?(bugs)

    
    

    
  
(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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: