Closed Bug 1038663 Opened 10 years ago Closed 9 years ago

Allow percentage values for 'word-spacing'

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
firefox45 --- fixed

People

(Reporter: sebo, Assigned: n.nethercote)

References

(Blocks 2 open bugs,
URL
)

Details

(Keywords: dev-doc-complete)

Attachments

(8 files, 5 obsolete files)

(part 1) - Make nsStyleText::mWordSpacing an nsStyleCoord
7.06 KB, patch
heycam
: review+
n.nethercote
: checkin+
Details | Diff | Splinter Review
(part 2) - Move GetFirstFontMetrics() up
2.18 KB, patch
heycam
: review+
Details | Diff | Splinter Review
(part 3) - Factor out space width computation
2.29 KB, patch
heycam
: review+
Details | Diff | Splinter Review
(part 4) - Inline StyleToCoord()
2.27 KB, patch
heycam
: review+
Details | Diff | Splinter Review
(part 5) - Change GetSpacingFlags()
3.39 KB, patch
n.nethercote
: review+
Details | Diff | Splinter Review
(part 6) - Allow percentage values for 'word-spacing'
9.83 KB, patch
heycam
: review+
Details | Diff | Splinter Review
(part 7) - Add test for percentage values for 'word-spacing'
3.51 KB, patch
heycam
: review+
Details | Diff | Splinter Review
(part 7, attempt 3) - Add test for percentage values for 'word-spacing'
8.22 KB, patch
heycam
: review+
Details | Diff | Splinter Review 
As of the current draft of the CSS Text Module Level 3 the 'word-spacing' property allows percentual values.

This should also be marked as blocker for bug 913153.

Sebastian
Blocks: css3test
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → n.nethercote
css-text-3 says that word-spacing always computes to an absolute length, but I don't think that makes sense for percentage values.  I mailed www-style about it:

https://lists.w3.org/Archives/Public/www-style/2015Oct/0196.html
This makes mWordSpacing a lot more like mLetterSpacing, while maintaining the
existing "'normal' computes to 0px" behaviour.
Attachment #8677241 - Flags: review?(cam)
Comment on attachment 8677241 [details] [diff] [review]
(part 1) - Make nsStyleText::mWordSpacing an nsStyleCoord

Review of attachment 8677241 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsStyleStruct.h
@@ +1686,5 @@
>    uint8_t mControlCharacterVisibility;  // [inherited] see nsStyleConsts.h
>    int32_t mTabSize;                     // [inherited] see nsStyleConsts.h
>  
> +  nsStyleCoord mWordSpacing;            // [inherited] coord, normal
> +  nsStyleCoord mLetterSpacing;          // [inherited] coord, normal

Remove these "normal"s; we should just mention the type of values that can be stored in the nsStyleCoord.
Attachment #8677241 - Flags: review?(cam) → review+
> > +  nsStyleCoord mWordSpacing;            // [inherited] coord, normal
> > +  nsStyleCoord mLetterSpacing;          // [inherited] coord, normal
> 
> Remove these "normal"s; we should just mention the type of values that can
> be stored in the nsStyleCoord.

mLetterSpacing can still be normal, so I'll leave that one.
To implement percentage units for word-spacing, I need to be able to get the width of a space character and then multiply that width by the percentage. jtd, is there a way to get that width in either WordSpacing() (earlier) or in PropertyProvider::GetSpacingInternal() (later)?

(Actually, for full CSS3 compliance we probably want to get it in GetSpacingInternal() because we need to handle unusual word-breaking chars like the Aegean Word Separator, which is not in the BMP and so I'm not sure how to detect it within IsCSSWordSpacingSpace() given that nsTExtFragment is UCS-2...)
Flags: needinfo?(jdaggett)
Keywords: leave-open
Attachment #8677241 - Flags: checkin+
No response from jtd after a week. jkew, do you know the answer to the question in comment 5?
Flags: needinfo?(jfkthame)
(In reply to Nicholas Nethercote [:njn] from comment #5)
> To implement percentage units for word-spacing, I need to be able to get the
> width of a space character and then multiply that width by the percentage.
> jtd, is there a way to get that width in either WordSpacing() (earlier) or
> in PropertyProvider::GetSpacingInternal() (later)?

Within GetSpacingInternal(), it shouldn't be a problem.... IIRC, the PropertyProvider has a reference to the textrun, and from the textrun you can get a fontmetrics and thence the width of a space. Compare the ComputeTabWidthAppUnits() helper in nsTextFrame.cpp.

If you need widths of other word-separating characters, you'll need to do a bit more work (space itself is easy because it's cached directly in the font metrics). Look at GetHyphenWidth(), which calls into the fontGroup to make a temporary textrun -- which handles finding the appropriate font, given that the character might not be supported by the first font in the list, so fallback may be needed -- and then measures the result; this could be generalized to take a desired character as a parameter, I guess.

(If you prefer to have the width early, in WordSpacing(), I think you'd want to add the textrun as a parameter there, and then all the above applies just the same.)

> (Actually, for full CSS3 compliance we probably want to get it in
> GetSpacingInternal() because we need to handle unusual word-breaking chars
> like the Aegean Word Separator, which is not in the BMP and so I'm not sure
> how to detect it within IsCSSWordSpacingSpace() given that nsTExtFragment is
> UCS-2...)

(To be more accurate, it's UTF-16, not UCS-2.)

To detect that, first change |ch| from a |char16_t| to |uint32_t|. Then, before the existing |switch (ch)|, you can do something like

  if (NS_IS_HIGH_SURROGATE(ch) &&
      aPos + 1 < aFrag->GetLength() &&
      NS_IS_LOW_SURROGATE(aFrag->CharAt(aPos + 1))) {
    ch = SURROGATE_TO_UCS4(ch, aFrag->CharAt(aPos + 1));
  }

so that the switch will see the full Unicode character code.
Flags: needinfo?(jfkthame)
Thank you for the detailed answer! Very helpful.
Flags: needinfo?(jdaggett)
This is necessary for a subsequent patch.
Attachment #8683411 - Flags: review?(cam)
This will be reused in a subsequent patch.
Attachment #8683412 - Flags: review?(cam)
This doesn't make much sense by itself, but one of the call sites will be
changed in a subsequent patch.
Attachment #8683413 - Flags: review?(cam)
Again, this doesn't make sense in isolation, but it will make it easier to
allow for percentage word-spacing in a subsequent patch.
Attachment #8683414 - Flags: review?(cam)
I did my best to follow the test format rules at
https://wiki.csswg.org/test/format, but I may well have gotten something wrong.
Attachment #8683417 - Flags: review?(cam)
Attachment #8683411 - Flags: review?(cam) → review+
Attachment #8683412 - Flags: review?(cam) → review+
Comment on attachment 8683413 [details] [diff] [review]
(part 4) - Inline StyleToCoord()

Review of attachment 8683413 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsTextFrame.cpp
@@ +1620,5 @@
>    if (!aStyleText) {
>      aStyleText = aFrame->StyleText();
>    }
> +
> +  nsStyleCoord coord = aStyleText->mLetterSpacing;

Make this a |const nsStyleCoord&| so we don't have to make a copy of it.

@@ +1637,5 @@
>    if (!aStyleText) {
>      aStyleText = aFrame->StyleText();
>    }
> +
> +  nsStyleCoord coord = aStyleText->mWordSpacing;

And here.
Attachment #8683413 - Flags: review?(cam) → review+
Comment on attachment 8683414 [details] [diff] [review]
(part 5) - Change GetSpacingFlags()

Review of attachment 8683414 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with these things fixed.

::: layout/generic/nsTextFrame.cpp
@@ +1647,5 @@
>  
> +// Returns gfxTextRunFactory::TEXT_ENABLE_SPACING if non-standard
> +// letter-spacing or word-spacing is present.
> +static uint32_t
> +GetSpacingFlags(nsIFrame* aFrame, const nsStyleText* aStyleText = nullptr)

aStyleText argument is not needed.

@@ +1660,5 @@
> +  nsStyleCoord coord = aStyleText->mLetterSpacing;
> +  if (eStyleUnit_Coord == coord.GetUnit()) {
> +    return coord.GetCoordValue() != 0
> +         ? gfxTextRunFactory::TEXT_ENABLE_SPACING
> +         : 0;

I don't think we want to return 0 here if mLetterSpacing is 0; we need to continue to check mWordSpacing.
Attachment #8683414 - Flags: review?(cam) → review+
Comment on attachment 8683416 [details] [diff] [review]
(part 6) - Allow percentage values for 'word-spacing'

Review of attachment 8683416 [details] [diff] [review]:
-----------------------------------------------------------------

I think we want to allow values like calc(50% + 2px).  There are a couple of changes needed for this:

::: layout/generic/nsTextFrame.cpp
@@ +1646,5 @@
>      return coord.GetCoordValue();
>    }
> +  if (eStyleUnit_Percent == coord.GetUnit()) {
> +    return GetSpaceWidthAppUnits(aTextRun) * coord.GetPercentValue();
> +  }

You will need to handle eStyleUnit_Calc values in here.  You can use nsRuleNode::ComputeCoordPercentCalc to handle eStyleUnit_Percent and eStyleUnit_Calc simultaneously.  (You could also use it for plain eStyleUnit_Coord values, but it might be worth skipping the GetSpaceWidthAppUnits() call for that case.)

::: layout/style/nsCSSPropList.h
@@ +3602,5 @@
>      WordSpacing,
>      CSS_PROPERTY_PARSE_VALUE |
>          CSS_PROPERTY_APPLIES_TO_FIRST_LETTER_AND_FIRST_LINE |
>          CSS_PROPERTY_APPLIES_TO_PLACEHOLDER |
>          CSS_PROPERTY_UNITLESS_LENGTH_QUIRK,

Add CSS_PROPERTY_STORES_CALC here, so that animations between calc() values that have percentages in them will work.

::: layout/style/nsRuleNode.cpp
@@ +4363,1 @@
>                 SETCOORD_CALC_LENGTH_ONLY | SETCOORD_UNSET_INHERIT,

Add SETCOORD_STORE_CALC so that SetCoord stores calc values in the nsStyleCoord, rather than only accept lengths in calc() and resolve them down to an nscoord.

::: layout/style/nsStyleStruct.h
@@ +1685,5 @@
>    uint8_t mTextCombineUpright;          // [inherited] see nsStyleConsts.h
>    uint8_t mControlCharacterVisibility;  // [inherited] see nsStyleConsts.h
>    int32_t mTabSize;                     // [inherited] see nsStyleConsts.h
>  
> +  nsStyleCoord mWordSpacing;            // [inherited] coord, percent

Mention calc in the comment.

::: layout/style/test/property_database.js
@@ +3722,5 @@
>      type: CSS_TYPE_LONGHAND,
>      initial_values: [ "normal", "0", "0px", "-0em",
>        "calc(-0px)", "calc(0em)"
>      ],
> +    other_values: [ "1em", "2px", "-3px", "0%", "50%",

Add a negative percentage value as well as some calc() values with percentages in them.
Attachment #8683416 - Flags: review?(cam)
Comment on attachment 8683417 [details] [diff] [review]
(part 7) - Add test for percentage values for 'word-spacing'

Review of attachment 8683417 [details] [diff] [review]:
-----------------------------------------------------------------

I think you might have to follow what's been done in the submitted/variables/ and submitted/flexbox/ tests, and that is to make a copy of Ahem.ttf, a small style sheet to reference it, stick them in a support/ subdirectory, then add a <link rel="stylesheet" href="support/ahem.css" type="text/css"> to the test file.  This isn't ideal, but I don't think the test harness has the ability to ensure an Ahem font is available by default (as the CSSWG test suite assumes).

::: layout/reftests/w3c-css/submitted/text3/text-word-spacing-001.html
@@ +1,3 @@
> +<!DOCTYPE html>
> +<title>CSS Text Test: Word Spacing</title>
> +<link rel="author" title="Nicholas Nethercote" href="nnethercote@mozilla.com">

mailto: before your email address.

@@ +9,5 @@
> +div.wsn40  { word-spacing:  -40%; }
> +div.ws0    { word-spacing:    0%; }
> +div.ws25   { word-spacing:   25%; }
> +div.ws100  { word-spacing:  100%; }
> +div.ws300  { word-spacing:  300%; }

Please add some sub-tests for calc() values with percentages in them.

@@ +19,5 @@
> +  <div class="ws0"   >A Bc Def Ghij</div>
> +  <div class="ws100" >A Bc Def Ghij</div>
> +  <div class="wsn40"> A &nbsp;&nbsp;&nbsp;&nbsp;Bc &nbsp;&nbsp;&nbsp;&nbsp;Def &nbsp;&nbsp;&nbsp;&nbsp;Ghij</div>
> +  <div class="ws300" >A Bc Def Ghij</div>
> +  <div class="ws25"  >A &nbsp;&nbsp;&nbsp;Bc &nbsp;&nbsp;&nbsp;Def &nbsp;&nbsp;&nbsp;Ghij</div>

These tests look great!
Attachment #8683417 - Flags: review?(cam)
> I think you might have to follow what's been done in the
> submitted/variables/ and submitted/flexbox/ tests, and that is to make a
> copy of Ahem.ttf, a small style sheet to reference it, stick them in a
> support/ subdirectory, then add a <link rel="stylesheet"
> href="support/ahem.css" type="text/css"> to the test file.  This isn't
> ideal, but I don't think the test harness has the ability to ensure an Ahem
> font is available by default (as the CSSWG test suite assumes).

Even though I mentioned Ahem in the flags I ended up not using it, so I think I can skip all this (and update the flags).
I fixed the problems with GetSpacingFlags(). Carrying over r+.
Attachment #8683414 - Attachment is obsolete: true
Comment on attachment 8683934 [details] [diff] [review]
(part 5) - Change GetSpacingFlags()

>+  nsStyleCoord ls = styleText->mLetterSpacing;
>+  nsStyleCoord ws = styleText->mWordSpacing;

Make these |const nsStyleCoord&|.
Attachment #8683934 - Flags: review+
Attachment #8683416 - Attachment is obsolete: true
Attachment #8683417 - Attachment is obsolete: true
Comment on attachment 8683940 [details] [diff] [review]
(part 6) - Allow percentage values for 'word-spacing'

Review of attachment 8683940 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with these things addressed.

::: layout/generic/nsTextFrame.cpp
@@ +1665,5 @@
>    const nsStyleText* styleText = aFrame->StyleText();
>    const nsStyleCoord& ls = styleText->mLetterSpacing;
>    const nsStyleCoord& ws = styleText->mWordSpacing;
>  
>    bool nonStandardSpacing =

Since calc() values with non-zero percentages and lengths still can end up resolving to 0, this makes our use of TEXT_ENABLE_SPACING less exact.  TEXT_ENABLE_SPACING is just used as an optimisation to skip unnecessary work, so it's not incorrect to return it, but I'm not sure where the exact tradeoff here is.  It's probably OK as you have it.

Put a comment in here to point out that we can still return TEXT_ENABLE_SPACING even if a calc() value resolves to 0, but that precise use of TEXT_ENABLE_SPACING is only used to skip work unnecessary work in gfxTextRun, so it doesn't matter too much given that it should be an uncommon case.

::: layout/style/nsStyleCoord.h
@@ +90,5 @@
>      bool operator!=(const CalcValue& aOther) const {
>        return !(*this == aOther);
>      }
> +
> +    bool IsZero() const {

Consider renaming this to something that suggests it only looks at the component percentage/length values, and does not guarantee that the calc() value will resolve to 0.
Attachment #8683940 - Flags: review?(cam) → review+
Comment on attachment 8683941 [details] [diff] [review]
(part 7) - Add test for percentage values for 'word-spacing'

Review of attachment 8683941 [details] [diff] [review]:
-----------------------------------------------------------------

Can you just add one more test with a calc() value that has a length that isn't stored as 0px.  (You could use a <span> in the reference image with a padding-left value of that length to shift the second word over.)
Attachment #8683941 - Flags: review?(cam) → review+
Woo! heycam: thank you for all the assistance.
Comment on attachment 8683940 [details] [diff] [review]
(part 6) - Allow percentage values for 'word-spacing'

>   const nsStyleCoord& coord = aStyleText->mWordSpacing;
>   if (eStyleUnit_Coord == coord.GetUnit()) {
>     return coord.GetCoordValue();
>   }
>+  if (eStyleUnit_Percent == coord.GetUnit() ||
>+      eStyleUnit_Calc == coord.GetUnit()) {
>+    return nsRuleNode::ComputeCoordPercentCalc(coord,
>+                                               GetSpaceWidthAppUnits(aTextRun));
>+  }
>   return 0;
> }

I'm not sure how expensive GetSpaceWidthAppUnits is, but it's possible to avoid it in slightly more cases by using nsStyleCoord::HasPercent or nsStyleCoord::ConvertsToLength, e.g., with something like:

  const nsStyleCoord& coord = aStyleText->mWordSpacing;
  if (coord.IsCoordPercentCalcUnit()) {
    nscoord pctBasis = coord.HasPercent() ? GetSpaceWidthAppUnits(aTextRun) : 0;
    return nsRuleNode::ComputeCoordPercentCalc(coord, pctBasis);
  }
  return 0;
Keywords: leave-open
Flags: needinfo?(n.nethercote)
Switching the test to use a monospace font might help.
Using a monospace font fixed the Android failure. I also made the change that dbaron suggested in comment 30 before re-landing.
https://hg.mozilla.org/integration/mozilla-inbound/rev/797d3b54d0db543988f97f42e1a819e35179d738
Backed out changeset f8fa4680e732 (bug 1038663) for OS X reftest bustage on a CLOSED TREE
(In reply to Nicholas Nethercote [:njn] from comment #35)
> Using a monospace font fixed the Android failure.

... but it introduced a similar failure on OS X:

http://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-macosx64/1447030295/mozilla-inbound_snowleopard_test-reftest-bm107-tests1-macosx-build3482.txt.gz&only_show_unexpected=1

At this point I think I'll go back to a non-monospace font and use some fuzzy matching on Android.
Can you instead restructure the test so that it forms the same text run separation in both test and reference?  (That seems likely to be the problem; there are probably sub-pixel positioning differences as a result.)

Alternatively, use the Ahem font, with a font size that's a multiple of 5 pixels.

I think one of those would be better than having to adjust fuzz as platforms change over time.
Flags: needinfo?(n.nethercote)
(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #38)
> Can you instead restructure the test so that it forms the same text run
> separation in both test and reference?

Sorry, I don't understand this suggestion.


> Alternatively, use the Ahem font, with a font size that's a multiple of 5
> pixels.

A multiple of 5 so that -40% is a round number?
(In reply to Nicholas Nethercote [:njn] from comment #40)
> (In reply to David Baron [:dbaron] ⌚UTC+8 from comment #38)
> > Can you instead restructure the test so that it forms the same text run
> > separation in both test and reference?
> 
> Sorry, I don't understand this suggestion.

On second thoughts, I'm not even sure it makes sense.  I could imagine extra word spaces implying a break in shaping, or not doing so.  I was thinking they didn't -- but if they didn't, then the test wouldn't be failing because of that.

It's probably failing because of assumptions about rounding, which means using Ahem is probably the right way to go.

> > Alternatively, use the Ahem font, with a font size that's a multiple of 5
> > pixels.
> 
> A multiple of 5 so that -40% is a round number?

That's good too, but I was actually saying that because things (e.g., descent is 20%, ascent is 80%) that all are integral numbers of pixels when the size is a multiple of 5px.

But yes, you probably do want your word-spacing percentages to come out reasonably round, ideally integral pixels.
Now using Ahem. Works locally, wish me luck for the try run I'm about to do...
Attachment #8685091 - Flags: review?(cam)
Attachment #8684755 - Attachment is obsolete: true
Attachment #8684755 - Flags: review?(cam)
Oh, I forgot to make the font size a multiple of 5 pixels.
Attachment #8685105 - Flags: review?(cam)
Attachment #8685091 - Attachment is obsolete: true
Attachment #8685091 - Flags: review?(cam)
Attachment #8685105 - Flags: review?(cam) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b84d7638278ac102cb20514fe926a3a66a6c6f7
Bug 1038663 (part 7, attempt 3) - Add test for percentage values for 'word-spacing'. r=heycam.
Status: RESOLVED → VERIFIED
Thank you for the documentation, Sebastian.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a63a0aefa179381c388fe0ad42c615e5855a87d0
Bug 1038663 followup - Add the link rel=match that makes the CSS WG harness believe this test a reftest.  No review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/18fb0664e59187d2a6b22384d484c8c54655262f
Bug 1038663 followup - Remove specification link from reftest reference, which the CSS WG harness considers an error.  No review.
https://hg.mozilla.org/integration/mozilla-inbound/rev/26bf115402e27a4f24de8a040f8b36ac8e7d8ce4
Bug 1038663 followup - Remove assertion from reftest reference, which the CSS WG harness considers an error.  No review.
Thank you for the test fixes, dbaron. I did my best to follow the format but I'm unfamiliar with it and so I'm not surprised there were minor problems.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: