RESOLVED FIXED 52994
WebKit should support tab-size.
https://bugs.webkit.org/show_bug.cgi?id=52994
Summary WebKit should support tab-size.
Hajime Morrita
Reported 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
Attachments
Patch (42.11 KB, patch)
2011-01-25 17:46 PST, Hajime Morrita
no flags
Patch (47.19 KB, patch)
2011-01-31 23:01 PST, Hajime Morrita
no flags
Took feedback and caught up to ToT (46.56 KB, patch)
2011-03-04 05:14 PST, Hajime Morrita
no flags
Patch (50.43 KB, patch)
2011-05-26 02:36 PDT, Hajime Morrita
webkit.review.bot: commit-queue-
Patch (59.03 KB, patch)
2011-05-26 03:39 PDT, Hajime Morrita
webkit.review.bot: commit-queue-
Patch (75.08 KB, patch)
2011-05-26 04:38 PDT, Hajime Morrita
no flags
Archive of layout-test-results from ec2-cr-linux-02 (1.25 MB, application/zip)
2011-05-26 06:25 PDT, WebKit Review Bot
no flags
Patch (75.76 KB, patch)
2011-05-26 17:56 PDT, Hajime Morrita
no flags
I'm back\! (35.06 KB, patch)
2011-12-21 03:11 PST, Hajime Morrita
no flags
Fixed test failure (38.09 KB, patch)
2011-12-21 05:30 PST, Hajime Morrita
no flags
Patch (38.18 KB, patch)
2011-12-22 19:00 PST, Hajime Morrita
no flags
smfr, hyatt: could you take another look? (29.85 KB, patch)
2012-01-10 17:33 PST, Hajime Morrita
no flags
Updated test expectaiotns - smfr, hyatt: could you take another look? (38.14 KB, patch)
2012-01-10 20:14 PST, Hajime Morrita
no flags
Patch for landing (33.72 KB, patch)
2012-05-09 22:57 PDT, Hajime Morrita
no flags
Patch for landing (35.89 KB, patch)
2012-05-10 00:38 PDT, Hajime Morrita
no flags
Archive of layout-test-results from ec2-cq-03 (876.13 KB, application/zip)
2012-05-10 03:23 PDT, WebKit Review Bot
no flags
Patch for landing (39.06 KB, patch)
2012-05-10 04:10 PDT, Hajime Morrita
no flags
Patch for landing (39.07 KB, patch)
2012-05-10 05:26 PDT, Hajime Morrita
no flags
Patch for landing (40.07 KB, patch)
2012-05-10 17:07 PDT, Hajime Morrita
no flags
Hajime Morrita
Comment 1 2011-01-25 17:46:18 PST
Hajime Morrita
Comment 2 2011-01-25 17:48:09 PST
Hi wizards, could you review this? This should just replace hard-coded "8" with some variables.
Simon Fraser (smfr)
Comment 3 2011-01-31 12:57:28 PST
Is this in a standard anywhere?
Dave Hyatt
Comment 4 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.
Simon Fraser (smfr)
Comment 5 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
Dave Hyatt
Comment 6 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.
mitz
Comment 7 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'
Dave Hyatt
Comment 8 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.
Peter Beverloo
Comment 9 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
Hajime Morrita
Comment 10 2011-01-31 23:01:38 PST
Hajime Morrita
Comment 11 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.
Hajime Morrita
Comment 12 2011-03-04 05:14:20 PST
Created attachment 84730 [details] Took feedback and caught up to ToT
Kent Tamura
Comment 13 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
Eric Seidel (no email)
Comment 14 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.
Hajime Morrita
Comment 15 2011-05-26 02:36:10 PDT
Kent Tamura
Comment 16 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?
Hajime Morrita
Comment 17 2011-05-26 03:39:19 PDT
Hajime Morrita
Comment 18 2011-05-26 03:40:14 PDT
> Don't you have tab-size-expected.png? Good catch! I added it.
WebKit Review Bot
Comment 19 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
WebKit Review Bot
Comment 20 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
Hajime Morrita
Comment 21 2011-05-26 04:38:43 PDT
Hajime Morrita
Comment 22 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.
WebKit Review Bot
Comment 23 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
WebKit Review Bot
Comment 24 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
WebKit Review Bot
Comment 25 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
Hajime Morrita
Comment 26 2011-05-26 17:56:46 PDT
Hajime Morrita
Comment 27 2011-05-27 01:46:01 PDT
Bots are almost green! It's review ready now.
Hajime Morrita
Comment 28 2011-12-21 03:11:25 PST
Created attachment 120164 [details] I'm back\!
WebKit Review Bot
Comment 29 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
Hajime Morrita
Comment 30 2011-12-21 05:30:41 PST
Created attachment 120171 [details] Fixed test failure
Mathias Bynens
Comment 31 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)
Hajime Morrita
Comment 32 2011-12-22 19:00:22 PST
Hajime Morrita
Comment 33 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.
Hajime Morrita
Comment 34 2012-01-10 17:33:44 PST
Created attachment 121950 [details] smfr, hyatt: could you take another look?
WebKit Review Bot
Comment 35 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
Hajime Morrita
Comment 36 2012-01-10 20:14:06 PST
Created attachment 121966 [details] Updated test expectaiotns - smfr, hyatt: could you take another look?
Mathias Bynens
Comment 37 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
Simon Fraser (smfr)
Comment 38 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.
Lea Verou
Comment 39 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.
Hajime Morrita
Comment 40 2012-05-09 22:57:41 PDT
Created attachment 141087 [details] Patch for landing
Hajime Morrita
Comment 41 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.
WebKit Review Bot
Comment 42 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
Hajime Morrita
Comment 43 2012-05-10 00:38:37 PDT
Created attachment 141101 [details] Patch for landing
WebKit Review Bot
Comment 44 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
WebKit Review Bot
Comment 45 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
Hajime Morrita
Comment 46 2012-05-10 04:10:01 PDT
Created attachment 141140 [details] Patch for landing
WebKit Review Bot
Comment 47 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.
Build Bot
Comment 48 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
Hajime Morrita
Comment 49 2012-05-10 05:26:34 PDT
Created attachment 141152 [details] Patch for landing
Build Bot
Comment 50 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
Hajime Morrita
Comment 51 2012-05-10 17:07:54 PDT
Created attachment 141296 [details] Patch for landing
WebKit Review Bot
Comment 52 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>
WebKit Review Bot
Comment 53 2012-05-10 20:28:47 PDT
All reviewed patches have been landed. Closing bug.
yisibl
Comment 54 2015-12-29 07:10:02 PST
tab-size should accept <length> values Blink(Chrome 42) support
Simon Fraser (smfr)
Comment 55 2015-12-31 20:38:10 PST
Please file a new bug, rather than commenting in this old, closed one.
Note You need to log in before you can comment on or make changes to this bug.