Closed Bug 164700 Opened 22 years ago Closed 18 years ago

css word-spacing applies at   at paint time but not at layout (reflow) time

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.9alpha1

People

(Reporter: rolf.offermanns, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

(Keywords: testcase, Whiteboard: [Pending check-in to 1.8 branch until bug 329987 is closed])

Attachments

(7 files, 18 obsolete files)

343 bytes, text/html
Details
965 bytes, text/html
Details
491 bytes, text/html
Details
509 bytes, text/html
Details
2.81 KB, text/html
Details
9.16 KB, patch
rbs
: review+
Details | Diff | Splinter Review
9.21 KB, patch
masayuki
: review+
masayuki
: superreview+
Details | Diff | Splinter Review
Please have a look at the attachment.
Everything is fine if you delete the leading    inside the div.
Attached file simple testcase
I see a few problems.  If you see any problems with what I wrote

One css compliance problem is that the  s should not be larger than normal
spaces: the spec defines additional space _between words_, not wider spaces. 
For example, if word-spacing were set to the width of a space, the snippet
"I  am" should render, using ASCII art, as:
I   am,(width three spaces) not:
I    am. (width four spaces)

The other problem is the rendering bug (Entry4 overlapping Entry3), which I
_think_ is caused by the additional space from the  s being treated oddly.
 This can be worked around by using indentation instead of  s.

Note that my build is somewhat old, and this may have been fixed in recent
nightlies.
Keywords: qawanted
QA Contact: petersen → ian
Attached file complete testcase
more complete testcase showing multiple permutations of style applications
Looking over this bug I'm wondering if this is 2 different bugs - one for the
additional space the nbsps are getting when word-spacing is applied - and one
for the apparant break the SPAN tag is causing... otherwise this should be
confirmed as I'm seeing it on 1.3b/OS X as well.
Keywords: testcase
OS: Linux → All
Hardware: PC → All
still seeing this ... marking new ... symptoms are similar to that of bug 198664
 but I can't relate the cause
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Target Milestone: --- → Future
.
Assignee: attinasi → font
Component: Layout → Layout: Fonts and Text
Priority: P3 → --
Target Milestone: Future → ---
Priority: -- → P3
Target Milestone: --- → Future
Attached patch patch (obsolete) — Splinter Review
A patch adds word spacing of html NBSP entity for the word fragment.
Attachment #132633 - Flags: review?(rbs)
Care to attach screnshots for before vs. after? (it takes a lot of my time to
review those text & font bugs, and looking at a screenshot helps me & others to
see whether the patch does what is expected and if it is worth the trouble of
iterating on the internals of the patch).
Attached patch patch (obsolete) — Splinter Review
The patch was updated becasue I found a new error for this bug. The dimension
of a nbsp entity differs from the dimension of a white space on my machine
(Debian GNU/Linux/PowerPC/X11). The display of mozilla was done on Win98SE
using X-Server.
Attachment #132633 - Attachment is obsolete: true
Attached image ScreenShot (obsolete) —
Attached file testcase2
Attachment #132715 - Flags: review?(rbs)
So you are considering x y as three words, "x", " ", "y". And then doing:

x [word-spacing]   [word-spacing] y

Which section of the spec says so? (I have mentioned several times that you are
better off documenting/expressing what you are trying to do...)

    // Before we get measuring, reconvert nbsp entities to white spaces.
--->RevertNBSPToSpaces(bp, wordLen);
[...]
    TextStyle ts(aLineLayout.mPresContext, rc, sc);
    if (ts.mSmallCaps) {
      MeasureSmallCapsText(aReflowState, ts, bp, wordLen, &dimensions);
    }
    else {
      rc.GetTextDimensions(bp, wordLen, dimensions);
      // NOTE: Don't forget to add letter spacing for the word fragment!
      dimensions.width += wordLen*ts.mLetterSpacing;
      // NOTE: add word spacing of the non-breaking space (html nbsp entity)!
----->dimensions.width += aNBSPLen*ts.mWordSpacing;
    }

|aNBSPLen| (which is best called |countNBSP|) is related to something else
(higher up). It is out of sync with the most recent |RevertNBSPToSpaces| since
it is not filled in as argument.
Attachment #132633 - Flags: review?(rbs)
Comment on attachment 132715 [details] [diff] [review]
patch

The patch does as following, because a word-sapceing is added for every
white-space in the processing of the rendering of the string.

for x y, x   [word-spacing] y
for x  y, x   [word-spacing]   [word-spacing] y
for x[white-space] y, x [white-space] [word-spacing]   [word-spacing]
y

The patch is wrong because only one word-sapceing should be added to the nbsp
entities.
Attachment #132715 - Flags: review?(rbs)
> The patch does as following, because a word-sapceing is added for every
> white-space in the processing of the rendering of the string.

This one is OK/understandable. It is because a white-space is rightly a
_separator_ for words. For example, "x y" does mean "x", "white-space", "y".

> The patch is wrong because only one word-sapceing should be added to the nbsp
> entities.

I wonder if this is is what is supposed to happen. That's why I asked you to
point at the spec. I am under the impression that "x y" is a single word.
Attached patch patch (obsolete) — Splinter Review
References are http://www.w3.org/TR/REC-CSS2/text.html and
	       http://www.w3.org/TR/REC-html40/struct/text.html

-------------------------------------------------
'word-spacing'
    Value:	normal | <length> | inherit

<length>
    This value indicates inter-word space in addition to the default space
between words. Values may be negative, but there may be implementation-specific
limits. 

Word spacing algorithms are user agent-dependent. Word spacing is also
influenced by justification.
-------------------------------------------------
Prohibiting a line break 

Sometimes authors may want to prevent a line break from occurring between two
words. The &nbsp; entity (&#160; or &#xA0;) acts as a space where user agents
should not cause a line break.
-------------------------------------------------

> I wonder if this is is what is supposed to happen. That's why I asked you to
> point at the spec. I am under the impression that "x&nbsp;y" is a single
word.

I think that "x&nbsp;y" is two words since a nbsp entity is same as a space.

Although a word-sapceing is added for every white-space since nbsp entities
were converted to white-spaces of the same number, it is not necessary that
only one word-sapceing is added.
Attachment #132715 - Attachment is obsolete: true
Attachment #133426 - Flags: review?(rbs)
*** Bug 202610 has been marked as a duplicate of this bug. ***
Summary: css word-spacing problem with &nbsp; → css word-spacing applies at &nbsp; at paint time but not at layout (reflow) time
I think this is the same bug, but I'm not sure.
Attachment #133426 - Flags: review?(rbs)
Attached patch test patch (obsolete) — Splinter Review
This patch came from posted patch (attachment 179051 [details] [diff] [review]) to Bug 96423.
Attachment #133426 - Attachment is obsolete: true
Attached image ScreenShot for test patch (obsolete) —
Attached patch patch (obsolete) — Splinter Review
In changes, in order to search the position of the content from the transformed
text position, two arguments are passed using the indexed buffer class,
 1. the pointer to a current instance of the text transformer and 
 2. the start position of the transformed text.
A word-spacing is added only when a text is a whitespace but not NBSP.
Attachment #179351 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
This patch needs to apply after the changes(attachment 182365 [details] [diff] [review]) for Bug 291321.

As for the additional functions, 
 1. |PassTo| passes two parameters using |nsAutoIndexBuffer| object,
   an address to a |nsTextTransformer| object and
   a start index of transformed texts.
 2. |GetContentCharAt| searches a content index from a transformed text index.

The large part of the changes is in order to pass the address to
|nsAutoIndexBuffer| object with two parameters.
Attachment #181122 - Attachment is obsolete: true
Attachment #182869 - Flags: review?(rbs)
Blocks: 290536
Comment on attachment 182869 [details] [diff] [review]
patch

I perceive that the whole point of your added GetContentCharAt() is to enable
you to detect if ch is NBSP_CH or not. (you have to tell your key reasons, as I
remind you often).

If that's the point of GetContentCharAt, then it is not necessary at all. Any
space that you hit there is NBSP -- otherwise you wouldn't see it, by design.
(If there were normal ' ', they would have been handled elsewhere.)

This means that your patch can be much simpler. I am under the impression that
the net-effect of all your are doing is adding a word-spacing if prev_ch!=nbsp
and ch==nbsp.

BTW, do you fix the "complete testcase", and bug 290536?
Note that it's not clear to me that bug 290536 is a bug...
> I perceive that the whole point of your added GetContentCharAt() is to enable

> you to detect if ch is NBSP_CH or not. (you have to tell your key reasons, as
I
> remind you often).

Added GetContentCharAt() is used to search a content index from a transformed
text index.

> If that's the point of GetContentCharAt, then it is not necessary at all. Any

> space that you hit there is NBSP -- otherwise you wouldn't see it, by design.

> (If there were normal ' ', they would have been handled elsewhere.)

I think that a word-spacing is added only when a space character in the
transformed text is normal space but not NBSP. I cannot understand your
following indication "it is not necessary at all...".

> This means that your patch can be much simpler. I am under the impression
that
> the net-effect of all your are doing is adding a word-spacing if
prev_ch!=nbsp
> and ch==nbsp.

Is the word-spacing added when a space character in the transformed text is
NBSP?
I desire a new bug to be opened after this bug is fixed.

> BTW, do you fix the "complete testcase", and bug 290536?

I posted a additional patch for justification. It seems that the testcase of
bug 290536 is displayed successfully.
Attached image screen shot for additional patch (obsolete) —
> BTW, do you fix the "complete testcase", and bug 290536?

I posted a screen shot for these testcase.
>Added GetContentCharAt() is used to search a content index from a transformed
>text index.

I can see 'what' it is doing (or attempting to do). It is often the 'why' that
you tend to leave out.

> I think that a word-spacing is added only when a space character in the
> transformed text is normal space but not NBSP. I cannot understand your
> following indication "it is not necessary at all...".

OK, I have seen that the NBSPs are lost are the call-sites where you are, so
that you need to recover them (but I think another approach is possible, see below).

I am trying to understand how what your are doing relates to what you said in
comment 13 and comment 14. You are not putting any word-spacing at nbsp anymore,
right? Not even a single word-spacing when there are several nbsp?

Issues with your patch: it puts the transformer in the index buffer, and does
some unwieldy genuflexions. (As more patches like that go in, things risk to
interwine eve further, making the code harder to maintain down the track.)

A simpler approach might be to preserve NBSP in PrepareUnicodeText(). Add
PrepareUnicodeText(..., aPreserveNBSP=FALSE), and call with aPreserveNBSP=TRUE
when needed (i.e, on call sites that are on the slow rendering/measuring path).

===============
additional patch for justification (attachment 183801 [details] [diff] [review]):

This patch seems to be equivalent to removing NBSP (0xa0u) from
nsTextFrame::IsJustifiableCharacter(). What is needed is to bypass NBSP only at
the beginning of the line. As this is getting involved, I suggest to leave this
issue in that separate bug.
Attached patch simpler fix (obsolete) — Splinter Review
This patch illustrates the other approach I alluded to. 

[Note: it treats "x&nbsp;" as a single word, and does not try anything fancy
there. But since the &nbsp; are readily accessible now, those fancy things can
be done if need be, while still keeping the code intelligible.]
I am seeing that Opera and IE apply different word-spacing in the face of nbsp:

IE: 
  "x&nbsp;&nbsp;y" is rendered using a gap of |countNBSP * word-spacing|:
   x&nbsp;[word-spacing]&nbsp;[word-spacing]y

  "x&nbsp;&nbsp; y" is rendered using a gap of |(countNBSP+1) * word-spacing|:
   x&nbsp;[word-spacing]&nbsp;[word-spacing] [word-spacing]y

Opera:
  "x&nbsp;&nbsp;y" is treated as a single word and doesn't get any word-spacing:
   x&nbsp;&nbsp;y

  "x&nbsp;&nbsp; y" is treated as two words and so is given one word-spacing:
   x&nbsp;&nbsp; [word-spacing]y

---------
Which way do people expect the most here?
I think Opera's result is right.

See http://www.w3.org/TR/css3-text/#word-spacing-prop
> If there are several word-separating characters (for example, multiple non
collapsed white space characters), the added <length> can only be applied once.
Oops...
the 0pera's case of "x&nbsp;&nbsp;y" is incorrect.

I think that follwing result is correct.

"x&nbsp;&nbsp;y"
x&nbsp;&nbsp;[word-spacing]y

"x&nbsp;&nbsp; y"
x&nbsp;&nbsp; [word-spacing]y
"If there are several word-separating characters (for example, multiple non
collapsed white space characters), the added <length> can only be applied once."

That quote that you curled from CSS3 Text clarifies the mess.

It also means that nsTextFrame needs to be brought in sync. See, e.g., what
MeasureText does:

    if (isWhitespace) {
      [...]
      if ('\t' == firstChar) {
        // Expand tabs to the proper width
        wordLen = 8 - (7 & column);
        // Apply word spacing to every space derived from a tab
                                 ^^^^^^^^^^^
        dimensions.width = (aTs.mSpaceWidth + aTs.mWordSpacing +
aTs.mLetterSpacing)*wordLen;
        [...]
      }
      [...]
      else {
        // Apply word spacing to every space, if there's more than one
                                 ^^^^^^^^^^^
        dimensions.width = wordLen*(aTs.mWordSpacing + aTs.mLetterSpacing +
aTs.mSpaceWidth);// XXX simplistic
      }
It is |letter-spacing| that needs to retain its existing behavior. The spec says
that it should be added to every space that takes space.

http://www.w3.org/TR/css3-text/#letter-spacing-prop
<length>
    This value indicates spacing added between grapheme clusters (*) in addition
to the default spacing between grapheme clusters. The spacing is not be added to
grapheme clusters that have a zero advance width but is added to all non
collapsed white space characters.

(*) A grapheme cluster is what a language user consider to be a character or a
basic unit of the language. [...] A white space character is a grapheme cluster.
I think nsTextFrmae has many issues for letter-spacing and word-spacing.

For word-spacing:
If we complete to implement word-spacing, we need to get next frame's first
character. But we cannot get the character now.
I think we need new frame chain system for nsTextFrame for this bug, bug 288439,
bug 289130 and bug 156369.
I'm thinking that how to implement this system...

For letter-spacing:
We need to implement grapheme cluster iterator. see bug 229896.
Attached patch css3 patch (obsolete) — Splinter Review
Doing word spacing per CSS3 has meant that distinguishing between ' ' and
&nbsp; is immaterial. Hence it is not even necessary to recover &nbsp;. This
patch should bring nsTextFrame in compliance with CSS3 (at least for
English/Western texts and similar).

It also fixes a couple of existing bugs:
- nsTextTransformer was forgetting to set |wasTransformed| in one of its
codepath.
- nsTextFrame had a loose end when the trailing space is trimmed and linelayout
is not made aware of this extra width. In the course of testing big values of
word spacing, I noted some oddities in that case. I added some detailed comment
about that and this explains the proposed fix.

I am travelling to the US and won't look at this bug again in the coming weeks.

Saito, feel free to iterate/drive the testing.
Attachment #182869 - Attachment is obsolete: true
Attachment #183801 - Attachment is obsolete: true
Attachment #184079 - Attachment is obsolete: true
Attachment #182869 - Flags: review?(rbs)
Looks good.
But the patch cannot solve following case.
<span>a&nbsp;</span> b

The patch render...

a[ls]NBSP[ls][ws] [ls][ws]b[ls]

But this is rare.
> a[ls]NBSP[ls][ws] [ls][ws]b[ls]
               ^^^^

That's the extra...

nsTextFrame knows how to collapse the space in "<span>a </span> b" [using
lineLayout.GetEndsInWhiteSpace()], so it should be possible to undo the extra
word spacing as well, with perhaps lineLayout.GetEndsInNonBreakingWhiteSpace()!

---------
Or, one could piggy-back on TrimTrailingWhiteSpace() by making it more explicit as:

  NS_IMETHOD TrimTrailingWhiteSpace(nsPresContext* aPresContext,
                                    nsIRenderingContext& aRC,
                                    nscoord& aSpaceWidth,
                                    nscoord& aLetterSpacing,
                                    nscoord& aWordSpacing,
                                    PRUnichar& aLastChar) = 0;

And the current textframe could fetch the values from its previous text sibling,
and then decides what to do.

Anyway, there is a price to pay to nail that edge case...
And I have a problem...

<p style="word-spacing:1em;">
<span style="font-size: 3em; word-spacing: 1em;">a&nbsp;</span> b
</p>

In this case, Should it is rendered

aNBSP [ws]b

or

aNBSP[ws] b

?
rbs:

The patch may be buggy at selection.

> @@ -2950,10 +2950,12 @@ nsTextFrame::RenderString(nsIRenderingCo
>    // context.
>    nscolor textColor;
>    aRenderingContext.GetColor(textColor);
> +  PRUnichar ch = 0;
>    for (; --aLength >= 0; aBuffer++) {
>      nsIFontMetrics* nextFont;
>      nscoord glyphWidth;
> -    PRUnichar ch = *aBuffer;
> +    PRUnichar prev_ch = ch;
> +    ch = *aBuffer;

and

> @@ -3120,8 +3126,10 @@ nsTextFrame::GetTextDimensionsOrLength(n
>    PRBool isCJ = IsChineseJapaneseLangGroup();
>    PRBool isEndOfLine = aIsEndOfFrame && IsEndOfLine(mState);
>  
> +  PRUnichar ch = 0;
>    while (--length >= 0) {
> -    PRUnichar ch = *inBuffer++;
> +    PRUnichar prev_ch = ch;
> +    ch = *inBuffer++;

are buggy.
if there is following text,

&nbsp;&nbsp;

the patch will render as follwing,

NBSPNBSP[ws]

but, if selecting as following,

[&nbsp;]&nbsp;

the patch may render as following,

&nbsp;[ws]&nbsp;[ws]

I'm taking this bug. I will fix this bug in 1.9 cycle.
Assignee: layout.fonts-and-text → masayuki
Oops. I correct above comment.

NBSPNBSP[ws] -> NBSP[ws]NBSP
Priority: P3 → P2
Target Milestone: Future → mozilla1.9alpha
Note that the ideal rendering is to, for word-spacing, to put half the
word-spacing after the end of each word, and half the word-spacing before the
beginning of each word, removing any such spaces at the visual start and end of
the line. So in:

   <p><a>A</a> <b>B</b></p>

...you render:

   1. "A" in <a> font
   2. Half of <a>'s word-spacing
   3. The space in <p> font
   4. Half of <b>'s word-spacing
   5. "B" in <b> font

...and <p>'s word-spacing has no effect.

(Similar things should ideally happen with letter-spacing but I guess that's
another bug.)

cc'ing dbaron in case I'm on crack.
Thank you, Ian. It is good idea.
I will implement so.
Summary: css word-spacing applies at &nbsp; at paint time but not at layout (reflow) time → css word-spacing applies at &nbsp; at paint time but not at layout (reflow) time (Reimplement word-spacing property)
Ian:

Your idea has two problem.
<p>word <a style="background-color: blue;">word</a> word</p>

In this case, I think that the page author may expect a element's
background-color should render to under the characters only. But in your idea,
the background is render like the element has padding. I.e.,

word[ws] [ws]word[ws] [ws]word
         |<-------->|
             blue
And another problem, if implementing so, selecting the word by mouse is not
easy. Because word's edges are not glyph edge.
If we want to add the word-spacing to space by word's font, it is not good for
footprint. It is need that the nsTextFrame is always cacheing the word-spacing
value.
> Your idea has two problems.
> <p>word <a style="background-color: blue;">word</a> word</p>
> 
> In this case, I think that the page author may expect a element's
> background-color should render to under the characters only. But in your idea,
> the background is render like the element has padding.

My proposal does not specify where the space is rendered, in terms of
interacting with the inline box model. I would suggest that the additional
whitespace be added to the actual space character, so that the net effect is
merely to make the actual space wider.


> And another problem, if implementing so, selecting the word by mouse is not
> easy. Because word's edges are not glyph edge.

Nothing in my proposal requires that the selection behaviour be affected by the
word spacing. Specifically, there is nothing requiring that the word-spacing
effect actually be counted as being a part of the word itself.


My proposal is merely that when the additional space is calculated, the actual
length of that space only take into account the 'word-spacing' of the two words.
I would recommend that that space be placed with the actual space that is being
affected (the U+0020 or other space character that was considered to split the
words in the first place). Similarly I would recommend that mouse selection
behaviour be left untouched such that it remains intuitive.
Ian, I can understand why the boundary between
  <a>foo</a><b>bar</b>
would be half the letter spacing of a plus half the letter-spacing of b, but I
don't understand how the space between a & b in this example
  <a>foo</a> <b>bar</b>
would be affected by the word-spacing settings within a & b any more than the !
in the following example would be affected by the letter-spacing settings within
a & b:
  <a>foo</a>!<b>bar</b>

The problem masayuki describes, where part of the space seems to be inside a &
b's boxes, is significant, particularly with negative word-spacing. (And
negative word-spacing is not an edge case, thanks to the way word-spacing is
defined as being /in addition to/ the default size of a space.) As for your
disclaimer, applying the extra spacing with the settings of one element but in
the box of the other is counter-intuitive.

Also, in the future, please bring up such proposals with the CSS WG before
suggesting that people implement them. Sorting out ambiguities in the spec
should be the WG's job, not the spec interpreter's. Particularly when the spec's
interpreter is not the spec's editor.
Currently, I'm working following spec.
# http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2821&action=view

<a>word</a> <b>word</b>

this is rendered as:

----+------------------+-----
word|[ws(a)][SP][ws(b)]|word
----+------------------+-----

<a>word</a>&nbsp;&nbsp;<b>word</b>

as:

----+----------------------+-----
word|[ws(a)][SP][SP][ws(b)]|word
----+----------------------+-----

the word-spacing is exists between non-word separator and word separator only.
and it is including word-separator.
if word separator and word is not same box, I use the width of word-spacing is
adjacent box's value.

But I think that this behavior is not 'natural'. Should we use the current box's
word-spacing value? My implementation has a problem.

The usage memory is bigger. When we render the word-separator, we must use
adjacent box's value. So, we must save the adjacent box's word-spacing value.
And the kind of box is many. we must save the img-box, object box, etc.

I propose that we should always use the current box's word-spacing value. I.e,

<a>word</a>&nbsp;&nbsp;&nbsp;<b>word</b>

is:

----+--------------------------------+-----
word|[ws(this)][SP][SP][SP][ws(this)]|word
----+--------------------------------+-----

<a>word </a><b>word</b>

----------------------+-----
word[ws(a)][SP][ws(a)]|word
----------------------+-----

<a>word</a> <img />

----+------------------------+------
word|[ws(this)][SP][ws(this)]|[img]
----+------------------------+------
And my patch has another problem.

It is performance problem. if following case:

<a style="word-spacing:0;">word </a><b style="word-spacing:1em;">word</b>

we must use slowly text rendering function for a element. Because a elements's
white space has b elements's word-spacing value. It is not good solution.

I will create new patch that is always use current box's word-spacing value...
Yeah, fair enough. Thanks for trying anyway!

I assume in a case where you have

   p { white-space: pre; }
   span { white-space: normal; }

   <p><span>A </span><span> </span> <span> </span><span> B</span></p>
            /|\                   /|\    /|\
             |                     |      |
             1                     2      3

...you'll get the three spaces indicated with the word-spacing from their parent
nodes, right? (Note that if you're just using the same spacing from the same
element now you no longer need to do it half-and-half on either side.)

Once you've fixed this bug, check if it also fixes bug 261591 and bug 159080.
Blocks: 159080, 261591
No longer blocks: 290536
In that case, my patch may make word-spacing to left of "1" and right of "3".
I.e.,
+---------++----+--------++-+
|A[ws][SP]||[SP]|[SP][ws]||B|
+---------++----+--------++-+

But actual, it may not work fine by empty frame...
<p>&nbsp;word&nbsp;</p>

In this case, my patch makes following result.
+--------------------+
|[SP][ws]word[ws][SP]|
+--------------------+

Should render as following?

+----------------------------+
|[ws][SP][ws]word[ws][SP][ws]|
+----------------------------+
 ^^^^                    ^^^^
Status: NEW → ASSIGNED
Wow, this bug has gone off tagent again. Please keep it simple (which is not 
to say that it is any "easier"...).

"Also, in the future, please bring up such proposals with the CSS WG before
suggesting that people implement them. Sorting out ambiguities in the spec
should be the WG's job, not the spec interpreter's. Particularly when the 
spec's interpreter is not the spec's editor."

Indeed. If the spec wants/expects something else, they should say it. Also, 
note that splitting the space makes it hard for users who want to impose a 
fixed/constant amount of spacing. In addition, implementing something 
radically different with letter-spacing will confuse users even more.

So we are better off aiming at _attaching_ the word-spacing to _one_ of the 
space chars. Exactly which one ultimately wins the word-spacing is what the 
current spec has left open. And that's pretty much only where we have room for 
maneuvering.

I can think of two options:
- the first space wins (could be the last, but first usually wins in HTML, eg. 
with <title> (call it "the first").
- the space with the maximum word-spacing wins (call it "the max").

Hence:

the edge case 1:
  <a style="word-spacing:0;">word </a><b style="word-spacing:1em;">word</b>

renders with no word-spacing (with either option), because ther is no ' ' to 
apply the non-zero word-spacing to. This abides by the spec, easy to 
undertand/explain to users, and they can easily markup a different behavior if 
they want to.

the edge case 2:
<p style="word-spacing:1em;">
<span style="font-size: 3em; word-spacing: 1em;">a&nbsp;</span> b
</p>

renders with a word-spacing of 3em (with either option, the &nbsp; wins, but 
could have been different with font-size: 0.5em). Gives the same user-friendly 
benefits as above.

So the real question would rather be whether the first should win or the max 
should win. I am tempted to go for the max. But the first is slightly easier 
to implement that the max and so it has some appeal too, for the sake of 
nsTextFrame...
Just thought of another situation where the max (as the default) might seem 
more intuitive:

<a style="word-spacing:5px;">word </a><b style="word-spacing:10px;"> word</b>

But I am still fine with either option (as the default) because the ambiguity 
can be resolved easily by a simple change to the markup (the user only has to 
delete the space) if either option is retained.
> Indeed. If the spec wants/expects something else, they should say it.

I'm rewriting that section of the spec /now/. (I just finished drafting
justification yesterday.) CSS3 Text has been considered unstable for a year now.
Unfortunately I haven't really had time to work on the rest of it until now;
since I've been focusing on vertical text layout (chapter 3) with a side trip
for line breaking.

> note that splitting the space makes it hard for users who want to impose a 
> fixed/constant amount of spacing

Splitting it half on one side and half on the other doesn't make that hard.
Requiring that word-spacing only applies to the word<->word-separator boundary
makes it hard. It makes it impossible to specify, for example, double spacing.
It also makes justification uneven. Large values of word-spacing in particular
would make the unmodified spaces insignificant in comparison.

> implementing something  radically different with letter-spacing will confuse
> users even more.

letter-spacing, iirc, should be applied half on one side and half on the other.
But it must indeed be applied with the settings of the character's element,
not some other element.

> renders with no word-spacing (with either option), [...] renders with a
> word-spacing of 3em

Actually, word-spacing is defined to be *in addition to*, not in place of, the
width of the space.
> Splitting it half on one side and half on the other doesn't make that hard.

Uhh... I haven't seen Masayuki's attempted patch. But I think he will disagree.
(As I haven't thought through the other points you mentioned, I don't necesarry
buy all of them, but will spare discussing them here).

> Actually, word-spacing is defined to be *in addition to*, not in place of, the
> width of the space.

Comments with little point like that don't add anything. You understand what I
meant.
my latest patch is here.
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2823&action=view

I think that rbs's idea that is max word-spacing wins is difficult for implment.
Because we cannot apply to word-spacing when it is reflowed the last element.
And we have to go back and apply it. e.g.,

<a>word </a><b> </b><c> </c><d>word</d>

In this case, we can decide when d element is reflowed. and if a element wins,
we have to go back and apply a element. it is not easy.

My patch is very simple. My patch's concept is "Splitting it half on one side
and half on the other in word and word-separator boundary". I think that it is
most 'natural' for web designer.
> > Splitting it half on one side and half on the other doesn't make that hard.
> 
> Uhh... I haven't seen Masayuki's attempted patch. But I think he will
> disagree.
>
> (As I haven't thought through the other points you mentioned, I don't
> necesarry buy all of them, but will spare discussing them here).

You were talking about making certain effects hard for the /user/. Masayuki's 
patch would be evidence for or against making /other/ things hard for the
/implementation/. If you've resolved to ignore half my comments, at least try to
consider the context of the parts you aren't ignoring before writing a sarcastic
reply.

Further comments:

  - CSS2.1 requires that _all_ preserved spaces be affected by word-spacing.
    This is in fact what both Mozilla and IE currently implement.
    (Note that CSS2.1 more accurately represents the working group consensus
    than CSS3 Text does.)
      http://www.w3.org/TR/CSS21/text.html#spacing-props
  - Some word separators are *visible*. Adding word-spacing on only one side
    of the separator would look badly skewed.
> <a>word </a><b> </b><c> </c><d>word</d>
> 
> In this case, we can decide when d element is reflowed. and if a element wins,
> we have to go back and apply a element. it is not easy.

It looks to me like justification. Two passes are needed. One has to figure out
which frames' rects have to be updated, and then apply the updates.

> http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2823&action=view

It might perhaps worth thinking about generalizing the justification code by
using an array of inter-frame spacings. Maybe, by changing
ComputeJustificationWeights/ApplyFrameJustification to
ComputeInterFrameSpacing/ApplyInterFrameSpacing.
For justification, the spacings will be constant, but could be different for the
word-spacing property. The array could then be cheaply kept on the stack during
HorizontalAlignFrames(). [allowing plently 0-spacings to simply align the i-th
frame at spacing[i] for the readability of the code.]

Re: fantasai. I had no interest in discussing what the spec should say there. My
reply isn't sarcastic. Your further comments are more constructive. So, in pre,
the code should make sure to insert the word-spacing to each space -- as opposed
to inserting it only once... For the 'visible' separators, got an illustration
of what you mean? I wonder if they matter since word-spacing only applies to
space (per CSS3). 
> It looks to me like justification. Two passes are needed. One has to figure out
> which frames' rects have to be updated, and then apply the updates.

Spoke too soon... It is trickier than that. Unlike justification, word-spacing
can push a frame to the next line.
CSS2.1 trumps CSS3 at the moment, when they conflict.
In CSS 2.1
http://www.w3.org/TR/CSS21/text.html#propdef-word-spacing
> This value indicates inter-word space in addition to the default space between
words. Values may be negative, but there may be implementation-specific limits.

This doesn't say "apply once". But, this doesn't say "apply every space".
This spec says only "between words".

I think that it is not "apply every space".
I don't think that CSS 2.1 and CSS 3 conflict.
Oops..

Sorry. CSS2.1 says following:

When white space is preserved, e.g. with 'white-space:pre', all space characters
are affected by word-spacing.

Should I create CSS2.1 patch?
See comment #32 for what "once" and "every" mean. For clarity, given many 
(wordlen) spaces, it is whether to use

dimensions.width +=
  (aTs.mSpaceWidth + aTs.mWordSpacing + aTs.mLetterSpacing)*wordLen;

vs.

dimensions.width += 
  (aTs.mSpaceWidth + aTs.mLetterSpacing)*wordLen + aTs.mWordSpacing;

I hope I am understanding the spec folks well: that in <pre>, it is the 
former. In other contexts, it is the latter.

---
Then, there is that other issue of where to physically put the extra width 
(and there come "the half" thing per hixie, or the "first" thing or "the max" 
thing that I alluded to).

I have reservations about your patch because it makes all frames become aware 
of text/line/whatever by changing a core interface just for an obscure 
feature. This generally isn't a good sign because things add-up and add-up... 
It is not "simple" (I differentiate "simple" from "easy". In my experience, 
getting simple patchs for textframe issues needs deep thinking even more).

Note: a different implementation (for "the max" or other approaches for that 
matter) might be simpler if the current frame only has to query the previous 
frames in a way that can be confined to line reflow.
rbs:

the frame chain on a line is need for CJ people. Because I want to fix bug
156369 and bug 289130 at 1.9 cycle. These bug need previous or next frame's
character. But currently, we cannot get they. See bug 295483.
Yeah I don't think the half thing is necessarily the best idea anymore. Maybe we
should just apply to each space (that is actually rendered) the word-spacing for
the element to which that space belongs, and not bother trying to be clever.
Re: comment #65

Indeed, that's has been my preference. But then, it remains open what to do with:

<a style="word-spacing:5px;">word </a><b style="word-spacing:10px;"> word</b>
                                 ^^                                 ^^
As one space has to be collapsed (i.e., only one space is actually rendered), we
then have to decide which one ultimate wins the collapsing game -- which leads
to "the first" vs. "the max" thing.

Re: comment #64
It seems to me that one can address those bugs without a whole raft of new APIs.
lineLayout.FindNextText() already tells the next "adjacent" text frame (it is
not bounded to reflow, but the metrics are needed to determine if it will fit
anyway). And autospace looks like a special letter-spacing problem -- where the
spacing array has to be filled in a special way based on the actual character
data. Might be better to discuss these in the other bugs.
Another question that is bugging me is what to do with, say:

<a style="word-spacing:5px;">word </a><b style="word-spacing:10px;">&nbsp;</b><c 
style="word-spacing:15px;"> word</c>

This will render with a run of 3 spaces between the two words. But the spec says
any run of spaces only attracts one word-spacing.
The spec is very clear about collapsing behavior, I think.
  http://www.w3.org/TR/CSS21/text.html#q8
The first space is the uncollapsed one. But which space gets preserved is a
separate issue from how word-spacing is applied.

I've asked Hixie to add an issue to the CSSWG issues list about how the wording in
section 16.4 doesn't seem to apply to &nbsp; sequences. I'm quite sure we'll
choose to require the same behavior for nbsp as we do for preserved spaces, but
I will put it on the agenda for the next telecon so we can get a definite answer.
> I'm quite sure we'll
> choose to require the same behavior for nbsp as we do for preserved spaces,

I think that we should add EN SPACE, EM SPACE and IDEOGRAPH SPACE to word separator.
> This will render with a run of 3 spaces between the two words. But the spec 
> says any run of spaces only attracts one word-spacing.

The spec clearly says "When white space is preserved, e.g. with
'white-space:pre', all space characters are affected by word-spacing." -- so all
three spaces there would be affected by word-spacing.

It seems to me obvious now that we should use the entire word-spacing of the
space that is not collapsed when determining what distance to add to a space.
This is because that's the space that the borders will go around.
The fixed-width (em) spaces should not be affected by word-spacing or by
justification. Their purpose is to be fixed-width for manual typesetting, not to
separate words.

Ian, preserving nbsp chars has never fallen under "preserving whitespace", and
the category "spaces" very often only includes U+0020, not U+00A0.
fantasai:

How about IDEOGRAPHIC SPACE(U+3000)?
http://www.unicode.org/charts/PDF/U3000.pdf
It is used like SPACE(U+0020) by Japanese people. It is called "Zen Kaku Space"
in Japan, it means "double width space".
jshin:

Is the IDEOGRAPHIC SPACE using in Korea? If using it, isn't it word separator?

In Japan, it may be used for the purpose of an indent, another case, it is used
with FULLWIDTH alphabets, instead of ASCII chars.
Attached patch css2.1 patch (obsolete) — Splinter Review
This fixes &nbsp; problem only. This conforms to CSS 2.1 spec(every space has
word-spacing).
Attachment #185876 - Flags: review?(rbs)
Summary: css word-spacing applies at &nbsp; at paint time but not at layout (reflow) time (Reimplement word-spacing property) → css word-spacing applies at &nbsp; at paint time but not at layout (reflow) time
Comment on attachment 185876 [details] [diff] [review]
css2.1 patch

It is buggy.
Attachment #185876 - Attachment is obsolete: true
Attachment #185876 - Flags: review?(rbs)
Attached image screenshot (obsolete) —
I cannot fix testcase2(attachment 132717 [details])'s second case.
See this screen shot. the table size is not including word-spacing.
Attached patch css2.1 patch rv2.0 (obsolete) — Splinter Review
O.K. this cleared all test case. this is similar to Saito-san's second
patch(attachment 132715 [details] [diff] [review]).
Attachment #185893 - Flags: review?(rbs)
Yes, I'd apply word-spacing to ideographic space.

BTW, draft text for CSS3 is at
  http://fantasai.inkedblade.net/style/specs/css3-text/scratchpad#word-spacing
I still need to talk with some i18n people about Tibetan and such, though.
It's been resolved to change 
    "When white space is preserved, e.g. with 'white-space:pre', all
     space characters are affected by word-spacing."
to 
    "Word-spacing affects each space (U+0020), non-breaking space (U+00A0),
     and ideographic space (U+3000) left in the text after white space
     processing rules have been applied."
in CSS2.1 section 16.4.
Comment on attachment 185893 [details] [diff] [review]
css2.1 patch rv2.0

1/ premature optimizations

+	     if (aTs.mWordSpacing) {
+	       PRInt32 wordSeparatorCount = 0;
+	       if (aTx.TransformedTextIsAscii()) {
+		 for (char* bp = bp1; bp < bp1 + wordLen; bp++) {
+		   if (*bp == ' ')
+		     wordSeparatorCount++;
+		 }
+	       } else {
+		 for (PRUnichar* bp = bp2; bp < bp2 + wordLen; bp++) {
+		   if (*bp == ' ')
+		     wordSeparatorCount++;
+		 }
+	       }
+	       if (wordSeparatorCount > 0)
+		 dimensions.width += aTs.mWordSpacing * wordSeparatorCount;
+	     }

This is too mouthful. Just go for readability and small code size since today's
blazzingly fast CPUs remove the need to have such hand-rolled optimizations.
You can get rid of the counter(s) throughout your patch and simply use:

+ if (*bp == ' ') // || *bp == CH_CJKSP) // CH_CJKSP = U+3000, per updated spec
+   dimensions.width += aTs.mWordSpacing;


2/ avoid nbsp -- this comment is FYI only.

It will be good to avoid passing nbsp straight to the font subsystem. It causes
other problems (e.g., bug 238598) that are as ugly to resolve down there than
up here. Avoiding them needs some creativity than one would think at first (you
want to avoid multiple conversions back and forth for linebreak and text
measurement). That's why I am not asking for it in my review. I am just
pointing it so that you can keep it in mind, in case you have a creative way to
address it... [The reason I am interested in this aspect is because it calls
into question the conversions done elsewhere (if nbsp can be left as-is and
passed down, then why bother with the other conversions to ' '. The code could
be simplified by getting rid of all these conversions, then).]

3/ buggy trimming.

There is still the buggy linebreak I indicated earlier (not your fault). I am
going to attach a screenshot for illustration.
Attachment #185893 - Flags: review?(rbs) → review-
Attached image screenshot showing the trimming bug (obsolete) —
This screenshot shows the trimming bug. To reproduce it, set a large
word-spacing, e.g., 100pt, as was done in the screenshot, and resize the window
accordingly.

To fix it requires the following bits from my attachment 184513 [details] [diff] [review]:
@@ -5201,6 +5216,11 @@ nsTextFrame::MeasureText
@@ -5614,9 +5652,11 @@ nsTextFrame::MeasureText

Also useful is this bit since it is clearly an oversight:
@@ -595,6 +595,7 @@ nsTextTransformer::ScanNormalUnicodeText

Feel free to include them in your patch. As I will be on the hook too, I will
take responsibility for unforseen problems that may happen, which are unlikely
to my knowledge.
Attachment #184513 - Attachment is obsolete: true
I file a new bug that is letter-spacing implementation. See bug 299943.
*** Bug 306567 has been marked as a duplicate of this bug. ***
rbs and roc:

I want to restart my work on this bug, is it O.K.?
# Or I should wait after the nsTextFrame.cpp being refactored?
# http://weblogs.mozillazine.org/roc/archives/2006/02/post_1.html
It is okay with me as the bug was nearly completion and is not dependent on the refactoring which still has a very long way to go.
Attached patch css2.1 patch rv3.0 (obsolete) — Splinter Review
Sorry for the delay.

rbs, please review this.
Attachment #132716 - Attachment is obsolete: true
Attachment #179353 - Attachment is obsolete: true
Attachment #183885 - Attachment is obsolete: true
Attachment #185884 - Attachment is obsolete: true
Attachment #185893 - Attachment is obsolete: true
Attachment #186900 - Attachment is obsolete: true
Attachment #214192 - Flags: superreview?(rbs)
Attachment #214192 - Flags: review?(rbs)
You didn't take all the bits from comment 81. You are missing this other bit:

@@ -595,6 +595,7 @@ nsTextTransformer::ScanNormalUnicodeText
         PRUnichar ch = *cp++;
         if (CH_NBSP == ch) {
           ch = ' ';
+          *aWasTransformed = PR_TRUE;
         }
Oops. Sorry.
Attachment #214192 - Attachment is obsolete: true
Attachment #214311 - Flags: superreview?(rbs)
Attachment #214311 - Flags: review?(rbs)
Attachment #214192 - Flags: superreview?(rbs)
Attachment #214192 - Flags: review?(rbs)
Attached patch with -w (obsolete) — Splinter Review
Comment on attachment 214311 [details] [diff] [review]
css2.1 patch rv3.1

r+sr=rbs
Attachment #214311 - Flags: superreview?(rbs)
Attachment #214311 - Flags: superreview+
Attachment #214311 - Flags: review?(rbs)
Attachment #214311 - Flags: review+
checked-in, thanks.

rbs:

Do we need to fix this on 1.8 branch? I don't know the severity of this bug for western language (space separated languages) users.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Go for the branch too (since that is where Firefox 2 is supposed to come from).
-> v.
Status: RESOLVED → VERIFIED
Keywords: qawanted
rbs:

O.K. Let's go to 1.8 branch.
Attachment #214312 - Attachment is obsolete: true
Attachment #214561 - Flags: superreview+
Attachment #214561 - Flags: review+
Attachment #214561 - Flags: approval-branch-1.8.1?(rbs)
Attachment #214561 - Flags: approval-branch-1.8.1?(rbs) → approval-branch-1.8.1+
I think this caused bug 329987
Depends on: 329987
Whiteboard: [Pending check-in to 1.8 branch until bug 329987 is closed]
Depends on: 330268
This caused bug 330268.

Please make sure to not land on branch until all regressions are addressed (and have branch approval), ok?
Yeah, of course.
No longer depends on: 331042
You need to log in before you can comment on or make changes to this bug.