Bug 52994

Summary: WebKit should support tab-size.
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: CSSAssignee: Hajime Morrita <morrita>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bugmail, dglazkov, eric, hyatt, kangax, lea, macpherson, mathias, medikoo+webkit.org, menard, mitz, ojan, paulirish, peter, rniwa, simon.fraser, timothy, tkent, webkit.bugzilla, webkit.review.bot
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 Hajime Morrita 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 Hajime Morrita 2011-01-25 17:46:18 PST
Created attachment 80152 [details]
Patch
Comment 2 Hajime Morrita 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 Simon Fraser (smfr) 2011-01-31 12:57:28 PST
Is this in a standard anywhere?
Comment 4 Dave Hyatt 2011-01-31 13:00:52 PST
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.
Comment 5 Simon Fraser (smfr) 2011-01-31 13:04:38 PST
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
Comment 6 Dave Hyatt 2011-01-31 13:07:23 PST
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 7 mitz@webkit.org 2011-01-31 13:08:47 PST
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'
Comment 8 Dave Hyatt 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 Peter Beverloo 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 Hajime Morrita 2011-01-31 23:01:38 PST
Created attachment 80719 [details]
Patch
Comment 11 Hajime Morrita 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])
> 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 Hajime Morrita 2011-03-04 05:14:20 PST
Created attachment 84730 [details]
Took feedback and caught up to ToT
Comment 13 Kent Tamura 2011-05-05 22:31:52 PDT
(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 Eric Seidel 2011-05-23 14:40:34 PDT
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.
Comment 15 Hajime Morrita 2011-05-26 02:36:10 PDT
Created attachment 94950 [details]
Patch
Comment 16 Kent Tamura 2011-05-26 02:40:26 PDT
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?
Comment 17 Hajime Morrita 2011-05-26 03:39:19 PDT
Created attachment 94955 [details]
Patch
Comment 18 Hajime Morrita 2011-05-26 03:40:14 PDT
> Don't you have tab-size-expected.png?
Good catch! I added it.
Comment 19 WebKit Review Bot 2011-05-26 04:14:33 PDT
Comment on attachment 94955 [details]
Patch

Attachment 94955 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8741031
Comment 20 WebKit Review Bot 2011-05-26 04:26:53 PDT
Comment on attachment 94950 [details]
Patch

Attachment 94950 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8736226
Comment 21 Hajime Morrita 2011-05-26 04:38:43 PDT
Created attachment 94960 [details]
Patch
Comment 22 Hajime Morrita 2011-05-26 04:47:42 PDT
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.
Comment 23 WebKit Review Bot 2011-05-26 05:09:50 PDT
Comment on attachment 94955 [details]
Patch

Attachment 94955 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8736235
Comment 24 WebKit Review Bot 2011-05-26 06:25:47 PDT
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
Comment 25 WebKit Review Bot 2011-05-26 06:25:52 PDT
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
Comment 26 Hajime Morrita 2011-05-26 17:56:46 PDT
Created attachment 95089 [details]
Patch
Comment 27 Hajime Morrita 2011-05-27 01:46:01 PDT
Bots are almost green! It's review ready now.
Comment 28 Hajime Morrita 2011-12-21 03:11:25 PST
Created attachment 120164 [details]
I'm back\!
Comment 29 WebKit Review Bot 2011-12-21 04:07:16 PST
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
Comment 30 Hajime Morrita 2011-12-21 05:30:41 PST
Created attachment 120171 [details]
Fixed test failure
Comment 31 Mathias Bynens 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 Hajime Morrita 2011-12-22 19:00:22 PST
Created attachment 120423 [details]
Patch
Comment 33 Hajime Morrita 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 Hajime Morrita 2012-01-10 17:33:44 PST
Created attachment 121950 [details]
smfr, hyatt: could you take another look?
Comment 35 WebKit Review Bot 2012-01-10 18:26:07 PST
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
Comment 36 Hajime Morrita 2012-01-10 20:14:06 PST
Created attachment 121966 [details]
Updated test expectaiotns - smfr, hyatt: could you take another look?
Comment 37 Mathias Bynens 2012-03-21 03:45:51 PDT
(In reply to comment #3)
> Is this in a standard anywhere?

Right here: http://www.w3.org/TR/css3-text/#tab-size0
Comment 38 Simon Fraser (smfr) 2012-05-04 12:54:05 PDT
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.
Comment 39 Lea Verou 2012-05-04 13:02:42 PDT
(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 Hajime Morrita 2012-05-09 22:57:41 PDT
Created attachment 141087 [details]
Patch for landing
Comment 41 Hajime Morrita 2012-05-09 22:59:47 PDT
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 WebKit Review Bot 2012-05-10 00:06:57 PDT
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
Comment 43 Hajime Morrita 2012-05-10 00:38:37 PDT
Created attachment 141101 [details]
Patch for landing
Comment 44 WebKit Review Bot 2012-05-10 03:23:40 PDT
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
Comment 45 WebKit Review Bot 2012-05-10 03:23:47 PDT
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
Comment 46 Hajime Morrita 2012-05-10 04:10:01 PDT
Created attachment 141140 [details]
Patch for landing
Comment 47 WebKit Review Bot 2012-05-10 04:31:13 PDT
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 Build Bot 2012-05-10 04:51:26 PDT
Comment on attachment 141140 [details]
Patch for landing

Attachment 141140 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12666114
Comment 49 Hajime Morrita 2012-05-10 05:26:34 PDT
Created attachment 141152 [details]
Patch for landing
Comment 50 Build Bot 2012-05-10 07:10:12 PDT
Comment on attachment 141152 [details]
Patch for landing

Attachment 141152 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12655881
Comment 51 Hajime Morrita 2012-05-10 17:07:54 PDT
Created attachment 141296 [details]
Patch for landing
Comment 52 WebKit Review Bot 2012-05-10 20:28:05 PDT
Comment on attachment 141296 [details]
Patch for landing

Clearing flags on attachment: 141296

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