The default bug view has changed. See this FAQ.

Implement -moz-tab-size

VERIFIED FIXED in mozilla1.9.3a1

Status

()

Core
Layout: Text
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

({dev-doc-complete})

unspecified
mozilla1.9.3a1
dev-doc-complete
Points:
---
Bug Flags:
wanted1.9.2 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

8 years ago
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.
(Assignee)

Updated

8 years ago
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
(Assignee)

Comment 5

8 years ago
Created attachment 401996 [details] [diff] [review]
patch v2
Attachment #401843 - Attachment is obsolete: true
(Assignee)

Comment 6

8 years ago
(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
(Assignee)

Comment 7

8 years ago
Created attachment 402065 [details] [diff] [review]
patch v3

Mochitests passed
Reftests when I find time
Attachment #401996 - Attachment is obsolete: true
Attachment #402065 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #402065 - Flags: review? → review?(dbaron)
(Assignee)

Comment 8

8 years ago
Created attachment 402238 [details] [diff] [review]
patch v4

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)
Attachment #402238 - Flags: review?(roc)
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.
(Assignee)

Comment 12

8 years ago
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.
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+
(Assignee)

Comment 14

8 years ago
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.
(Assignee)

Comment 16

8 years ago
Created attachment 402722 [details] [diff] [review]
patch v6
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+
(Assignee)

Comment 18

8 years ago
Created attachment 402787 [details] [diff] [review]
patch v7
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
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Flags: wanted1.9.2?
Keywords: dev-doc-needed
Whiteboard: [doc-waiting-1.9.3]
For docs:

http://lists.w3.org/Archives/Public/www-style/2008Dec/0009.html
Flags: wanted1.9.2? → wanted1.9.2-
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).
Attachment #406373 - Flags: review?(dbaron)
Attachment #406373 - Flags: review?(dbaron) → review+
build-warning fix landed: http://hg.mozilla.org/mozilla-central/rev/cb01e7897aad

Updated

7 years ago
Duplicate of this bug: 430432
https://developer.mozilla.org/en/CSS/-moz-tab-size
Keywords: dev-doc-needed → dev-doc-complete
Whiteboard: [doc-waiting-1.9.3]
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.