sjw Reporter
	Description • 11 years ago

-moz-tab-size does not accept <length> values yet.

	Mats Palmgren (inactive)
	Comment 1 • 11 years ago

This doesn't look that hard to me, I'll put it on my list as a background task...

	Cameron McCormack (:heycam)
	Comment 5 • 11 years ago

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

	Cameron McCormack (:heycam)
	Comment 6 • 11 years ago

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?

	Cameron McCormack (:heycam)
	Comment 7 • 11 years ago

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?

	Cameron McCormack (:heycam)
	Comment 8 • 11 years ago

Comment on attachment 8340693 [details] [diff] [review]
Layout for tab-size:<length>

Cancelling review awaiting a response to the question in comment 6.

	Gordon P. Hemsley [:GPHemsley]
	Comment 9 • 11 years ago

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

	Cameron McCormack (:heycam)
	Comment 10 • 11 years ago

(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

	Gordon P. Hemsley [:GPHemsley]
	Comment 11 • 11 years ago

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

	Cameron McCormack (:heycam)
	Comment 12 • 11 years ago

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

	Gordon P. Hemsley [:GPHemsley]
	Comment 13 • 11 years ago

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

	sjw Reporter
	Comment 14 • 10 years ago

Is this patch stable enough for checkin?

	Mats Palmgren (inactive)
	Comment 15 • 10 years ago

(In reply to sjw from comment #14)
> Is this patch stable enough for checkin?

No, I need to address the review comments first.

	yisibl
	Comment 16 • 8 years ago

Not support calc() in tab-size.

	sjw Reporter
	Comment 17 • 8 years ago

See also bug 594933

	Mats Palmgren (inactive)
	Comment 19 • 8 years ago

Note the spec change that tab-size:0px should be supported:
https://github.com/w3c/csswg-drafts/issues/460

	Manish Goregaokar [:manishearth]
	Comment 20 • 8 years ago

Any updates?

	Thomas Wisniewski Assignee
	Comment 21 • 8 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 24 • 8 years ago

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.

	Thomas Wisniewski Assignee
	Comment 25 • 8 years 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)

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 26 • 8 years ago

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

	Thomas Wisniewski Assignee
	Comment 27 • 8 years ago

Alright, here's a new version of that patch with those issues addressed.

	Thomas Wisniewski Assignee
	Comment 28 • 8 years ago

Friendly review ping; let me know if another reviewer might be better, please and thanks!

	Cameron McCormack (:heycam)
	Comment 29 • 8 years ago

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

	Cameron McCormack (:heycam)
	Comment 30 • 8 years ago

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

	Cameron McCormack (:heycam)
	Comment 31 • 8 years ago

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

	Cameron McCormack (:heycam)
	Comment 32 • 8 years ago

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

	Thomas Wisniewski Assignee
	Comment 33 • 8 years 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.

	Thomas Wisniewski Assignee
	Comment 34 • 8 years ago

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.

	David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)
	Comment 38 • 8 years ago

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

	Pulsebot
	Comment 39 • 8 years 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

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 40 • 8 years ago

Backed out for failing own test tab-size-length.html on at least Windows 8 x64 opt+debug and OS X debug:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f7547f974ec95863a95923562ad0e843e6bd786
https://hg.mozilla.org/integration/mozilla-inbound/rev/6472cf401d391c2aec0a189e77c477ed7d715139
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7f46e6d453f9dce45b2f7e45cb5fa8d392b1034

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=39903423&repo=mozilla-inbound

Pushes with failures:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=cb3dc03015529ea5f36b51c360bb407357b7e5e2
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=bfee5b1a5508f7e7f31e737f97c82371e8916b67

	Thomas Wisniewski Assignee
	Comment 41 • 8 years 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?

	Xidorn Quan [:xidorn] UTC+11
	Comment 42 • 8 years ago

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

	Thomas Wisniewski Assignee
	Comment 43 • 8 years ago

Thanks, xidorn! I'll assess whether using Ahem is good enough, and figure out how to add enough fuzz otherwise.

	Thomas Wisniewski Assignee
	Comment 44 • 8 years 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.

	Thomas Wisniewski Assignee
	Comment 45 • 8 years ago

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?

	Thomas Wisniewski Assignee
	Comment 46 • 8 years ago

Ugh, sorry, the try-run link was this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4389a57fe2486afd2912165ab6f9031feee20b65

	Xidorn Quan [:xidorn] UTC+11
	Comment 47 • 8 years ago

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.

	Xidorn Quan [:xidorn] UTC+11
	Comment 48 • 8 years ago

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

	Thomas Wisniewski Assignee
	Comment 49 • 8 years 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.

	Xidorn Quan [:xidorn] UTC+11
	Comment 50 • 8 years ago

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

	Thomas Wisniewski Assignee
	Comment 51 • 8 years ago

Alright, thanks! Here's the final version.

The patches still apply cleanly on inbound, so I'll request checkin.

	Pulsebot
	Comment 52 • 8 years 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

	Carsten Book [:Tomcat]
	Comment 53 • 8 years 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

	Sebastian Zartner [:sebo]
	Comment 54 • 8 years ago

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