WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Add TabSize header to Source/WebCore/Headers.cmake
(36.74 KB, patch)
2019-06-05 07:55 PDT
,
Joonghun Park
no flags
Details
Formatted Diff
Diff
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
Details
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
Details
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
Details
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
Details
Update Layout Test And Result
(41.11 KB, patch)
2019-06-05 17:42 PDT
,
Joonghun Park
no flags
Details
Formatted Diff
Diff
Apply Simon's comments
(41.47 KB, patch)
2019-06-05 23:43 PDT
,
Joonghun Park
no flags
Details
Formatted Diff
Diff
Patch For Landing
(42.89 KB, patch)
2019-06-06 23:13 PDT
,
Joonghun Park
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 371393
[details]
Patch
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
<
rdar://problem/51515096
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug