Mozilla and Opera have this. So it's nice for us to have. https://developer.mozilla.org/en/CSS/-moz-tab-size
Created attachment 80152 [details] Patch
Hi wizards, could you review this? This should just replace hard-coded "8" with some variables.
Is this in a standard anywhere?
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 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 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'
Yeah, auto seems pretty pointless, especially if it's just going to end up being identical to initial.
(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
Created attachment 80719 [details] Patch
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.
Created attachment 84730 [details] Took feedback and caught up to ToT
(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 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>	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>	x</pre> > +<pre>		x</pre> > +<pre>	x	x</pre> > +<pre>x		x</pre> > +<pre>x	x	x</pre> > +<pre>xx	xx	x</pre> > +<pre>xxx	xxx	x</pre> > +</div> > +<div id="dynamic"> > +<pre>	x</pre> > +<pre>		x</pre> > +<pre>	x	x</pre> > +<pre>x		x</pre> > +<pre>x	x	x</pre> > +<pre>xx	xx	x</pre> > +<pre>xxx	xxx	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.
Created attachment 94950 [details] Patch
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?
Created attachment 94955 [details] Patch
> Don't you have tab-size-expected.png? Good catch! I added it.
Comment on attachment 94955 [details] Patch Attachment 94955 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8741031
Comment on attachment 94950 [details] Patch Attachment 94950 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8736226
Created attachment 94960 [details] Patch
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>	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 on attachment 94955 [details] Patch Attachment 94955 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/8736235
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
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
Created attachment 95089 [details] Patch
Bots are almost green! It's review ready now.
Created attachment 120164 [details] I'm back\!
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
Created attachment 120171 [details] Fixed test failure
(In reply to comment #22) > >> LayoutTests/fast/css/tab-size.html:19 > >> +<pre>	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: 	 (See http://www.whatwg.org/specs/web-apps/current-work/multipage/named-character-references.html)
Created attachment 120423 [details] Patch
> > 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.
Created attachment 121950 [details] smfr, hyatt: could you take another look?
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
Created attachment 121966 [details] Updated test expectaiotns - smfr, hyatt: could you take another look?
(In reply to comment #3) > Is this in a standard anywhere? Right here: http://www.w3.org/TR/css3-text/#tab-size0
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.
(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.
Created attachment 141087 [details] Patch for landing
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 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
Created attachment 141101 [details] Patch for landing
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
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
Created attachment 141140 [details] Patch for landing
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 on attachment 141140 [details] Patch for landing Attachment 141140 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12666114
Created attachment 141152 [details] Patch for landing
Comment on attachment 141152 [details] Patch for landing Attachment 141152 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12655881
Created attachment 141296 [details] Patch for landing
Comment on attachment 141296 [details] Patch for landing Clearing flags on attachment: 141296 Committed r116723: <http://trac.webkit.org/changeset/116723>
All reviewed patches have been landed. Closing bug.
tab-size should accept <length> values Blink(Chrome 42) support
Please file a new bug, rather than commenting in this old, closed one.