RESOLVED FIXED 179022
Implement tab-size with units
https://bugs.webkit.org/show_bug.cgi?id=179022
Summary Implement tab-size with units
Simon Fraser (smfr)
Reported 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"
Attachments
Patch (36.17 KB, patch)
2019-06-05 07:26 PDT, Joonghun Park
no flags
Add TabSize header to Source/WebCore/Headers.cmake (36.74 KB, patch)
2019-06-05 07:55 PDT, Joonghun Park
no flags
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (3.34 MB, application/zip)
2019-06-05 09:03 PDT, EWS Watchlist
no flags
Archive of layout-test-results from ews103 for mac-highsierra (3.45 MB, application/zip)
2019-06-05 09:12 PDT, EWS Watchlist
no flags
Archive of layout-test-results from ews112 for mac-highsierra (3.25 MB, application/zip)
2019-06-05 09:51 PDT, EWS Watchlist
no flags
Archive of layout-test-results from ews125 for ios-simulator-wk2 (3.32 MB, application/zip)
2019-06-05 10:01 PDT, EWS Watchlist
no flags
Update Layout Test And Result (41.11 KB, patch)
2019-06-05 17:42 PDT, Joonghun Park
no flags
Apply Simon's comments (41.47 KB, patch)
2019-06-05 23:43 PDT, Joonghun Park
no flags
Patch For Landing (42.89 KB, patch)
2019-06-06 23:13 PDT, Joonghun Park
no flags
Gérard Talbot (no longer involved)
Comment 1 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
Simon Fraser (smfr)
Comment 2 2019-02-28 11:22:42 PST
*** Bug 152683 has been marked as a duplicate of this bug. ***
Joonghun Park
Comment 3 2019-06-05 07:26:49 PDT
Joonghun Park
Comment 4 2019-06-05 07:55:37 PDT
Created attachment 371395 [details] Add TabSize header to Source/WebCore/Headers.cmake
EWS Watchlist
Comment 5 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
EWS Watchlist
Comment 6 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
EWS Watchlist
Comment 7 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
EWS Watchlist
Comment 8 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
EWS Watchlist
Comment 9 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
EWS Watchlist
Comment 10 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
EWS Watchlist
Comment 11 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
EWS Watchlist
Comment 12 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
Joonghun Park
Comment 13 2019-06-05 17:42:46 PDT
Created attachment 371460 [details] Update Layout Test And Result
Simon Fraser (smfr)
Comment 14 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.
Joonghun Park
Comment 15 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.
Joonghun Park
Comment 16 2019-06-05 23:43:22 PDT
Created attachment 371477 [details] Apply Simon's comments
Simon Fraser (smfr)
Comment 17 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 ?
Joonghun Park
Comment 18 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.
Joonghun Park
Comment 19 2019-06-06 23:13:40 PDT
Created attachment 371566 [details] Patch For Landing
WebKit Commit Bot
Comment 20 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>
WebKit Commit Bot
Comment 21 2019-06-07 01:27:34 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 22 2019-06-07 01:28:21 PDT
Note You need to log in before you can comment on or make changes to this bug.