Bug 52994

Summary: WebKit should support tab-size.
Product: WebKit Reporter: Hajime Morrita <morrita@google.com>
Component: CSSAssignee: Hajime Morrita <morrita@google.com>
Status: RESOLVED FIXED    
Severity: Normal CC: alexis@webkit.org, ap@webkit.org, bugmail@eligrey.com, dglazkov@chromium.org, eric@webkit.org, hyatt@apple.com, kangax@gmail.com, lea@w3.org, macpherson@chromium.org, mathias@qiwi.be, medikoo+webkit.org@medikoo.com, mitz@webkit.org, ojan@chromium.org, paulirish@chromium.org, peter@chromium.org, rniwa@webkit.org, simon.fraser@apple.com, timothy@apple.com, tkent@chromium.org, webkit.bugzilla@jyp.ch, webkit.review.bot@gmail.com
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Mac OS X 10.5   
Bug Depends on: 86179    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Took feedback and caught up to ToT
none
Patch
webkit.review.bot: commit‑queue-
Patch
webkit.review.bot: commit‑queue-
Patch
none
Archive of layout-test-results from ec2-cr-linux-02
none
Patch
none
I'm back\!
none
Fixed test failure
none
Patch
none
smfr, hyatt: could you take another look?
none
Updated test expectaiotns - smfr, hyatt: could you take another look?
none
Patch for landing
none
Patch for landing
none
Archive of layout-test-results from ec2-cq-03
none
Patch for landing
none
Patch for landing
none
Patch for landing none

Description From 2011-01-23 23:50:34 PST
Mozilla and Opera have this. So it's nice for us to have.
https://developer.mozilla.org/en/CSS/-moz-tab-size
------- Comment #1 From 2011-01-25 17:46:18 PST -------
Created an attachment (id=80152) [details]
Patch
------- Comment #2 From 2011-01-25 17:48:09 PST -------
Hi wizards, could you review this? This should just replace hard-coded "8" with some variables.
------- Comment #3 From 2011-01-31 12:57:28 PST -------
Is this in a standard anywhere?
------- Comment #4 From 2011-01-31 13:00:52 PST -------
(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.
------- Comment #5 From 2011-01-31 13:04:38 PST -------
(From update of attachment 80152 [details])
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
------- Comment #6 From 2011-01-31 13:07:23 PST -------
(From update of attachment 80152 [details])
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 #7 From 2011-01-31 13:08:47 PST -------
(From update of attachment 80152 [details])
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'
------- Comment #8 From 2011-01-31 13:13:48 PST -------
Yeah, auto seems pretty pointless, especially if it's just going to end up being identical to initial.
------- Comment #9 From 2011-01-31 13:15:19 PST -------
(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
------- Comment #10 From 2011-01-31 23:01:38 PST -------
Created an attachment (id=80719) [details]
Patch
------- Comment #11 From 2011-01-31 23:05:42 PST -------
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] [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.
------- Comment #12 From 2011-03-04 05:14:20 PST -------
Created an attachment (id=84730) [details]
Took feedback and caught up to ToT
------- Comment #13 From 2011-05-05 22:31:52 PST -------
(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
------- Comment #14 From 2011-05-23 14:40:34 PST -------
(From update of attachment 84730 [details])
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.
------- Comment #15 From 2011-05-26 02:36:10 PST -------
Created an attachment (id=94950) [details]
Patch
------- Comment #16 From 2011-05-26 02:40:26 PST -------
(From update of attachment 94950 [details])
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?
------- Comment #17 From 2011-05-26 03:39:19 PST -------
Created an attachment (id=94955) [details]
Patch
------- Comment #18 From 2011-05-26 03:40:14 PST -------
> Don't you have tab-size-expected.png?
Good catch! I added it.
------- Comment #19 From 2011-05-26 04:14:33 PST -------
(From update of attachment 94955 [details])
Attachment 94955 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8741031
------- Comment #20 From 2011-05-26 04:26:53 PST -------
(From update of attachment 94950 [details])
Attachment 94950 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8736226
------- Comment #21 From 2011-05-26 04:38:43 PST -------
Created an attachment (id=94960) [details]
Patch
------- Comment #22 From 2011-05-26 04:47:42 PST -------
(From update of attachment 84730 [details])
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.
------- Comment #23 From 2011-05-26 05:09:50 PST -------
(From update of attachment 94955 [details])
Attachment 94955 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8736235
------- Comment #24 From 2011-05-26 06:25:47 PST -------
(From update of attachment 94960 [details])
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
------- Comment #25 From 2011-05-26 06:25:52 PST -------
Created an attachment (id=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
------- Comment #26 From 2011-05-26 17:56:46 PST -------
Created an attachment (id=95089) [details]
Patch
------- Comment #27 From 2011-05-27 01:46:01 PST -------
Bots are almost green! It's review ready now.
------- Comment #28 From 2011-12-21 03:11:25 PST -------
Created an attachment (id=120164) [details]
I'm back\!
------- Comment #29 From 2011-12-21 04:07:16 PST -------
(From update of attachment 120164 [details])
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
------- Comment #30 From 2011-12-21 05:30:41 PST -------
Created an attachment (id=120171) [details]
Fixed test failure
------- Comment #31 From 2011-12-22 03:31:05 PST -------
(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)
------- Comment #32 From 2011-12-22 19:00:22 PST -------
Created an attachment (id=120423) [details]
Patch
------- Comment #33 From 2011-12-22 19:01:54 PST -------
> 
> Actually, there is: &Tab;
> 
> (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.
------- Comment #34 From 2012-01-10 17:33:44 PST -------
Created an attachment (id=121950) [details]
smfr, hyatt: could you take another look?
------- Comment #35 From 2012-01-10 18:26:07 PST -------
(From update of attachment 121950 [details])
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
------- Comment #36 From 2012-01-10 20:14:06 PST -------
Created an attachment (id=121966) [details]
Updated test expectaiotns - smfr, hyatt: could you take another look?
------- Comment #37 From 2012-03-21 03:45:51 PST -------
(In reply to comment #3)
> Is this in a standard anywhere?

Right here: http://www.w3.org/TR/css3-text/#tab-size0
------- Comment #38 From 2012-05-04 12:54:05 PST -------
(From update of attachment 121966 [details])
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.
------- Comment #39 From 2012-05-04 13:02:42 PST -------
(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.
------- Comment #40 From 2012-05-09 22:57:41 PST -------
Created an attachment (id=141087) [details]
Patch for landing
------- Comment #41 From 2012-05-09 22:59:47 PST -------
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.
------- Comment #42 From 2012-05-10 00:06:57 PST -------
(From update of attachment 141087 [details])
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
------- Comment #43 From 2012-05-10 00:38:37 PST -------
Created an attachment (id=141101) [details]
Patch for landing
------- Comment #44 From 2012-05-10 03:23:40 PST -------
(From update of attachment 141101 [details])
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
------- Comment #45 From 2012-05-10 03:23:47 PST -------
Created an attachment (id=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
------- Comment #46 From 2012-05-10 04:10:01 PST -------
Created an attachment (id=141140) [details]
Patch for landing
------- Comment #47 From 2012-05-10 04:31:13 PST -------
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.
------- Comment #48 From 2012-05-10 04:51:26 PST -------
(From update of attachment 141140 [details])
Attachment 141140 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12666114
------- Comment #49 From 2012-05-10 05:26:34 PST -------
Created an attachment (id=141152) [details]
Patch for landing
------- Comment #50 From 2012-05-10 07:10:12 PST -------
(From update of attachment 141152 [details])
Attachment 141152 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12655881
------- Comment #51 From 2012-05-10 17:07:54 PST -------
Created an attachment (id=141296) [details]
Patch for landing
------- Comment #52 From 2012-05-10 20:28:05 PST -------
(From update of attachment 141296 [details])
Clearing flags on attachment: 141296

Committed r116723: <http://trac.webkit.org/changeset/116723>
------- Comment #53 From 2012-05-10 20:28:47 PST -------
All reviewed patches have been landed.  Closing bug.