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)
Core
Layout: Text and Fonts
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+
rbs
:
superreview+
|
Details | Diff | Splinter Review |
9.21 KB,
patch
|
masayuki
:
review+
masayuki
:
superreview+
rbs
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
Please have a look at the attachment. Everything is fine if you delete the leading inside the div.
Reporter | ||
Comment 1•22 years ago
|
||
Comment 2•22 years ago
|
||
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.
Comment 3•21 years ago
|
||
more complete testcase showing multiple permutations of style applications
Comment 4•21 years ago
|
||
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.
Comment 5•21 years ago
|
||
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
Updated•21 years ago
|
Priority: -- → P3
Target Milestone: --- → Future
Comment 6•21 years ago
|
||
.
Assignee: attinasi → font
Component: Layout → Layout: Fonts and Text
Priority: P3 → --
Target Milestone: Future → ---
Updated•21 years ago
|
Priority: -- → P3
Target Milestone: --- → Future
Comment 7•21 years ago
|
||
A patch adds word spacing of html NBSP entity for the word fragment.
Updated•21 years ago
|
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).
Comment 9•21 years ago
|
||
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.
Updated•21 years ago
|
Attachment #132633 -
Attachment is obsolete: true
Comment 10•21 years ago
|
||
Comment 11•21 years ago
|
||
Updated•21 years ago
|
Attachment #132715 -
Flags: review?(rbs)
Comment 12•21 years ago
|
||
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.
Updated•21 years ago
|
Attachment #132633 -
Flags: review?(rbs)
Comment 13•21 years ago
|
||
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)
Comment 14•21 years ago
|
||
> 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.
Comment 15•21 years ago
|
||
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 entity (  or  ) 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 y" is a single word. I think that "x 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
Updated•21 years ago
|
Attachment #133426 -
Flags: review?(rbs)
*** Bug 202610 has been marked as a duplicate of this bug. ***
Summary: css word-spacing problem with → css word-spacing applies at at paint time but not at layout (reflow) time
Comment 17•19 years ago
|
||
I think this is the same bug, but I'm not sure.
Updated•19 years ago
|
Attachment #133426 -
Flags: review?(rbs)
Comment 18•19 years ago
|
||
This patch came from posted patch (attachment 179051 [details] [diff] [review]) to Bug 96423.
Attachment #133426 -
Attachment is obsolete: true
Comment 19•19 years ago
|
||
Comment 20•19 years ago
|
||
Comment 21•19 years ago
|
||
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
Comment 22•19 years ago
|
||
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)
Comment 23•19 years ago
|
||
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?
Comment 24•19 years ago
|
||
Note that it's not clear to me that bug 290536 is a bug...
Comment 25•19 years ago
|
||
> 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.
Comment 26•19 years ago
|
||
> BTW, do you fix the "complete testcase", and bug 290536?
I posted a screen shot for these testcase.
Comment 27•19 years ago
|
||
>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.
Comment 28•19 years ago
|
||
This patch illustrates the other approach I alluded to. [Note: it treats "x " as a single word, and does not try anything fancy there. But since the are readily accessible now, those fancy things can be done if need be, while still keeping the code intelligible.]
Comment 29•19 years ago
|
||
I am seeing that Opera and IE apply different word-spacing in the face of nbsp: IE: "x y" is rendered using a gap of |countNBSP * word-spacing|: x [word-spacing] [word-spacing]y "x y" is rendered using a gap of |(countNBSP+1) * word-spacing|: x [word-spacing] [word-spacing] [word-spacing]y Opera: "x y" is treated as a single word and doesn't get any word-spacing: x y "x y" is treated as two words and so is given one word-spacing: x [word-spacing]y --------- Which way do people expect the most here?
Assignee | ||
Comment 30•19 years ago
|
||
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.
Assignee | ||
Comment 31•19 years ago
|
||
Oops... the 0pera's case of "x y" is incorrect. I think that follwing result is correct. "x y" x [word-spacing]y "x y" x [word-spacing]y
Comment 32•19 years ago
|
||
"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 }
Comment 33•19 years ago
|
||
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.
Assignee | ||
Comment 34•19 years ago
|
||
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.
Comment 35•19 years ago
|
||
Doing word spacing per CSS3 has meant that distinguishing between ' ' and is immaterial. Hence it is not even necessary to recover . 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)
Assignee | ||
Comment 36•19 years ago
|
||
Looks good. But the patch cannot solve following case. <span>a </span> b The patch render... a[ls]NBSP[ls][ws] [ls][ws]b[ls] But this is rare.
Comment 37•19 years ago
|
||
> 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...
Assignee | ||
Comment 38•19 years ago
|
||
And I have a problem... <p style="word-spacing:1em;"> <span style="font-size: 3em; word-spacing: 1em;">a </span> b </p> In this case, Should it is rendered aNBSP [ws]b or aNBSP[ws] b ?
Assignee | ||
Comment 39•19 years ago
|
||
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, the patch will render as follwing, NBSPNBSP[ws] but, if selecting as following, [ ] the patch may render as following, [ws] [ws] I'm taking this bug. I will fix this bug in 1.9 cycle.
Assignee: layout.fonts-and-text → masayuki
Assignee | ||
Comment 40•19 years ago
|
||
Oops. I correct above comment. NBSPNBSP[ws] -> NBSP[ws]NBSP
Assignee | ||
Updated•19 years ago
|
Priority: P3 → P2
Target Milestone: Future → mozilla1.9alpha
Comment 41•19 years ago
|
||
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.
Assignee | ||
Comment 42•19 years ago
|
||
Thank you, Ian. It is good idea. I will implement so.
Assignee | ||
Updated•19 years ago
|
Summary: css word-spacing applies at at paint time but not at layout (reflow) time → css word-spacing applies at at paint time but not at layout (reflow) time (Reimplement word-spacing property)
Assignee | ||
Comment 43•19 years ago
|
||
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.
Assignee | ||
Comment 44•19 years ago
|
||
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.
Comment 45•19 years ago
|
||
> 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.
Comment 46•19 years ago
|
||
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.
Assignee | ||
Comment 47•19 years ago
|
||
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> <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> <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] ----+------------------------+------
Assignee | ||
Comment 48•19 years ago
|
||
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...
Comment 49•19 years ago
|
||
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.
Assignee | ||
Comment 50•19 years ago
|
||
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...
Assignee | ||
Comment 51•19 years ago
|
||
<p> word </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
Comment 52•19 years ago
|
||
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 </span> b </p> renders with a word-spacing of 3em (with either option, the 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...
Comment 53•19 years ago
|
||
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.
Comment 54•19 years ago
|
||
> 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.
Comment 55•19 years ago
|
||
> 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.
Assignee | ||
Comment 56•19 years ago
|
||
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.
Comment 57•19 years ago
|
||
> > 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.
Comment 58•19 years ago
|
||
> <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).
Comment 59•19 years ago
|
||
> 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.
Comment 60•19 years ago
|
||
CSS2.1 trumps CSS3 at the moment, when they conflict.
Assignee | ||
Comment 61•19 years ago
|
||
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.
Assignee | ||
Comment 62•19 years ago
|
||
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?
Comment 63•19 years ago
|
||
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.
Assignee | ||
Comment 64•19 years ago
|
||
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.
Comment 65•19 years ago
|
||
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.
Comment 66•19 years ago
|
||
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.
Comment 67•19 years ago
|
||
Another question that is bugging me is what to do with, say: <a style="word-spacing:5px;">word </a><b style="word-spacing:10px;"> </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.
Comment 68•19 years ago
|
||
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 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.
Assignee | ||
Comment 69•19 years ago
|
||
> 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.
Comment 70•19 years ago
|
||
> 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.
Comment 71•19 years ago
|
||
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.
Assignee | ||
Comment 72•19 years ago
|
||
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".
Assignee | ||
Comment 73•19 years ago
|
||
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.
Assignee | ||
Comment 74•19 years ago
|
||
This fixes problem only. This conforms to CSS 2.1 spec(every space has word-spacing).
Attachment #185876 -
Flags: review?(rbs)
Assignee | ||
Updated•19 years ago
|
Summary: css word-spacing applies at at paint time but not at layout (reflow) time (Reimplement word-spacing property) → css word-spacing applies at at paint time but not at layout (reflow) time
Assignee | ||
Comment 75•19 years ago
|
||
Comment on attachment 185876 [details] [diff] [review] css2.1 patch It is buggy.
Attachment #185876 -
Attachment is obsolete: true
Attachment #185876 -
Flags: review?(rbs)
Assignee | ||
Comment 76•19 years ago
|
||
I cannot fix testcase2(attachment 132717 [details])'s second case.
See this screen shot. the table size is not including word-spacing.
Assignee | ||
Comment 77•19 years ago
|
||
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)
Comment 78•19 years ago
|
||
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.
Comment 79•19 years ago
|
||
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 80•19 years ago
|
||
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-
Comment 81•19 years ago
|
||
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
Assignee | ||
Comment 82•19 years ago
|
||
I file a new bug that is letter-spacing implementation. See bug 299943.
Comment 83•19 years ago
|
||
*** Bug 306567 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 84•18 years ago
|
||
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
Comment 85•18 years ago
|
||
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.
Assignee | ||
Comment 86•18 years ago
|
||
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)
Comment 87•18 years ago
|
||
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; }
Assignee | ||
Comment 88•18 years ago
|
||
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)
Assignee | ||
Comment 89•18 years ago
|
||
Comment 90•18 years ago
|
||
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+
Assignee | ||
Comment 91•18 years ago
|
||
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
Comment 92•18 years ago
|
||
Go for the branch too (since that is where Firefox 2 is supposed to come from).
Assignee | ||
Comment 94•18 years ago
|
||
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+
Comment 95•18 years ago
|
||
I think this caused bug 329987
Assignee | ||
Updated•18 years ago
|
Whiteboard: [Pending check-in to 1.8 branch until bug 329987 is closed]
Comment 96•18 years ago
|
||
This caused bug 330268. Please make sure to not land on branch until all regressions are addressed (and have branch approval), ok?
Assignee | ||
Comment 97•18 years ago
|
||
Yeah, of course.
This may have caused bug 331042.
You need to log in
before you can comment on or make changes to this bug.
Description
•