Last Comment Bug 517882 - Implement -moz-tab-size
: Implement -moz-tab-size
Status: VERIFIED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla1.9.3a1
Assigned To: Geoff Lankow (:darktrojan)
:
:
Mentors:
: 430432 (view as bug list)
Depends on:
Blocks: 295506
  Show dependency treegraph
 
Reported: 2009-09-21 06:59 PDT by Geoff Lankow (:darktrojan)
Modified: 2010-06-22 10:22 PDT (History)
12 users (show)
dbaron: wanted1.9.2-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (12.24 KB, patch)
2009-09-21 06:59 PDT, Geoff Lankow (:darktrojan)
no flags Details | Diff | Splinter Review
patch v2 (12.21 KB, patch)
2009-09-21 18:01 PDT, Geoff Lankow (:darktrojan)
no flags Details | Diff | Splinter Review
patch v3 (14.30 KB, patch)
2009-09-22 04:50 PDT, Geoff Lankow (:darktrojan)
no flags Details | Diff | Splinter Review
patch v4 (19.04 KB, patch)
2009-09-22 17:38 PDT, Geoff Lankow (:darktrojan)
no flags Details | Diff | Splinter Review
patch v5 (21.12 KB, patch)
2009-09-24 03:18 PDT, Geoff Lankow (:darktrojan)
roc: review+
Details | Diff | Splinter Review
patch v6 (22.91 KB, patch)
2009-09-24 17:24 PDT, Geoff Lankow (:darktrojan)
dbaron: review+
Details | Diff | Splinter Review
patch v7 (23.44 KB, patch)
2009-09-25 00:46 PDT, Geoff Lankow (:darktrojan)
no flags Details | Diff | Splinter Review
follow-up build-warning fix: reorder nsStyleText constructor's initializer list (878 bytes, patch)
2009-10-14 17:48 PDT, Daniel Holbert [:dholbert] (vacation, returning 2/27)
dbaron: review+
Details | Diff | Splinter Review

Description User image Geoff Lankow (:darktrojan) 2009-09-21 06:59:48 PDT
Created attachment 401843 [details] [diff] [review]
patch v1

Variable tab width for use in things such as view-source.

I really don't expect that this patch is up to scratch, but it does appear to work.
Comment 1 User image Markus Stange [:mstange] 2009-09-21 16:58:41 PDT
If you want this patch to be reviewed, try requesting review from dbaron.
Comment 2 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-21 17:07:48 PDT
Yes, dbaron will need to review this.

The patch looks pretty good to me!

+  } else if (eCSSUnit_Null == textData.mTabSize.GetUnit()) {

Shouldn't this be eCSSUnit_Initial?

We'll want at least one reftest here. Should be pretty easy, e.g.

<div style="white-space:pre; -moz-tab-size:4;">A&#9;X</div>
vs
<div style="white-space:pre;">A   X</div>
Comment 3 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-21 17:31:21 PDT
Hmm, you might need mTabSize to be PRInt32. As-is you can get overflow here:
+    text->mTabSize = textData.mTabSize.GetIntValue();
Comment 4 User image David Baron :dbaron: ⌚️UTC-8 2009-09-21 17:49:36 PDT
Before requesting review from me, you should make appropriate changes to layout/style/test/property_database.js to add the new property, and then make sure that the tests in that directory pass.  You can run them with something like:

cd $OBJDIR/_tests/testing/mochitest/
python ./runtests.py --test-path=layout/style --autorun

Also see https://developer.mozilla.org/en/Mozilla_automated_testing#Mochitest
Comment 5 User image Geoff Lankow (:darktrojan) 2009-09-21 18:01:50 PDT
Created attachment 401996 [details] [diff] [review]
patch v2
Comment 6 User image Geoff Lankow (:darktrojan) 2009-09-21 18:06:24 PDT
(In reply to comment #3)
> Hmm, you might need mTabSize to be PRInt32. As-is you can get overflow here:
> +    text->mTabSize = textData.mTabSize.GetIntValue();

Not quite sure what I was thinking when I wrote that. Monkey see, monkey do, I guess.

(In reply to comment #4)
> Before requesting review from me, you should make appropriate changes to
> layout/style/test/property_database.js to add the new property, and then make
> sure that the tests in that directory pass.  You can run them with something
> like:
> 
> cd $OBJDIR/_tests/testing/mochitest/
> python ./runtests.py --test-path=layout/style --autorun
> 
> Also see https://developer.mozilla.org/en/Mozilla_automated_testing#Mochitest

Yup, I'll get on to that.
Comment 7 User image Geoff Lankow (:darktrojan) 2009-09-22 04:50:59 PDT
Created attachment 402065 [details] [diff] [review]
patch v3

Mochitests passed
Reftests when I find time
Comment 8 User image Geoff Lankow (:darktrojan) 2009-09-22 17:38:11 PDT
Created attachment 402238 [details] [diff] [review]
patch v4

Now with reftests
Comment 9 User image David Baron :dbaron: ⌚️UTC-8 2009-09-23 22:53:52 PDT
nsCSSPropList.h:

+    CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE,

I think this should not apply to :first-letter and :first-line.  (I think
the closest analogous property is 'white-space'.)

nsComputedDOMStyle.cpp:

+  val->SetNumber (GetStyleText()->mTabSize);

No space between SetNumber and (.

>+    COMPUTED_STYLE_MAP_ENTRY_LAYOUT(_moz_tab_size,          MozTabSize),

Drop the "_LAYOUT"; that's for entries that require layout information
in the method implementation.

nsRuleNode.cpp:

+  switch (textData.mTabSize.GetUnit()) {
+    case eCSSUnit_Initial:
+      text->mTabSize = 8;
+      break;
+    case eCSSUnit_Inherit:
+      text->mTabSize = parentText->mTabSize;
+      break;
+    case eCSSUnit_Integer:
+      text->mTabSize = textData.mTabSize.GetIntValue();
+      break;
+  }

Instead of doing this, you should use SetDiscrete.  (SetDiscrete will
turn out useful when we implement additional generic values.)

nsTextFrameThebes.cpp:

>+  const nsStyleText* textStyle = aLineContainer->GetStyleText();

This makes it seem like you're not supporting tab-width on inline
elements, only on blocks.  Is that intended?  It's inconsistent with
'white-space', which seems like the most related property.

In order to support it there, I suspect you might need to do something
to ensure that we don't continue text runs across changes in the
'-moz-tab-size' property.  But roc would know for sure.

>+  return textStyle->mTabSize*spaceWidthAppUnits;

I think this would be more readable with space on either side of the *.

roc needs to review the changes to nsTextFrameThebes.cpp (and look at
the above comments)
Comment 10 User image David Baron :dbaron: ⌚️UTC-8 2009-09-23 22:58:24 PDT
Another question for roc:  does the result of the calculation that currently uses 8 for tab width end up in the text run cache, such that it could get re-used for another text run with different tab-width?
Comment 11 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-23 23:32:31 PDT
(In reply to comment #9)
> This makes it seem like you're not supporting tab-width on inline
> elements, only on blocks.  Is that intended?  It's inconsistent with
> 'white-space', which seems like the most related property.

Yes, it may as well work on inlines, it's just a matter of getting the tab size from the text frame's style context. Guess we should have a test for that. Otherwise looks good.

> In order to support it there, I suspect you might need to do something
> to ensure that we don't continue text runs across changes in the
> '-moz-tab-size' property.  But roc would know for sure.

You don't. Spacing information is not stored in the textrun.

(In reply to comment #10)
> Another question for roc:  does the result of the calculation that currently
> uses 8 for tab width end up in the text run cache, such that it could get
> re-used for another text run with different tab-width?

No.
Comment 12 User image Geoff Lankow (:darktrojan) 2009-09-24 03:18:14 PDT
Created attachment 402559 [details] [diff] [review]
patch v5

I'm not sure I understood everything that was said as most of this code is completely new to me, but I think I've covered it all.
Comment 13 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2009-09-24 03:25:56 PDT
Comment on attachment 402559 [details] [diff] [review]
patch v5

Good enough for me.

A reftest that tested -moz-tab-size on spans might be worth having too. E.g.
<pre><span style="-moz-tab-size:4">&#9;</span>X</pre>
Comment 14 User image Geoff Lankow (:darktrojan) 2009-09-24 03:28:00 PDT
Comment on attachment 402559 [details] [diff] [review]
patch v5

>diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp

>+  // tab-size: integer, inherit
>+  SetDiscrete(textData.mTabSize, text->mTabSize, canStoreInRuleTree,
>+              SETDSC_NORMAL | SETDSC_INTEGER, parentText->mTabSize,
>+              NS_STYLE_TABSIZE_NORMAL, 0, 0, 0, 0);

On second thoughts (now that I've gone and uploaded the patch... typical):

That 4th argument should be just SETDSC_INTEGER.
The constant would be better named NS_STYLE_TABSIZE_INITIAL.
Comment 15 User image David Baron :dbaron: ⌚️UTC-8 2009-09-24 12:25:34 PDT
Comment on attachment 402559 [details] [diff] [review]
patch v5

> // See nsStyleText
>+#define NS_STYLE_TABSIZE_NORMAL                 8

Yeah, I agree this should be INITIAL rather than NORMAL.

>+  // tab-size: integer, inherit
>+  SetDiscrete(textData.mTabSize, text->mTabSize, canStoreInRuleTree,
>+              SETDSC_NORMAL | SETDSC_INTEGER, parentText->mTabSize,
>+              NS_STYLE_TABSIZE_NORMAL, 0, 0, 0, 0);

You don't need the SETDSC_NORMAL here ; SETDSC_INTEGER alone should be sufficient.  SETDSC_NORMAL would be for a property that accepts 'normal' as a value.

Fix those two and add some reftests for -moz-tab-size on inlines, and this looks good.
Comment 16 User image Geoff Lankow (:darktrojan) 2009-09-24 17:24:37 PDT
Created attachment 402722 [details] [diff] [review]
patch v6
Comment 17 User image David Baron :dbaron: ⌚️UTC-8 2009-09-24 23:29:43 PDT
Comment on attachment 402722 [details] [diff] [review]
patch v6

I just noticed one other thing:

layout/reftests/reftest.list needs an "include tab-size/reftest.list", or the reftests you added won't get run.

r=dbaron with that fixed.

I'll probably edit that myself shortly and then push this to try server (https://wiki.mozilla.org/Build:TryServer) to make sure the tests are all fine.
Comment 18 User image Geoff Lankow (:darktrojan) 2009-09-25 00:46:54 PDT
Created attachment 402787 [details] [diff] [review]
patch v7
Comment 19 User image David Baron :dbaron: ⌚️UTC-8 2009-09-26 13:28:10 PDT
Landed on mozilla-central:

http://hg.mozilla.org/mozilla-central/rev/f4f374e783cc

Good work on the patch.
Comment 20 User image Christopher Blizzard (:blizzard) 2009-10-07 10:48:27 PDT
For docs:

http://lists.w3.org/Archives/Public/www-style/2008Dec/0009.html
Comment 21 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2009-10-14 17:48:31 PDT
Created attachment 406373 [details] [diff] [review]
follow-up build-warning fix: reorder nsStyleText constructor's initializer list

This checkin introduced this build warning (on Linux):

> nsStyleStruct.h:1076: warning: ‘nsStyleText::mTextShadow’ will be initialized after
> nsStyleStruct.h:1069: warning:   ‘PRInt32 nsStyleText::mTabSize’
> nsStyleStruct.cpp:2359: warning:   when initialized here

Attached follow-up patch fixes this by reordering the constructor's initialization list to put mTabSize between mWordWrap and mLetterSpacing (to match the order used in the nsStyleText class declaration).
Comment 22 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2009-10-14 19:43:13 PDT
build-warning fix landed: http://hg.mozilla.org/mozilla-central/rev/cb01e7897aad
Comment 23 User image Nickolay_Ponomarev 2010-02-02 06:06:50 PST
*** Bug 430432 has been marked as a duplicate of this bug. ***
Comment 24 User image :Ms2ger (⌚ UTC+1/+2) 2010-06-22 10:18:17 PDT
https://developer.mozilla.org/en/CSS/-moz-tab-size
Comment 25 User image :Ms2ger (⌚ UTC+1/+2) 2010-06-22 10:18:44 PDT
Verifying.
Comment 26 User image Eric Shepherd [:sheppy] 2010-06-22 10:22:09 PDT
Added it to the Firefox 4 for developers page as well.

Note You need to log in before you can comment on or make changes to this bug.