Bug 52994 – WebKit should support tab-size.

• Component: CSS
• Status: RESOLVED FIXED

[Hajime Morrita]

Mozilla and Opera have this. So it's nice for us to have.
https://developer.mozilla.org/en/CSS/-moz-tab-size

[Hajime Morrita]

Created attachment 80152 [details]
Patch

[Hajime Morrita]

Hi wizards, could you review this? This should just replace hard-coded "8" with some variables.

[Simon Fraser (smfr)]

Is this in a standard anywhere?

[Dave Hyatt]

Comment on attachment 80152 [details]
Patch

You forgot to patch RenderStyle::diff to return a Layout hint if this property changes.  I'd like to see a test that dynamically changes the tab-size, since this was overlooked.

[Simon Fraser (smfr)]

Comment on attachment 80152 [details]
Patch

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

> Source/WebCore/css/CSSStyleSelector.cpp:5219
> +        if (primitiveValue)
> +            m_style->setTabSize(std::max(0, primitiveValue->getIntValue()));

See bug 53449

[Dave Hyatt]

Comment on attachment 80152 [details]
Patch

Also, don't you need auto to map back to the initial value?  It seems like auto is doing nothing right now.  Make a test where body has a tab size of 20 and then make a div inside that body set the tab size to auto.  It should go back to 8, but with your code I bet it stays at 20.

Comment on attachment 80152 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:1528
> +        if (id == CSSValueAuto)
> +            validPrimitive = true;

Why is 'auto' allowed and how is it applied? The Mozilla version doesn’t appear to support 'auto'

[Dave Hyatt]

Yeah, auto seems pretty pointless, especially if it's just going to end up being identical to initial.

[Peter Beverloo]

(In reply to comment #3)
> Is this in a standard anywhere?

No. Anne van Kesteren did post a brief rationale for it on www-style late 2008[1], based on which Elika Etemad raised ISSUE-76.

[1] http://lists.w3.org/Archives/Public/www-style/2008Dec/0009.html
[2] http://www.w3.org/Style/CSS/Tracker/issues/76

[Hajime Morrita]

Created attachment 80719 [details]
Patch

[Hajime Morrita]

Hyatt, Mitz, thank you for take a look!
I updated the patch for addressing your feedback.
Could you take another look?

> (From update of attachment 80152 [details])
> You forgot to patch RenderStyle::diff to return a Layout hint if this property changes.  I'd like to see a test that dynamically changes the tab-size, since this was overlooked.
Changed RenderStyle::diff() to handle tabSize and added a dynamic change case to the test.

I also removed "auto" value handling because it doesn't make sense to have "auto" for tab-size,
Mozilla also doesn't have that.

[Hajime Morrita]

Created attachment 84730 [details]
Took feedback and caught up to ToT

[Kent Tamura]

(In reply to comment #9)
> (In reply to comment #3)
> > Is this in a standard anywhere?
> 
> No. Anne van Kesteren did post a brief rationale for it on www-style late 2008[1], based on which Elika Etemad raised ISSUE-76.
> 
> [1] http://lists.w3.org/Archives/Public/www-style/2008Dec/0009.html
> [2] http://www.w3.org/Style/CSS/Tracker/issues/76

Now a draft of CSS3 text has tab-size.
http://www.w3.org/TR/2011/WD-css3-text-20110412/#tab-size

[Eric Seidel (no email)]

Comment on attachment 84730 [details]
Took feedback and caught up to ToT

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

> LayoutTests/fast/css/tab-size.html:8
> +    window.setTimeout(function() {

Why do we need setTimeout here?

> LayoutTests/fast/css/tab-size.html:19
> +<pre>&#9;x</pre>

Sad that there is no named entity for tab... or is there?

> LayoutTests/fast/css/tab-size.html:44
> +<div style="-webkit-tab-size: 2;">
> +<pre>&#9;x</pre>
> +<pre>&#9;&#9;x</pre>
> +<pre>&#9;x&#9;x</pre>
> +<pre>x&#9;&#9;x</pre>
> +<pre>x&#9;x&#9;x</pre>
> +<pre>xx&#9;xx&#9;x</pre>
> +<pre>xxx&#9;xxx&#9;x</pre>
> +</div>
> +<div id="dynamic">
> +<pre>&#9;x</pre>
> +<pre>&#9;&#9;x</pre>
> +<pre>&#9;x&#9;x</pre>
> +<pre>x&#9;&#9;x</pre>
> +<pre>x&#9;x&#9;x</pre>
> +<pre>xx&#9;xx&#9;x</pre>
> +<pre>xxx&#9;xxx&#9;x</pre>
> +</div>

Seems like a lot of copy/paste in this test.  Can't we just generate more of this from JS to make it easier to read?

I would also like us to test negative tab sizes.  What does the spec say for those?

> LayoutTests/platform/mac/fast/css/tab-size-expected.txt:4
> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x600
> +  RenderBlock {HTML} at (0,0) size 800x600

Does this need to be a render dump test?

> Source/WebCore/css/CSSStyleSelector.cpp:5593
> +        HANDLE_INHERIT_AND_INITIAL(tabSize, TabSize);
> +        if (primitiveValue)
> +            m_style->setTabSize(std::max(0, primitiveValue->getIntValue()));

Luke may have comment on if this is "modern" style.

> Source/WebCore/platform/graphics/TextRun.h:41
> +    TextRun(const UChar* c, int len, int tabSize = 0, float xpos = 0, float expansion = 0, TrailingExpansionBehavior trailingExpansionBehavior = AllowTrailingExpansion, bool rtl = false, bool directionalOverride = false)

Is 0 the right default?

> Source/WebCore/platform/graphics/TextRun.h:95
> +    bool allowTabs() const { return m_tabSize; }

This is a bit odd, honestly.  Should at least have a comment to explain why we're using m_tabSize == 0 to mean "disallow tabs" (whatever that even means?)

> Source/WebCore/platform/graphics/TextRun.h:130
> +    int m_tabSize;

do we want to allow negative tab sizes?  If so, we should test them.

> Source/WebCore/rendering/RenderText.cpp:577
> +        float tabWidth = allowTabs() ? monospaceCharacterWidth * style()->tabSize() : 0;

Seems this "allowTabs" check isn't needed if we're using 0 to mean "disallowed"?  Or I guess it is since we're checking against the style?

> Source/WebCore/rendering/RenderText.h:121
>      bool allowTabs() const { return !style()->collapseWhiteSpace(); }

Seems we should just rename allowTabs to be collapseWhiteSpace!  Since that makes a lot more sense with tabSize == 0. Or at least collapseTabs.

> Source/WebCore/rendering/style/RenderStyle.cpp:413
> +            || rareInheritedData->tabSize != other->rareInheritedData->tabSize)

Indent 4 spaces.

> Source/WebCore/rendering/style/StyleRareInheritedData.cpp:55
> +    , tabSize(RenderStyle::initialTabSize())

Please use m_ for new members.

[Hajime Morrita]

Created attachment 94950 [details]
Patch

[Kent Tamura]

Comment on attachment 94950 [details]
Patch

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

> LayoutTests/ChangeLog:12
> +        * fast/css/tab-size.html: Added.
> +        * platform/mac/fast/css/getComputedStyle/computed-style-without-renderer-expected.txt: Removed.
> +        * platform/mac/fast/css/tab-size-expected.txt: Added.

Don't you have tab-size-expected.png?

[Hajime Morrita]

Created attachment 94955 [details]
Patch

[Hajime Morrita]

> Don't you have tab-size-expected.png?
Good catch! I added it.

[WebKit Review Bot]

Comment on attachment 94955 [details]
Patch

Attachment 94955 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8741031

[WebKit Review Bot]

Comment on attachment 94950 [details]
Patch

Attachment 94950 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8736226

[Hajime Morrita]

Created attachment 94960 [details]
Patch

[Hajime Morrita]

Comment on attachment 84730 [details]
Took feedback and caught up to ToT

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

I updated the patch to address Eric's point.
Thank you for taking a look this aged patch!
Tab-size is now a part of the standard and I think we are ready to add it.

>> LayoutTests/fast/css/tab-size.html:8
>> +    window.setTimeout(function() {
> 
> Why do we need setTimeout here?

I want to ensure to run this after the first full layout
because this tests dynamic property change.

>> LayoutTests/fast/css/tab-size.html:19
>> +<pre>&#9;x</pre>
> 
> Sad that there is no named entity for tab... or is there?

Unfortunately, no. We could use actual tab character but it would be worse than the number-based entity.

>> LayoutTests/fast/css/tab-size.html:44
>> +</div>
> 
> Seems like a lot of copy/paste in this test.  Can't we just generate more of this from JS to make it easier to read?
> 
> I would also like us to test negative tab sizes.  What does the spec say for those?

That makes sense. Changed to use script for generating the cases.

>> LayoutTests/platform/mac/fast/css/tab-size-expected.txt:4
>> +  RenderBlock {HTML} at (0,0) size 800x600
> 
> Does this need to be a render dump test?

Yes. This change is all about layout,
and tab layout is not simple enough for giving a clear assertion() in the script.

>> Source/WebCore/css/CSSStyleSelector.cpp:5593
>> +            m_style->setTabSize(std::max(0, primitiveValue->getIntValue()));
> 
> Luke may have comment on if this is "modern" style.

Sure. now removed std::max() because the parser already rejected negative value before here.

>> Source/WebCore/platform/graphics/TextRun.h:41
>> +    TextRun(const UChar* c, int len, int tabSize = 0, float xpos = 0, float expansion = 0, TrailingExpansionBehavior trailingExpansionBehavior = AllowTrailingExpansion, bool rtl = false, bool directionalOverride = false)
> 
> Is 0 the right default?

Yes. that is equivalent to allowTabs = false, which was the original default.

>> Source/WebCore/platform/graphics/TextRun.h:95
>> +    bool allowTabs() const { return m_tabSize; }
> 
> This is a bit odd, honestly.  Should at least have a comment to explain why we're using m_tabSize == 0 to mean "disallow tabs" (whatever that even means?)

I noticed that we no longer need this. I remove allowTabs().

>> Source/WebCore/platform/graphics/TextRun.h:130
>> +    int m_tabSize;
> 
> do we want to allow negative tab sizes?  If so, we should test them.

Good catch. It's invalid. I changed this to unsigned.
I also added the test case for it.

>> Source/WebCore/rendering/RenderText.cpp:577
>> +        float tabWidth = allowTabs() ? monospaceCharacterWidth * style()->tabSize() : 0;
> 
> Seems this "allowTabs" check isn't needed if we're using 0 to mean "disallowed"?  Or I guess it is since we're checking against the style?

Good point. I added RenderStyle::collapsedTabSize() for handling these cases.

>> Source/WebCore/rendering/RenderText.h:121
>>      bool allowTabs() const { return !style()->collapseWhiteSpace(); }
> 
> Seems we should just rename allowTabs to be collapseWhiteSpace!  Since that makes a lot more sense with tabSize == 0. Or at least collapseTabs.

I removed this since we don't need this anymore.

>> Source/WebCore/rendering/style/RenderStyle.cpp:413
>> +            || rareInheritedData->tabSize != other->rareInheritedData->tabSize)
> 
> Indent 4 spaces.

Fixed.

>> Source/WebCore/rendering/style/StyleRareInheritedData.cpp:55
>> +    , tabSize(RenderStyle::initialTabSize())
> 
> Please use m_ for new members.

Sure. Added.

[WebKit Review Bot]

Comment on attachment 94955 [details]
Patch

Attachment 94955 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8736235

[WebKit Review Bot]

Comment on attachment 94960 [details]
Patch

Attachment 94960 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8733928

New failing tests:
fast/css/tab-size.html

[WebKit Review Bot]

Created attachment 94966 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Hajime Morrita]

Created attachment 95089 [details]
Patch

[Hajime Morrita]

Bots are almost green! It's review ready now.

[Hajime Morrita]

Created attachment 120164 [details]
I'm back\!

[WebKit Review Bot]

Comment on attachment 120164 [details]
I'm back\!

Attachment 120164 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10988666

New failing tests:
svg/css/getComputedStyle-basic.xhtml

[Hajime Morrita]

Created attachment 120171 [details]
Fixed test failure

[Mathias Bynens]

(In reply to comment #22)
> >> LayoutTests/fast/css/tab-size.html:19
> >> +<pre>&#9;x</pre>
> > 
> > Sad that there is no named entity for tab... or is there?
> 
> Unfortunately, no. We could use actual tab character but it would be worse than the number-based entity.

Actually, there is: &Tab;

(See http://www.whatwg.org/specs/web-apps/current-work/multipage/named-character-references.html)

[Hajime Morrita]

Created attachment 120423 [details]
Patch

[Hajime Morrita]

> 
> Actually, there is: 	
> 
> (See http://www.whatwg.org/specs/web-apps/current-work/multipage/named-character-references.html)
You are right. Thanks for the catch! Updated the patch to use it.

[Hajime Morrita]

Created attachment 121950 [details]
smfr, hyatt: could you take another look?

[WebKit Review Bot]

Comment on attachment 121950 [details]
smfr, hyatt: could you take another look?

Attachment 121950 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11196297

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html

[Hajime Morrita]

Created attachment 121966 [details]
Updated test expectaiotns - smfr, hyatt: could you take another look?

[Mathias Bynens]

(In reply to comment #3)
> Is this in a standard anywhere?

Right here: http://www.w3.org/TR/css3-text/#tab-size0

[Simon Fraser (smfr)]

Comment on attachment 121966 [details]
Updated test expectaiotns - smfr, hyatt: could you take another look?

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

> Source/WebCore/css/CSSParser.cpp:1890
> +        validPrimitive = !id && validUnit(value, FInteger | FNonNeg, false);

Should tab-size:0 be allowed?

> Source/WebCore/platform/graphics/TextRun.h:115
> +    bool allowTabs() const { return 0 < m_tabSize; }

I think m_tabSize > 0 would be more readable.

> LayoutTests/fast/css/tab-size.html:27
> +    setupBlock("Default tab size (8).", null);
> +    setupBlock("Tab size = -10, should fall back to the default.", "tab-size: -10;");
> +    setupBlock("Tab size = 2.", "tab-size: 2;");

Test tab-size: 0 too.

[Lea Verou]

(In reply to comment #38)
> Should tab-size:0 be allowed?

Yes, according to the draft. Both <integer> and <length> can be (generally) zero, and there is nothing stating that zero is invalid in tab-size, just that negative values are not allowed. Therefore, it's safe to assume zero should be allowed.
I can even remotely imagine some use cases for it.

[Hajime Morrita]

Created attachment 141087 [details]
Patch for landing

[Hajime Morrita]

Thanks for reviewing, Simon.
The landing patch adds a test for tab-size:0. It also distinguishes tab-size:0 case and non-tab case.

[WebKit Review Bot]

Comment on attachment 141087 [details]
Patch for landing

Rejecting attachment 141087 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
gument 5 of 'WebCore::TextRun::TextRun(const WTF::String&, float, float, unsigned int, WebCore::TextDirection, bool, bool, unsigned int)'
  CXX(target) out/Debug/obj.target/webcore_platform/Source/WebCore/platform/graphics/GeneratorGeneratedImage.o
  CXX(target) out/Debug/obj.target/webcore_platform/Source/WebCore/platform/graphics/GraphicsContext.o
make: *** [out/Debug/obj.target/webcore_platform/Source/WebCore/platform/chromium/PopupListBox.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/12663227

[Hajime Morrita]

Created attachment 141101 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 141101 [details]
Patch for landing

Rejecting attachment 141101 [details] from commit-queue.

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html
Full output: http://queues.webkit.org/results/12665221

[WebKit Review Bot]

Created attachment 141129 [details]
Archive of layout-test-results from ec2-cq-03

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Hajime Morrita]

Created attachment 141140 [details]
Patch for landing

[WebKit Review Bot]

Attachment 141140 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/platform/graphics/Font.h:163:  The parameter name "fontData" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 141140 [details]
Patch for landing

Attachment 141140 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12666114

[Hajime Morrita]

Created attachment 141152 [details]
Patch for landing

[Build Bot]

Comment on attachment 141152 [details]
Patch for landing

Attachment 141152 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12655881

[Hajime Morrita]

Created attachment 141296 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 141296 [details]
Patch for landing

Clearing flags on attachment: 141296

Committed r116723: <http://trac.webkit.org/changeset/116723>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[yisibl]

tab-size should accept <length> values

Blink(Chrome 42) support

[Simon Fraser (smfr)]

Please file a new bug, rather than commenting in this old, closed one.