517882 - Implement -moz-tab-size

Status: VERIFIED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Geoff Lankow (:darktrojan)]

Attachment: 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

[Markus Stange [:mstange]]

If you want this patch to be reviewed, try requesting review from dbaron.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, you might need mTabSize to be PRInt32. As-is you can get overflow here:
+    text->mTabSize = textData.mTabSize.GetIntValue();

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[Geoff Lankow (:darktrojan)]

Attachment: patch v2

Comment 6

[Geoff Lankow (:darktrojan)]

(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

[Geoff Lankow (:darktrojan)]

Attachment: patch v3

Mochitests passed
Reftests when I find time

Comment 8

[Geoff Lankow (:darktrojan)]

Attachment: patch v4

Now with reftests

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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

[Geoff Lankow (:darktrojan)]

Attachment: 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

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

[Geoff Lankow (:darktrojan)]

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

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[Geoff Lankow (:darktrojan)]

Attachment: patch v6

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[Geoff Lankow (:darktrojan)]

Attachment: patch v7

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Landed on mozilla-central:

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

Good work on the patch.

Comment 20

[Christopher Blizzard (:blizzard)]

For docs:

http://lists.w3.org/Archives/Public/www-style/2008Dec/0009.html

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: 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

[Daniel Holbert [:dholbert]]

build-warning fix landed: http://hg.mozilla.org/mozilla-central/rev/cb01e7897aad

Comment 24

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://developer.mozilla.org/en/CSS/-moz-tab-size

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Verifying.

Comment 26

[Eric Shepherd [:sheppy]]

Added it to the Firefox 4 for developers page as well.