tab-size should accept <length> values

RESOLVED FIXED in Firefox 53

Status

()

Core
CSS Parsing and Computation
--
enhancement
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: sjw, Assigned: Thomas Wisniewski)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

unspecified
mozilla53
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(URL)

Attachments

(3 attachments, 9 obsolete attachments)

14.64 KB, patch
Details | Diff | Splinter Review
8.43 KB, patch
Details | Diff | Splinter Review
6.66 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
-moz-tab-size does not accept <length> values yet.
(Reporter)

Updated

4 years ago
This doesn't look that hard to me, I'll put it on my list as a background task...
Assignee: nobody → matspal
Severity: normal → enhancement
Created attachment 8340692 [details] [diff] [review]
Style system support for tab-size:<length>
Attachment #8340692 - Flags: review?(cam)
Created attachment 8340693 [details] [diff] [review]
Layout for tab-size:<length>
Attachment #8340693 - Flags: review?(cam)
Comment on attachment 8340692 [details] [diff] [review]
Style system support for tab-size:<length>

Review of attachment 8340692 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: layout/style/nsCSSPropList.h
@@ +347,5 @@
>      CSS_PROP_DOMPROP_PREFIXED(TabSize),
>      CSS_PROPERTY_PARSE_VALUE |
>          CSS_PROPERTY_VALUE_NONNEGATIVE,
>      "",
> +    VARIANT_HLICALC,

This is the first property we'd allow calc() in that takes an integer.  Our calc() handling code doesn't support calculating anything other than lengths and percentages at the moment.  We currently also have 'line-height' which takes a number, and should also take a calc() but doesn't, presumably because of the lack of support for calc() on numbers.  Bug 594933 is that broader issue.  Maybe we don't need to solve that in this bug, but I think we should before unprefixing it in bug 737785.

::: layout/style/nsComputedDOMStyle.cpp
@@ +3002,5 @@
>  CSSValue*
>  nsComputedDOMStyle::DoGetTabSize()
>  {
>    nsROCSSPrimitiveValue* val = new nsROCSSPrimitiveValue;
> +  SetValueToCoord(val, StyleText()->mTabSize, true);

(I guess it doesn't matter whether we pass in true or false for aClampNegativeCalc, since we're not using VARIANT_STORES_CALC when parsing it anyway...)

::: layout/style/nsRuleNode.cpp
@@ +3904,5 @@
> +  if (tabSizeValue->GetUnit() == eCSSUnit_Initial) {
> +    text->mTabSize.SetIntValue(NS_STYLE_TABSIZE_INITIAL, eStyleUnit_Integer);
> +  } else {
> +    SetCoord(*tabSizeValue, text->mTabSize, parentText->mTabSize,
> +             SETCOORD_LHI | SETCOORD_CALC_LENGTH_ONLY | SETCOORD_UNSET_INHERIT,

I think we should be using SETCOORD_CLAMP_NONNEGATIVE here, so that the computed value of the property when you write |tab-size: calc(1px - 2px)| is 0 rather than -1px.  I think then you can assert in ComputeTabWidthAppUnits that the value is non-negative, rather than doing the clamping, as we're using CSS_PROPERTY_VALUE_NONNEGATIVE in the property definition macro.

::: layout/style/nsStyleStruct.h
@@ +1321,5 @@
>    uint8_t mHyphens;                     // [inherited] see nsStyleConsts.h
>    uint8_t mTextSizeAdjust;              // [inherited] see nsStyleConsts.h
>    uint8_t mTextOrientation;             // [inherited] see nsStyleConsts.h
>    uint8_t mTextCombineHorizontal;       // [inherited] see nsStyleConsts.h
> +  nsStyleCoord mTabSize;                // [inherited] coord, integer

Move this down next to the other nsStyleCoords for better alignment.

::: layout/style/test/property_database.js
@@ +1070,5 @@
>  		domProp: "MozTabSize",
>  		inherited: true,
>  		type: CSS_TYPE_LONGHAND,
>  		initial_values: [ "8" ],
> +		other_values: [ "0", "3", "99", "12000", "0px", "1em", "calc(1px + 1em)" ],

Add a value like "calc(1px - 2px)" to test that negative values are allowed in the calc().
Attachment #8340692 - Flags: review?(cam) → review+
Comment on attachment 8340693 [details] [diff] [review]
Layout for tab-size:<length>

Review of attachment 8340693 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsTextFrame.cpp
@@ +2932,5 @@
> +  } else {
> +    nscoord w = nsRuleNode::ComputeCoordPercentCalc(textStyle->mTabSize, 0);
> +    if (w >= 0) {
> +      return w;
> +    }

As mentioned in the comment on the previous patch, you can assert w is non-negative here.

@@ +3042,4 @@
>    // Now add tab spacing, if there is any
>    if (!aIgnoreTabs) {
> +    gfxFloat tabWidth = ComputeTabWidthAppUnits(mFrame, mTextRun);
> +    if (tabWidth > 0) {

Does not calling ApplySpacing here when tabWidth == 0 result in each tab character being given no advance?  I can't see anything in the spec that says how to handle tab-width:0.  Is this a spec hole?
In the reftest, can you test tab-size:0px and tab-size:calc(1px - 2px) too, once it's clear what the correct behaviour should be?
(Reporter)

Updated

4 years ago
Blocks: 913153
Comment on attachment 8340693 [details] [diff] [review]
Layout for tab-size:<length>

Cancelling review awaiting a response to the question in comment 6.
Attachment #8340693 - Flags: review?(cam)
(In reply to Cameron McCormack (:heycam) from comment #6)
> Does not calling ApplySpacing here when tabWidth == 0 result in each tab
> character being given no advance?  I can't see anything in the spec that
> says how to handle tab-width:0.  Is this a spec hole?

Spec: "Integers represent the measure as multiples of the space character's advance width (U+0020)."

0 * space character's advance width = 0

Seems pretty straightforward to me: a value of 0 means no advance.

(In reply to Cameron McCormack (:heycam) from comment #5)
> Add a value like "calc(1px - 2px)" to test that negative values are allowed
> in the calc().

Spec: "Negative values are not allowed."

I wouldn't expect calc() to change the fact that negative values are not allowed.

(In reply to Cameron McCormack (:heycam) from comment #7)
> In the reftest, can you test tab-size:0px and tab-size:calc(1px - 2px) too,
> once it's clear what the correct behaviour should be?

I suppose this is a context where '0' and '0px' are not strictly the same, but it would seem to me that they would have the same effect. Specifying an integer means the width of the tab is the length resulting from that integer multiplied by the advance width of the space character; specifying a length means the width of the tab character is that length directly.

Bottom line: both '0' and '0px' mean the tab has no width, and a negative value means an invalid declaration.

At least, that's how I interpret things.
(In reply to Gordon P. Hemsley [:GPHemsley] from comment #9)
> Spec: "Integers represent the measure as multiples of the space character's
> advance width (U+0020)."
> 
> 0 * space character's advance width = 0
> 
> Seems pretty straightforward to me: a value of 0 means no advance.

I think that not advancing makes sense, but I don't know that that sentence from the spec above makes it straightforward.  tab-size:0px could mean that there are an infinite number of tab stops in the line.  What does it mean to advance to the next tab stop from the current position?  Perhaps it should advance to (current_position + epsilon)?

> Spec: "Negative values are not allowed."
> 
> I wouldn't expect calc() to change the fact that negative values are not
> allowed.

It does change whether the value is a parse error or not.  For example:

  p { tab-size: 8px; tab-size: -4px; }

should result in a tab-size of 8px, while

  p { tab-size: 8px; tab-size: calc(2px - 4px); }

should result in a tab-size of 0px, as the second declaration will be parsed but will clamp its result.  As far as I understand how plain lengths and calc values work:

  http://dev.w3.org/csswg/css-values/#numeric-types
  http://dev.w3.org/csswg/css-values/#calc-range
(In reply to Cameron McCormack (:heycam) from comment #10)
> (In reply to Gordon P. Hemsley [:GPHemsley] from comment #9)
> > Spec: "Integers represent the measure as multiples of the space character's
> > advance width (U+0020)."
> > 
> > 0 * space character's advance width = 0
> > 
> > Seems pretty straightforward to me: a value of 0 means no advance.
> 
> I think that not advancing makes sense, but I don't know that that sentence
> from the spec above makes it straightforward.  tab-size:0px could mean that
> there are an infinite number of tab stops in the line.  What does it mean to
> advance to the next tab stop from the current position?  Perhaps it should
> advance to (current_position + epsilon)?

I admit I don't know anything about the underlying implementation, but why is the number of tab stops important? (Why are tab stops even in play?) You'll always have a finite number of tab characters in the content being styled, and it's just a question of how wide to display each one of them, isn't it?

> > Spec: "Negative values are not allowed."
> > 
> > I wouldn't expect calc() to change the fact that negative values are not
> > allowed.
> 
> It does change whether the value is a parse error or not.  For example:
> 
>   p { tab-size: 8px; tab-size: -4px; }
> 
> should result in a tab-size of 8px, while
> 
>   p { tab-size: 8px; tab-size: calc(2px - 4px); }
> 
> should result in a tab-size of 0px, as the second declaration will be parsed
> but will clamp its result.  As far as I understand how plain lengths and
> calc values work:
> 
>   http://dev.w3.org/csswg/css-values/#numeric-types
>   http://dev.w3.org/csswg/css-values/#calc-range

Ah, I wasn't aware that calc() had that restriction. Carry on. :)
(In reply to Gordon P. Hemsley [:GPHemsley] from comment #11)
> (In reply to Cameron McCormack (:heycam) from comment #10)
> I admit I don't know anything about the underlying implementation, but why
> is the number of tab stops important? (Why are tab stops even in play?)
> You'll always have a finite number of tab characters in the content being
> styled, and it's just a question of how wide to display each one of them,
> isn't it?

The number of tab stops isn't important, no, but the spec says that:

  Tab stops occur at points that are multiples of the tab size from the block's starting content edge.

and when you encounter a tab character:

  Each tab is rendered as a horizontal shift that lines up the start edge of the next glyph with the
  next tab stop.

  http://www.w3.org/TR/css3-text/#white-space-phase-2

I don't think it's well defined what this all means when tab-size is 0.
(In reply to Cameron McCormack (:heycam) from comment #12)
> (In reply to Gordon P. Hemsley [:GPHemsley] from comment #11)
> > (In reply to Cameron McCormack (:heycam) from comment #10)
> > I admit I don't know anything about the underlying implementation, but why
> > is the number of tab stops important? (Why are tab stops even in play?)
> > You'll always have a finite number of tab characters in the content being
> > styled, and it's just a question of how wide to display each one of them,
> > isn't it?
> 
> The number of tab stops isn't important, no, but the spec says that:
> 
>   Tab stops occur at points that are multiples of the tab size from the
> block's starting content edge.
> 
> and when you encounter a tab character:
> 
>   Each tab is rendered as a horizontal shift that lines up the start edge of
> the next glyph with the
>   next tab stop.
> 
>   http://www.w3.org/TR/css3-text/#white-space-phase-2
> 
> I don't think it's well defined what this all means when tab-size is 0.

Ah, well, I suppose you could consider that infinite tab stops. But you could also consider that as always a single tab stop: No matter how many tabs you have in a row, the next 'tab stop' is always the same as the block's starting content edge.

Perhaps the spec could be clarified to make this more explicit, but I don't think there's any ambiguity in the intended behavior.
(Reporter)

Comment 14

4 years ago
Is this patch stable enough for checkin?
Flags: needinfo?(matspal)
(In reply to sjw from comment #14)
> Is this patch stable enough for checkin?

No, I need to address the review comments first.
Flags: needinfo?(matspal)
(Reporter)

Updated

2 years ago
Status: NEW → ASSIGNED

Comment 16

2 years ago
Not support calc() in tab-size.
(Reporter)

Comment 17

2 years ago
See also bug 594933
Duplicate of this bug: 1308174
Note the spec change that tab-size:0px should be supported:
https://github.com/w3c/csswg-drafts/issues/460
Any updates?
Flags: needinfo?(mats)
(Assignee)

Comment 21

a year ago
Created attachment 8803665 [details] [diff] [review]
943918-style_system_support_for_tab_size_length_and_tab_size_number.diff

In the interest in getting this across the finish line, I've rebased the patches and addressed the review comments. Three patches incoming.

I've also gone ahead and switched the patches over to accept <number> instead of <integer>, since the spec has since changed to <number> (see bug 1308108). This didn't change the patches much, save for requiring a couple of additional changes:
- allowing the nsStyleCoord(int, unit) constructor to accept the eStyleUnit_Factor unit as well.
- adding support for nsRuleNode::eStyleUnit_Factor to ComputeCoordPercentCalc.

Try seems fine (just a couple of unrelated intermittents): https://treeherder.mozilla.org/#/jobs?repo=try&revision=960c25b6c26b8d3f3621880477bb5535bebec2ab

As for specific review comments:

>We currently also have 'line-height' which takes a number, and should also take a calc() but doesn't, presumably because of the lack of support for calc() on numbers. Bug 594933 is that broader issue. Maybe we don't need to solve that in this bug, but I think we should before unprefixing it in bug 737785.

Bug 594933 is fixed now, so this doesn't appear to be an issue anymore.

>I think we should be using SETCOORD_CLAMP_NONNEGATIVE here

Since this version of the patchset supports <number> instead of <integer>, I instead handle the calc() case by re-using LineHeightCalcOps and clamping to non-negative manually.

I do wonder if we're actually supposed to clamp to non-negative, given that the spec text says "Computed value: the specified integer or length made absolute" (at https://drafts.csswg.org/css-text-3/#propdef-tab-size ). Should we instead be taking the absolute value? Note that Chrome outright ignores the rules calc(-3) and calc(3-2), while this patchset does not. Chrome also ignores negative <integer> and <length> values (so does this patch), and I'm not sure what's correct here.

>Move this down next to the other nsStyleCoords for better alignment.

Done.

>Add a value like "calc(1px - 2px)" to test that negative values are allowed in the calc().

Done. I also made sure that the <number> case has values tested appropriately.

>you can assert w is non-negative here.

Done.

>I can't see anything in the spec that says how to handle tab-width:0.

As per comment 19, the draft spec has been updated to state that: "If the tab size is zero, tabs are not rendered."  This patchset does that.

>In the reftest, can you test tab-size:0px and tab-size:calc(1px - 2px) too, once it's clear what the correct behaviour should be?

I've adjusted the reftest accordingly, including adding tests for the <number> cases.
Attachment #8340692 - Attachment is obsolete: true
Flags: needinfo?(mats)
Attachment #8803665 - Flags: review?(cam)
(Assignee)

Comment 22

a year ago
Created attachment 8803666 [details] [diff] [review]
943918-layout_for_tab_size_length_and_tab_size_number.diff
Attachment #8340693 - Attachment is obsolete: true
Attachment #8803666 - Flags: review?(cam)
(Assignee)

Comment 23

a year ago
Created attachment 8803667 [details] [diff] [review]
943918-reftest_for_tab_size_length_and_tab_size_number.diff
Attachment #8340694 - Attachment is obsolete: true
Attachment #8803667 - Flags: review?(cam)
Comment on attachment 8803665 [details] [diff] [review]
943918-style_system_support_for_tab_size_length_and_tab_size_number.diff

>diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp
>--- a/layout/style/nsRuleNode.cpp
>+++ b/layout/style/nsRuleNode.cpp
>@@ -777,16 +777,18 @@ nsRuleNode::ComputeComputedCalc(const ns
> 
> /* static */ nscoord
> nsRuleNode::ComputeCoordPercentCalc(const nsStyleCoord& aCoord,
>                                     nscoord aPercentageBasis)
> {
>   switch (aCoord.GetUnit()) {
>     case eStyleUnit_Coord:
>       return aCoord.GetCoordValue();
>+    case eStyleUnit_Factor:
>+      return aCoord.GetFactorValue();

This doesn't seem like a valid assumption in general -- a factor isn't necessarily convertible to an nscoord.  Why do you need this?

> nsStyleCoord::nsStyleCoord(int32_t aValue, nsStyleUnit aUnit)
>   : mUnit(aUnit)
> {
>   //if you want to pass in eStyleUnit_Coord, don't. instead, use the
>   //constructor just above this one... MMP
>   NS_ASSERTION((aUnit == eStyleUnit_Enumerated) ||
>+               (aUnit == eStyleUnit_Factor) ||
>                (aUnit == eStyleUnit_Integer), "not an int value");
>   if ((aUnit == eStyleUnit_Enumerated) ||
>       (aUnit == eStyleUnit_Integer)) {
>     mValue.mInt = aValue;
>   }
>+  else if ((aUnit == eStyleUnit_Factor)) {
>+    mValue.mFloat = (float)aValue;
>+  }
>   else {
>     mUnit = eStyleUnit_Null;
>     mValue.mInt = 0;
>   }
> }

This seems like a bad thing to encourage, since callers with eStyleUnit_Factor will truncate their float to an int32_t to pass it to this constructor.
(Assignee)

Comment 25

a year ago
Thanks, dbaron!

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #24)
> This doesn't seem like a valid assumption in general -- a factor isn't
> necessarily convertible to an nscoord.  Why do you need this?

Actually, I don't think I do need this after all. It's just a hold-over from before I realized how to construct an nsStyleCoord with a Factor unit. I'll remove this.


> This seems like a bad thing to encourage, since callers with
> eStyleUnit_Factor will truncate their float to an int32_t to pass it to this
> constructor.

Yeah, in hindsight I'm inclined to agree. I only did this because NS_STYLE_TABSIZE_INITIAL is #defined as 8, and so the compiler wasn't sure which constructor to use (the int or float version). Would you be against me just calling the constructor like this instead to disambiguate?

>nsStyleCoord(static_cast<float>(NS_STYLE_TABSIZE_INITIAL), eStyleUnit_Factor)
Flags: needinfo?(dbaron)
(In reply to Thomas Wisniewski from comment #25)
> Yeah, in hindsight I'm inclined to agree. I only did this because
> NS_STYLE_TABSIZE_INITIAL is #defined as 8, and so the compiler wasn't sure
> which constructor to use (the int or float version). Would you be against me
> just calling the constructor like this instead to disambiguate?
> 
> >nsStyleCoord(static_cast<float>(NS_STYLE_TABSIZE_INITIAL), eStyleUnit_Factor)

That's fine, except I prefer construction-style casts for integral types, so:

  nsStyleCoord(float(NS_STYLE_TABSIZE_INITIAL), eStyleUnit_Factor)
Flags: needinfo?(dbaron)
(Assignee)

Comment 27

a year ago
Created attachment 8803670 [details] [diff] [review]
943918-style_system_support_for_tab_size_length_and_tab_size_number.diff

Alright, here's a new version of that patch with those issues addressed.
Attachment #8803665 - Attachment is obsolete: true
Attachment #8803665 - Flags: review?(cam)
Attachment #8803670 - Flags: review?(cam)
(Assignee)

Comment 28

a year ago
Friendly review ping; let me know if another reviewer might be better, please and thanks!
Flags: needinfo?(cam)
(In reply to Thomas Wisniewski from comment #21)
> I do wonder if we're actually supposed to clamp to non-negative, given that
> the spec text says "Computed value: the specified integer or length made
> absolute" (at https://drafts.csswg.org/css-text-3/#propdef-tab-size ).
> Should we instead be taking the absolute value? Note that Chrome outright
> ignores the rules calc(-3) and calc(3-2), while this patchset does not.
> Chrome also ignores negative <integer> and <length> values (so does this
> patch), and I'm not sure what's correct here.

I think "absolute" here means as opposed to a relative <length> unit, rather than a non-negative number.

https://drafts.csswg.org/css-values/#calc-range says that calc(-3) should be accepted as the specified value, and clamped later.  But at what later point is still a question.  That section says that clamping must happen at used value time, and that when percentages can be resolved at computed value time that is done.  I think when percentages do resolve at computed value time, that the clamping should also be done at computed value time, and implementations bear this out:

  http://mcc.id.au/temp/calc.html

shows the computed value of |min-width: calc(-1px - 2px)| is 0 and not calc(-3px).

Complicating this is that due to https://drafts.csswg.org/cssom/#resolved-values for some common properties it's not possible to inspect the computed value.  (Although maybe with animation you could?)

That spec section on calc() ranges doesn't talk about other values that can compute (or not) at computed value time, like the number here in tab-size, but I think we should treat it analagously.  So since we don't convert the integer into a length until we actually use it, in nsTextFrame, I think it makes sense for the computed values to look like calc(-1px - 2).

You are right that we shouldn't use SETCOORD_CALC_CLAMP_NONNEGATIVE, since that only affects how SETCOORD_CALC_LENGTH_ONLY behaves, which is not what we're using here.  (It seems there are a few incorrect uses of SETCOORD_CALC_CLAMP_NONNEGATIVE in nsRuleNode.cpp, when we're not also passing in SETCOORD_CALC_LENGTH_ONLY.  I guess we should add an assertion for that.)
Flags: needinfo?(cam)
> So since we don't convert the integer into a length until we
> actually use it, in nsTextFrame, I think it makes sense for the
> computed values to look like calc(-1px - 2).

Actually, calc() expressions that mix numbers and lengths in an additive manner like this aren't even valid, so that simplifies things.
Comment on attachment 8803670 [details] [diff] [review]
943918-style_system_support_for_tab_size_length_and_tab_size_number.diff

Review of attachment 8803670 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay in reviewing.  r=me with the below addressed.

::: layout/style/nsRuleNode.cpp
@@ +4554,5 @@
> +  if (tabSizeValue->GetUnit() == eCSSUnit_Initial) {
> +    text->mTabSize = nsStyleCoord(float(NS_STYLE_TABSIZE_INITIAL), eStyleUnit_Factor);
> +  } else if (eCSSUnit_Calc == tabSizeValue->GetUnit()) {
> +    SetLineHeightCalcOps ops(aContext, mPresContext, conditions);
> +    LineHeightCalcObj obj = css::ComputeCalc(*tabSizeValue, ops);

It feels a bit weird that in SetLineHeightCalcOps it will look for percentages to resolve against the font-size.  Even though we shouldn't get in there, since the property is defined as taking VARIANT_LNCALC.

Maybe we can factor this out a bit, e.g.:

* rename SetLineHeightCalcOps to LengthNumberCalcOps, LineHeightCalcObj to LengthNumberCalcObj, and then LengthNumberCalcObj::mLineHeight to mValue

* have SetLineHeightCalcOps derive from LengthNumberCalcOps, and only provide a ComputeLeafValue function

* remove the percentage-to-font-size bit of LengthNumberCalcOps
Attachment #8803670 - Flags: review?(cam) → review+
Comment on attachment 8803666 [details] [diff] [review]
943918-layout_for_tab_size_length_and_tab_size_number.diff

Review of attachment 8803666 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsTextFrame.cpp
@@ +3351,5 @@
> +{
> +  const nsStyleText* textStyle = aFrame->StyleText();
> +  gfxFloat spaces;
> +  if (textStyle->mTabSize.GetUnit() == eStyleUnit_Factor) {
> +    spaces = textStyle->mTabSize.GetFactorValue();

Assert that spaces is >= 0 too?

@@ +3353,5 @@
> +  gfxFloat spaces;
> +  if (textStyle->mTabSize.GetUnit() == eStyleUnit_Factor) {
> +    spaces = textStyle->mTabSize.GetFactorValue();
> +  } else {
> +    nscoord w = nsRuleNode::ComputeCoordPercentCalc(textStyle->mTabSize, 0);

We don't need to call ComputeCoordPercentCalc, do we?  We should only ever set a factor or a coord, right?

Nit: since we're doing an early return, it might be clearer to put this branch first, then we won't need an else.

@@ +3422,5 @@
> +      CalcTabWidths(aRange, tabWidth);
> +      if (mTabWidths) {
> +        mTabWidths->ApplySpacing(aSpacing,
> +                                 aRange.start - mStart.GetSkippedOffset(),
> +                                 aRange.end - aRange.start);

Why not leave this as aRange.Length()?
Attachment #8803666 - Flags: review?(cam) → review+
Attachment #8803667 - Flags: review?(cam) → review+
(Assignee)

Comment 33

a year ago
>Sorry for the delay in reviewing.

No problemo, thanks for the reviews! I'll try to address the comments and come up final patches tomorrow.
(Assignee)

Comment 34

a year ago
Created attachment 8814413 [details] [diff] [review]
943918-1-style_system_support_for_tab_size_length_and_tab_size_number.diff

Alright, I've updated the patchset and addressed your comments. A fresh try-run still seems ok (despite some new intermittents and  scary-looking-but-unrelated bustage on the valgrind build): https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4475383ff8587b3c62db0938e4cc504b060ac6e

I'll upload the patches (carrying over r+) and request check-in.
Assignee: mats → wisniewskit
Attachment #8803670 - Attachment is obsolete: true
(Assignee)

Comment 35

a year ago
Created attachment 8814414 [details] [diff] [review]
943918-2-layout_for_tab_size_length_and_tab_size_number.diff
Attachment #8803666 - Attachment is obsolete: true
(Assignee)

Comment 36

a year ago
Created attachment 8814415 [details] [diff] [review]
943918-3-reftest_for_tab_size_length_and_tab_size_number.diff
Attachment #8803667 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Keywords: checkin-needed
(Assignee)

Updated

a year ago
Duplicate of this bug: 1308108
(In reply to Cameron McCormack (:heycam) from comment #30)
> > So since we don't convert the integer into a length until we
> > actually use it, in nsTextFrame, I think it makes sense for the
> > computed values to look like calc(-1px - 2).
> 
> Actually, calc() expressions that mix numbers and lengths in an additive
> manner like this aren't even valid, so that simplifies things.

They should become valid in the near future, though.

Comment 39

a year ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a6c538cf61a
Part 1: style system support for tab-size:<length> and tab-size:<number>. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c5eb3fd9fb5
Part 2: layout for tab-size:<length> and tab-size:<number>. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cfdfff9c5c8
Part 3: reftest for tab-size:<length> and tab-size:<number>. r=heycam
Keywords: checkin-needed
(Assignee)

Comment 41

a year ago
Little help, dholbert? The data URIs that it's spitting out for me to compare the outputs don't seem to be appreciably different to my eyes, but evidently they're off by a few too many pixels:

>image comparison, max difference: 50, number of differing pixels: 117
>(https://treeherder.mozilla.org/logviewer.html#?job_id=39903607&repo=mozilla-inbound#L60155)
> 
>image comparison, max difference: 143, number of differing pixels: 128
>(https://treeherder.mozilla.org/logviewer.html#?job_id=39903794&repo=mozilla-inbound#L59270)

Are these just expected antialiasing-related issues that I should fuzz away, or would you know of something subtler that I'd need to account for?
Flags: needinfo?(wisniewskit) → needinfo?(dholbert)
(In reply to Thomas Wisniewski from comment #41)
> Little help, dholbert? The data URIs that it's spitting out for me to
> compare the outputs don't seem to be appreciably different to my eyes, but
> evidently they're off by a few too many pixels:

FWIW, you can just use the analyzer (open analyser on the top bar of log viewer, or besides the log button on treeherder) to see the difference. You can use it to figure out the actually difference directly.

> >image comparison, max difference: 50, number of differing pixels: 117
> >(https://treeherder.mozilla.org/logviewer.html#?job_id=39903607&repo=mozilla-inbound#L60155)
> > 
> >image comparison, max difference: 143, number of differing pixels: 128
> >(https://treeherder.mozilla.org/logviewer.html#?job_id=39903794&repo=mozilla-inbound#L59270)
> 
> Are these just expected antialiasing-related issues that I should fuzz away,
> or would you know of something subtler that I'd need to account for?

This kind of minor difference should be resolved by just adding fuzzies.

I think it would be better if you use Ahem font instead. That may still have fuzzy issues, but the area of reliable pixels should be larger than using a random font.
(Assignee)

Comment 43

a year ago
Thanks, xidorn! I'll assess whether using Ahem is good enough, and figure out how to add enough fuzz otherwise.
Flags: needinfo?(dholbert)
(Assignee)

Comment 44

a year ago
Sadly, while OSX is happier with Ahem, it looks like Win8 still actually need some fuzz (in fact, possibly more of it): https://treeherder.mozilla.org/logviewer.html#?job_id=31931678&repo=try#L60160

But at least all that's left is adding the fuzz, so I should be able to submit this for check-in again soon.
(Assignee)

Comment 45

a year ago
Created attachment 8815039 [details] [diff] [review]
943918-3-reftest_for_tab_size_length_and_tab_size_number.diff

Alright, here's a new reftest that uses the Ahem font instead, and fuzzes for Windows (OSX was fine with just the change to Ahem). Try confirms it should be fine: /media/ssd/mozilla/943918-3-reftest_for_tab_size_length_and_tab_size_number.diff

Xidorn, mind giving this version a quick final review to make sure I'm not doing anything odd with Ahem or fuzzing?
Attachment #8814415 - Attachment is obsolete: true
Attachment #8815039 - Flags: review?(xidorn+moz)
Comment on attachment 8815039 [details] [diff] [review]
943918-3-reftest_for_tab_size_length_and_tab_size_number.diff

Review of attachment 8815039 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. It would be more clearer if you can mark cases with what happens there. e.g. for negative plain values and percentage, mark that they are invalid so fallback to the initial "8"; for negative values from calc(), mark that they are clamped; etc.

::: layout/reftests/tab-size/tab-size-length-ref.html
@@ +39,5 @@
> +#t17 tab { width:20px; } /* 0.0 */
> +#t18 tab { width:40px; } /* 1 */
> +#t19 tab { width:60px; } /* 3 */
> +#t20 tab { width:68px; } /* 3.4 */
> +#t21 tab { width:160px; } /* -1.5 */

I'd prefer you reindent these lines to align with the content you added above.
Attachment #8815039 - Flags: review?(xidorn+moz) → review+
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #47)
> Looks good. It would be more clearer if you can mark cases with what happens
> there. e.g. for negative plain values and percentage, mark that they are
> invalid so fallback to the initial "8"; for negative values from calc(),
> mark that they are clamped; etc.

I mean, in the reference page. Since it isn't always easy to understand from the numbers directly.
(Assignee)

Comment 49

a year ago
Would it be sufficient to just change the comments in the ref file to ones like this, in place of the ones I have there?

>/* tab-size:0.8ch (valid) */
>/* tab-size:calc(-2em) (valid, but clamped to zero) */
>/* tab-size:calc(10% + 2em) (invalid, rule ignored, falls back to initial value 8) */

I'll also have the usual link to the spec section in the meta tag in both files, just in case.
Flags: needinfo?(xidorn+moz)
(In reply to Thomas Wisniewski from comment #49)
> Would it be sufficient to just change the comments in the ref file to ones
> like this, in place of the ones I have there?
> 
> >/* tab-size:0.8ch (valid) */
> >/* tab-size:calc(-2em) (valid, but clamped to zero) */
> >/* tab-size:calc(10% + 2em) (invalid, rule ignored, falls back to initial value 8) */

Yeah, just changing the comments in the reference file like this should be fine.
Flags: needinfo?(xidorn+moz)
(Assignee)

Comment 51

a year ago
Created attachment 8815155 [details] [diff] [review]
943918-3-reftest_for_tab_size_length_and_tab_size_number.diff

Alright, thanks! Here's the final version.

The patches still apply cleanly on inbound, so I'll request checkin.
Attachment #8815039 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 52

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/619201a8d2b8
Part 1: style system support for tab-size:<length> and tab-size:<number>. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/908bee3b62a9
Part 2: layout for tab-size:<length> and tab-size:<number>. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea44d42c5517
Part 3: reftest for tab-size:<length> and tab-size:<number>. r=heycam,r=xidorn
Keywords: checkin-needed

Comment 53

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/619201a8d2b8
https://hg.mozilla.org/mozilla-central/rev/908bee3b62a9
https://hg.mozilla.org/mozilla-central/rev/ea44d42c5517
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Updated

a year ago
Keywords: dev-doc-needed
Updated compatibility at https://developer.mozilla.org/en-US/docs/Web/CSS/tab-size and added a release note at https://developer.mozilla.org/en-US/Firefox/Releases/53.

Sebastian
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.