Bug 179022 – Implement tab-size with units

• Component: CSS
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

We should support tab-size with units.

This will require plumbing a Length through down to TextRun etc. We might want a new Length type which indicates "unitless value with special meaning"

[Gérard Talbot (no longer involved)]

I believe bug 152683 and this bug report are the same. If this is correct, then one of the 2 should be resolved as DUPLICATE.

- - - -

testing tab-size: calc(10px)
http://w3c-test.org/css/css-values/calc-numbers.html

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

[Simon Fraser (smfr)]

*** Bug 152683 has been marked as a duplicate of this bug. ***

[Joonghun Park]

Created attachment 371393 [details]
Patch

[Joonghun Park]

Created attachment 371395 [details]
Add TabSize header to Source/WebCore/Headers.cmake

[EWS Watchlist]

Comment on attachment 371395 [details]
Add TabSize header to Source/WebCore/Headers.cmake

Attachment 371395 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12384726

New failing tests:
fast/text/whitespace/simple-line-layout-tab-position.html
imported/w3c/web-platform-tests/css/css-text/inheritance.html
fast/text/simple-line-wordspacing.html
css3/tab-size.html
imported/w3c/web-platform-tests/css/css-text/parsing/tab-size-valid.html
fast/css/tab-size.html

[EWS Watchlist]

Created attachment 371404 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 371395 [details]
Add TabSize header to Source/WebCore/Headers.cmake

Attachment 371395 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12384855

New failing tests:
fast/text/whitespace/simple-line-layout-tab-position.html
imported/w3c/web-platform-tests/css/css-text/inheritance.html
fast/text/simple-line-wordspacing.html
css3/tab-size.html
imported/w3c/web-platform-tests/css/css-text/parsing/tab-size-valid.html
fast/css/tab-size.html

[EWS Watchlist]

Created attachment 371405 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 371395 [details]
Add TabSize header to Source/WebCore/Headers.cmake

Attachment 371395 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12384964

New failing tests:
fast/text/whitespace/simple-line-layout-tab-position.html
imported/w3c/web-platform-tests/css/css-text/inheritance.html
fast/text/simple-line-wordspacing.html
css3/tab-size.html
imported/w3c/web-platform-tests/css/css-text/parsing/tab-size-valid.html
fast/css/tab-size.html

[EWS Watchlist]

Created attachment 371411 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 371395 [details]
Add TabSize header to Source/WebCore/Headers.cmake

Attachment 371395 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/12384991

New failing tests:
fast/text/whitespace/simple-line-layout-tab-position.html
imported/w3c/web-platform-tests/css/css-text/inheritance.html
fast/text/simple-line-wordspacing.html
css3/tab-size.html
imported/w3c/web-platform-tests/css/css-text/parsing/tab-size-valid.html
fast/css/tab-size.html

[EWS Watchlist]

Created attachment 371415 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.14.5

[Joonghun Park]

Created attachment 371460 [details]
Update Layout Test And Result

[Simon Fraser (smfr)]

Comment on attachment 371460 [details]
Update Layout Test And Result

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3030
> +            return cssValuePool.createValue(
> +                style.tabSize().getPixelSize(1.0), style.tabSize().isSpaces() ? CSSPrimitiveValue::CSS_NUMBER : CSSPrimitiveValue::CSS_PX);

We don't wrap like this in WebKit code.

> Source/WebCore/css/StyleBuilderConverter.h:264
> +        return TabSize(primitiveValue.floatValue(), true);

We prefer enum to bool arguments.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:1126
> +    RefPtr<CSSPrimitiveValue> tabSize = consumeNumber(range, ValueRangeNonNegative);

auto tabSize

> Source/WebCore/layout/inlineformatting/text/TextUtil.cpp:63
> +    if (tabWidth.getPixelSize(1.0))

Why does passing 1.0 as the spaceWidth make sense here?

> Source/WebCore/platform/graphics/TabSize.h:35
> +    TabSize(float numOrLength, bool isSpaces = true)

isSpaces should be an enum class with two values

> Source/WebCore/platform/graphics/TabSize.h:46
> +    float getPixelSize(float spaceWidth) const

widthInPixels?

> Source/WebCore/platform/graphics/TabSize.h:52
> +    unsigned m_isSpaces : 1;

No point using a bitfield here; the struct will get padded anyway. Just use a bool.

> Source/WebCore/platform/graphics/TextRun.h:100
> +    void setTabSize(bool, TabSize);

Why is TabSize passed by value here, but by reference earlier?

> Source/WebCore/rendering/SimpleLineLayoutTextFragmentIterator.cpp:259
> +        if (m_style.tabWidth.getPixelSize(1.0))

That mystery 1.0 again.

[Joonghun Park]

Comment on attachment 371460 [details]
Update Layout Test And Result

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

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3030
>> +                style.tabSize().getPixelSize(1.0), style.tabSize().isSpaces() ? CSSPrimitiveValue::CSS_NUMBER : CSSPrimitiveValue::CSS_PX);
> 
> We don't wrap like this in WebKit code.

Ok, I will remove this wrap here.

>> Source/WebCore/css/StyleBuilderConverter.h:264
>> +        return TabSize(primitiveValue.floatValue(), true);
> 
> We prefer enum to bool arguments.

Ok, I will use enum here.

>> Source/WebCore/css/parser/CSSPropertyParser.cpp:1126
>> +    RefPtr<CSSPrimitiveValue> tabSize = consumeNumber(range, ValueRangeNonNegative);
> 
> auto tabSize

Done.

>> Source/WebCore/layout/inlineformatting/text/TextUtil.cpp:63
>> +    if (tabWidth.getPixelSize(1.0))
> 
> Why does passing 1.0 as the spaceWidth make sense here?

I used this to check if the value TabSize::m_floatValue is non-zero.
Then I will add 
operator bool() const { return m_floatValue; }
to TabSize.h instead of call to getPixelSize explicitly.

>> Source/WebCore/platform/graphics/TabSize.h:35
>> +    TabSize(float numOrLength, bool isSpaces = true)
> 
> isSpaces should be an enum class with two values

Done.

>> Source/WebCore/platform/graphics/TabSize.h:46
>> +    float getPixelSize(float spaceWidth) const
> 
> widthInPixels?

Ok, I will change the naming to this one.

>> Source/WebCore/platform/graphics/TabSize.h:52
>> +    unsigned m_isSpaces : 1;
> 
> No point using a bitfield here; the struct will get padded anyway. Just use a bool.

Done.

>> Source/WebCore/platform/graphics/TextRun.h:100
>> +    void setTabSize(bool, TabSize);
> 
> Why is TabSize passed by value here, but by reference earlier?

Ok, I will change these getter/setter from pass by value to pass by reference.

>> Source/WebCore/rendering/SimpleLineLayoutTextFragmentIterator.cpp:259
>> +        if (m_style.tabWidth.getPixelSize(1.0))
> 
> That mystery 1.0 again.

Ditto.

[Joonghun Park]

Created attachment 371477 [details]
Apply Simon's comments

[Simon Fraser (smfr)]

Comment on attachment 371477 [details]
Apply Simon's comments

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

> Source/WebCore/css/StyleBuilderConverter.h:260
> +inline TabSize StyleBuilderConverter::convertLengthOrTabSpaces(const StyleResolver& styleResolver, const CSSValue& value)

I think this should just be called convertTabSize()

> Source/WebCore/platform/graphics/TabSize.h:58
> +    float m_floatValue;

m_value

> LayoutTests/css3/tab-size.html:16
> +.int-tab-size {
> +  tab-size: 9;
> +}

What about fractional tab sizes: tab-size: 2.5 ?

[Joonghun Park]

Comment on attachment 371477 [details]
Apply Simon's comments

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

>> Source/WebCore/css/StyleBuilderConverter.h:260
>> +inline TabSize StyleBuilderConverter::convertLengthOrTabSpaces(const StyleResolver& styleResolver, const CSSValue& value)
> 
> I think this should just be called convertTabSize()

Ok, I will rename it with convertTabSize.

>> Source/WebCore/platform/graphics/TabSize.h:58
>> +    float m_floatValue;
> 
> m_value

Done.

>> LayoutTests/css3/tab-size.html:16
>> +}
> 
> What about fractional tab sizes: tab-size: 2.5 ?

Ok, I will add a fraction related test entry too.

[Joonghun Park]

Created attachment 371566 [details]
Patch For Landing

[WebKit Commit Bot]

Comment on attachment 371566 [details]
Patch For Landing

Clearing flags on attachment: 371566

Committed r246193: <https://trac.webkit.org/changeset/246193>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51515096>