Closed Bug 517882 Opened 11 years ago Closed 11 years ago

Implement -moz-tab-size

Categories

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

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9.3a1

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 6 obsolete files)

Attached patch patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → geoff
If you want this patch to be reviewed, try requesting review from dbaron.
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>
Hmm, you might need mTabSize to be PRInt32. As-is you can get overflow here:
+    text->mTabSize = textData.mTabSize.GetIntValue();
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
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #401843 - Attachment is obsolete: true
(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.
Status: NEW → ASSIGNED
Attached patch patch v3 (obsolete) — Splinter Review
Mochitests passed
Reftests when I find time
Attachment #401996 - Attachment is obsolete: true
Attachment #402065 - Flags: review?
Attachment #402065 - Flags: review? → review?(dbaron)
Attached patch patch v4 (obsolete) — Splinter Review
Now with reftests
Attachment #402065 - Attachment is obsolete: true
Attachment #402238 - Flags: review?(dbaron)
Attachment #402065 - Flags: review?(dbaron)
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)
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?
(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.
Attached patch patch v5 (obsolete) — Splinter Review
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.
Attachment #402238 - Attachment is obsolete: true
Attachment #402559 - Flags: review?(dbaron)
Attachment #402238 - Flags: review?(roc)
Attachment #402238 - Flags: review?(dbaron)
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>
Attachment #402559 - Flags: review+
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.
Attachment #402559 - Flags: review?(roc)
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.
Attached patch patch v6 (obsolete) — Splinter Review
Attachment #402559 - Attachment is obsolete: true
Attachment #402722 - Flags: review?(dbaron)
Attachment #402559 - Flags: review?(roc)
Attachment #402559 - Flags: review?(dbaron)
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.
Attachment #402722 - Flags: review?(dbaron) → review+
Attached patch patch v7Splinter Review
Attachment #402722 - Attachment is obsolete: true
Landed on mozilla-central:

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

Good work on the patch.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Flags: wanted1.9.2?
Keywords: dev-doc-needed
Whiteboard: [doc-waiting-1.9.3]
Flags: wanted1.9.2? → wanted1.9.2-
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).
Attachment #406373 - Flags: review?(dbaron)
Attachment #406373 - Flags: review?(dbaron) → review+
Duplicate of this bug: 430432
Verifying.
Status: RESOLVED → VERIFIED
Added it to the Firefox 4 for developers page as well.
You need to log in before you can comment on or make changes to this bug.