Bug 179022

Summary: Implement tab-size with units
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: CSSAssignee: Joonghun Park <jh718.park>
Status: RESOLVED FIXED    
Severity: Normal CC: 50167214, browserbugs2, commit-queue, darin, ews-watchlist, gyuyoung.kim, jh718.park, koivisto, mmaxfield, rniwa, sam, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: BrowserCompat, InRadar, W3CTest, WebExposed
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
URL: https://drafts.csswg.org/css-text-3/#tab-size-property
Attachments:
Description Flags
Patch
none
Add TabSize header to Source/WebCore/Headers.cmake
none
Archive of layout-test-results from ews107 for mac-highsierra-wk2
none
Archive of layout-test-results from ews103 for mac-highsierra
none
Archive of layout-test-results from ews112 for mac-highsierra
none
Archive of layout-test-results from ews125 for ios-simulator-wk2
none
Update Layout Test And Result
none
Apply Simon's comments
none
Patch For Landing none

Description Simon Fraser (smfr) 2017-10-30 11:11:33 PDT
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"
Comment 1 GĂ©rard Talbot 2019-02-28 10:44:06 PST
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
Comment 2 Simon Fraser (smfr) 2019-02-28 11:22:42 PST
*** Bug 152683 has been marked as a duplicate of this bug. ***
Comment 3 Joonghun Park 2019-06-05 07:26:49 PDT
Created attachment 371393 [details]
Patch
Comment 4 Joonghun Park 2019-06-05 07:55:37 PDT
Created attachment 371395 [details]
Add TabSize header to Source/WebCore/Headers.cmake
Comment 5 EWS Watchlist 2019-06-05 09:03:16 PDT
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
Comment 6 EWS Watchlist 2019-06-05 09:03:18 PDT
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
Comment 7 EWS Watchlist 2019-06-05 09:12:10 PDT
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
Comment 8 EWS Watchlist 2019-06-05 09:12:12 PDT
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
Comment 9 EWS Watchlist 2019-06-05 09:51:03 PDT
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
Comment 10 EWS Watchlist 2019-06-05 09:51:07 PDT
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
Comment 11 EWS Watchlist 2019-06-05 10:01:05 PDT
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
Comment 12 EWS Watchlist 2019-06-05 10:01:06 PDT
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
Comment 13 Joonghun Park 2019-06-05 17:42:46 PDT
Created attachment 371460 [details]
Update Layout Test And Result
Comment 14 Simon Fraser (smfr) 2019-06-05 18:45:47 PDT
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.
Comment 15 Joonghun Park 2019-06-05 23:37:54 PDT
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.
Comment 16 Joonghun Park 2019-06-05 23:43:22 PDT
Created attachment 371477 [details]
Apply Simon's comments
Comment 17 Simon Fraser (smfr) 2019-06-06 10:53:35 PDT
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 ?
Comment 18 Joonghun Park 2019-06-06 23:12:08 PDT
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.
Comment 19 Joonghun Park 2019-06-06 23:13:40 PDT
Created attachment 371566 [details]
Patch For Landing
Comment 20 WebKit Commit Bot 2019-06-07 01:27:31 PDT
Comment on attachment 371566 [details]
Patch For Landing

Clearing flags on attachment: 371566

Committed r246193: <https://trac.webkit.org/changeset/246193>
Comment 21 WebKit Commit Bot 2019-06-07 01:27:34 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Radar WebKit Bug Importer 2019-06-07 01:28:21 PDT
<rdar://problem/51515096>