This one's really minor, but I noticed that if you type a line in a text box that wraps, and the very next character is a space, that the space ends up at the beginning of the next line, rather than at the end of the previous line. This causes the left edge of the text to look ragged. I believe 4.x handles this by treating leading spaces specially.

This seems similar to bug #14588. Frank--I am reassigning to you since I believe this is a line-breaking issue. If you feel that it is really an editor issue, please reassign back to beppe@netscape.com or me.

warren, can you make a test case for this ?

No I can't. You'll just have to type into a text box to see it.

My experience with 4.7 and any number of other apps that do word-wrapping suggests that the heuristic in this circumstance is that the space after a word is considered part of the word for purposes of line-breaking (most purposes, actually, including selection). Open any program that automatically word-wraps and will let you resize the editing window narrower; after the word-wrap algorithm is triggered, there will always be a [space] character left at the end of any line that the algorithm broke. You can select this space character (In a word-processor, in full-justify mode, this space will show up in the margin). File>New>Message in Communicator will let you try this. (The same in Mozilla only show that this bug isn't the only line break bug - new bug 19492 added) I believe that this is a nearly universal, XP convention - certainly it applies to any editor I've used on Mac and Windows. I disagree that this is minor. Scenario: user is typing a message, sees space at start of line 2, kills it, removes an injudicious phrase from line 2, manually reformats by typing [END], [DELETE] to move up words from line 3, and, because the space was not left at the end of line 2 by the lin-breaking algorithm, the first word of line 3 ends up joined to the last word of line 2. This is *very* annoying, much more so than the ragged left edge. Changing summary from "leading spaces handled improperly" to "Word-wrap improperly breaks before space following last word" Adding dependency to bug 14588 - it's about reworking the algorithm.

It is a problem with -moz-pre-wrap I guess kipp break this when he rewrite nsTextTransformer.cpp Attach the test cases Clear TM and reassign to rickg. RickG- Who take over kipp's line wrapping code thest days? I can help after I fix all my PDT+

*** Bug 20223 has been marked as a duplicate of this bug. ***

according to cvs log http://cvs-mirror.mozilla.org/webtools/bonsai/cvslog.cgi?file=mozilla/layout/htm l/base/src/nsTextTransformer.cpp kipp check in his rewrite (r1.27) in Oct 19. We probably should try build before Oct 18 to see is this bug caused by his rewrite or not. If the bug does not exist before Oct 18, then the bug is inside nsTextTransformer.cpp GetNextWord(), otherwise, it is in nsTextFrame.cpp

Giving to kipp as placeholder for now.

*** Bug 19409 has been marked as a duplicate of this bug. ***

Updating to default Editor Assignee...kipp no longer with us :-(

reassign back to kipp per comments by troy

mass moving all Kipp's pre-beta bugs to M15. Nisheeth and I will prioritize these and selectively move high-priority bugs into M13 and M14.

Based on comments, marking this {css-moz}.

*** Bug 30200 has been marked as a duplicate of this bug. ***

mine! mine mine mine! all mine! whoo-hoo!

moving all buster m15 bugs to m16.

nothing will be done with this for beta2 if it stays on my list. Setting to M17.

*** Bug 36183 has been marked as a duplicate of this bug. ***

This problem also occurs in HTML forms. Suggestion as to the proper behavior: spaces after a word to be wrapped are treated as newline characters (ick--just happenned in this phrase.) In that case they can be deleted and restored without fear of mangling the text.

I don't know if this is related but now you can't tell how many spaces you have typed between words any more.

neil@parkwaycc.co.uk -- I believe that is a separate issue from this bug. Could you file a bug on it and cc me? Assign it to the editor Component and assign it to jfrancis@netscape.com. Thanks!

The multiple spaces I mentioned have been fixed but now the wrapping in text areas has changed. It appears that multiple consecutive spaces are being treated as nonbreaking except for the first and last space. In Navigator which I assume uses a Windows edit control, if the wrap occurs on the [first of multiple] spaces then that space takes up no space. However if the wrap occurs on later spaces then they all take up space. FYI, in Microsoft Word the wrap always occurs on the last space. This can mean that lines can be longer than the wrapping width, if there are a lot of spaces, but wrapped lines then never start with a space.

Here is the initial batch of layout [TEXT] bugs. I'll search for more today, but this should keep you busy for a while! Please review these for possible dogfood or nsbeta2 candidates, and assign your own priority and milestone for each. The current settings were relative to my bug list, and they might not make sense any more.

M11 and M12 both have this bug. M13 fixed it. Then M14 introduced a bad regression where the text doesn't wrap at all, producing a horizontal scrollbar instead. M15 has the same problem. I'm leaving this bug open until the regression is fixed, so that we can check whether this bug is still fixed. Does anybody know if the regression has been filed separately? This is for a <TEXTAREA WRAP> in HTML.

As per meeting with ChrisD today, taking QA.

Mark it as M23 for now.

*** Bug 60024 has been marked as a duplicate of this bug. ***

*** Bug 60193 has been marked as a duplicate of this bug. ***

*** Bug 59540 has been marked as a duplicate of this bug. ***

adding mostfreq keyword

*** Bug 60931 has been marked as a duplicate of this bug. ***

cc to self

cc shanjian@netscape.com

*** Bug 68927 has been marked as a duplicate of this bug. ***

*** Bug 69113 has been marked as a duplicate of this bug. ***

*** Bug 69288 has been marked as a duplicate of this bug. ***

*** Bug 70080 has been marked as a duplicate of this bug. ***

*** Bug 70661 has been marked as a duplicate of this bug. ***

This looks fixed to me. Hixie - as QA contact, can you check this out? Gerv

Close now... a single space after the last visible character that will fit on a line is not ending up at the beginning of the next line in Composer, nor in a <TEXTAREA> iff a vertical scrollbar appears, but if a <TEXTAREA> has no vertical scrollbar, it ends up on the next line. Also, if more than one space trails the last visible character that will fit, all but the first end up on the next line. In the Additional Comments text box above: (a) If there is a vertical scrollbar, visually 81 characters will fit, but in reality, the 81st character must be a space, otherwise it (and the word it is attached to) end up on the next line. (b) If there is a vertical scrollbar, and if there are two or more spaces trailing the 80th character, only the first stays on the current line. (c) If there is no vertical scrollbar, visually and in reality 82 characters will fit, but a space after the 82nd will end up on the next line. Observations (b) and (c) do not match expectations set by NN 4.x, and by text editors and word processors everywhere. Any number of trailing spaces after the last visible character that will fit should stay logically and visibly part of the current line, no matter how many such spaces, and no matter how many lines in the editing area. Observation (b) also holds for Composer, whether or not there is a vertical scrollbar. Tested with: 2001-03-19-15-M0.8.1 on WinNT 2001-03-20-06-Mtrunk on WinNT Suggested: * Resolve this bug as FIXED or WORKSFORME for the common case; the common case works. * Open a new bug to solve problem (b), to fully solve the problem reported and bring us to 4xp compliance, referencing this bug. * Open a second new bug, dependent on the first, to solve problem (c) in the unlikely case that the fix for the first new bug doesn't fix it too. Additional Information: Problem (c) would be reduced to an edge case if greyed-out vertical scrollbars appeared in all <TEXTAREA>s that haven't been styled to prevent scrolling, but I'm guessing that there is a reason that they no longer appear.

Not quite sure how I ended up with QA on this, ->sujay@netscape.com...

this is a layout issue; changing component

*** Bug 73462 has been marked as a duplicate of this bug. ***

Gerardo, please assign qa_contact to someone in your group...thanks.

line breaking issue. Reassign to shanjian . This is currently mark as P3. Mark it as moz1.0 for now.

I read through the bug report and it seems part of the problem is fixed, part of it is not. I tried the 2 test cases, one works and one does not. I proposed a patch to fix the remaining test case (No. 1). Please file other bugs if there is issues not covered by the 2 testcases.

to code reviewer: In my proposed patch, there is one thing that is arguable. That is either the width of space should be added to aTextData.mX. In my patch, this is not done. The con of this approach is that user will not be able to see the space. Another approach is to always add this space to aTextData.mX. The consequence is that it will cause the horizontal scrollbar appears. I believe the H-scrollbar should be undesired for wrapped text area.

i think not getting a scroll bar is the lesser of evils. Other browsers let that trailing space be "invisible". People are used to it.

Joe, did you check my patch. If so, can I put r=jfrancis?

you need to get an r= from someone in layout, or at least more familiar with that source file. I'm not familiar enough with this to do a good review. (I did look at the patch.)

These two conditions are mutually exclusive, ((0 == aTextData.mX) || !aTextData.mWrapping || (aTextData.mX + width <= maxWidth)) ((0 != aTextData.mX) && aTextData.mWrapping && (aTextData.mX + width > maxWidth)) and the code in between the two if statements does not mutate any of the conditions upon which these conditions depend. So, wouldn't the following accomplish the same end without requiring an extra set of tests? Index: nsTextFrame.cpp =================================================================== RCS file: /cvsroot/mozilla/layout/html/base/src/nsTextFrame.cpp,v retrieving revision 1.293 diff -u -r1.293 nsTextFrame.cpp --- nsTextFrame.cpp 2001/05/02 11:03:03 1.293 +++ nsTextFrame.cpp 2001/05/02 21:46:37 @@ -4457,6 +4457,12 @@ width = wordLen*(aTs.mWordSpacing + aTs.mSpaceWidth);// XXX simplistic } + prevColumn = column; + column += wordLen; + endsInWhitespace = PR_TRUE; + prevOffset = aTextData.mOffset; + aTextData.mOffset += contentLen; + if (aTextData.mMeasureText) { // See if there is room for the text if ((0 != aTextData.mX) && aTextData.mWrapping && (aTextData.mX + width > maxWidth)) { @@ -4465,11 +4471,6 @@ } aTextData.mX += width; } - prevColumn = column; - column += wordLen; - endsInWhitespace = PR_TRUE; - prevOffset = aTextData.mOffset; - aTextData.mOffset += contentLen; } else { // See if the first thing in the section of text is a // non-breaking space (html nbsp entity). If it is then make Let me know if I've missed the point. I need to think harder about the implications of this change: > The con of this approach is that user will not be able to see the > space. In other words, when typing, will the caret fail to advance on the last space in a line? (Try filling a line in a <textarea> with nothing but spaces using the current build; it doesn't exactly look to be expected behavior: probably another bug?)

Between the judgement of those 2 condition, one variable is changed, that is aTextData.mX. Suppose in first condition, aTextData.mX is 0, so width is added to aTextData.mX, and in 2nd condition aTextData.mX is no longer 0, and we might break there. On my debug build, after advance to the unseen space, the caret become invisible. Adcancing again and caret will because visible again. Because the space is invisible, user might guess that caret is out of visible view area as well. And back or forward key will make it visible again. So I guess that is not really a problem.

Ah, right. (Duh!) Ok, it'll take me a bit of time to understand what's going on.

Ok, I think this fix is reasonable from a layout standpoint (could use some logic cleanup and some commentary about what's happening); however, I think it's going to have bad UI repercussions. Specifically, if you add *many* spaces near the end of a line, you'll push the caret *way* off into space. (Losing the caret at all is disconcerting; with this change, you can really shoot it if off into the weeds.) What if, in the -moz-pre-wrap case, we 1) treat whitespace as ``belonging'' to the previous word, and 2) set the TextStyle::mSpaceWidth to zero In other words, the string |abc | would count as being 4ch wide, and would be separated from the next word by ``whitespace'' that took up no width. That would cause us to wrap as follows (underscore means the currently ``typed'' space character, dot indicates the zero-width ``whitespace''): (start) ->a .b .c_ <- -> <- (last character on the line) ->a .b .c__<- -> <- (one character too many!) ->a .b . <- ->c___ <- I think that doing this wouldn't be that hard. Specifically, the text transformer would simply return the whitespace that follows each word along with the word, alternated with a zero-length whitespace span. (It might not even be necessary to stomp the TextStyle::mSpaceWidth.) (I'm sort of johnny-come-lately to this bug, so I may be saying stupid stuff.)

> Specifically, if you add *many* spaces near the end of a line, you'll push > the caret *way* off into space. In native Mac OS text controls in this situation, the caret is always visible one space away from the end of the last word -- no matter how many spaces you enter on the end of the line. This makes it feel a bit weird if you press Backspace to delete the multiple spaces (since, except for when deleting the last space, the caret doesn't move), but it's better than hiding the caret entirely.

Treating space character is part of belonging of next word might not be the right way to go. Just like in your example, word "c" is moved to next line just because it has many space connected to it. That will make the text looks ugly, and it is different from behavior of any other similar applications. I can give you a example that is even worse. Suppose the line only have one word, and it has lot of space character connected to it and you have to break between space in order to make the caret visible. Then you got the old problem again, a line started with space! I would suggest to make the caret visible if its real position is out of visible range. Just let the caret appears in the end of the line. I am not sure if there is easy to so. If not, we might just add the space width and let Horizontal scrollbar appears.

in IE on mac, textareas get a caret pinned to the end of the line if there are trailing spaces. As Matthew points out, this feels weird when backspacing, but it seems like a lesser of evils to me.

(My suggestion is stolen from IE5 on windows, FWIW.)

NN 4.76 on Windows works the same as Mac text controls: any number of spaces at the end of a line always logically stay part of that line; only one space is displayed, always on the same line; the caret is always visible; the next non-space character begins the next line. The same behaviour is used by *every* line-aware and word-wrap-capable text editor I have ever used, on any platform. 4.x parity is the way to go here; 4.x got it right. In contrast IE 5.5 on Windows sends the last word on a line to the next line if one too many characters of any kind are typed -- including a space. That violates expectations from all normal editors, and would make hand-reflowing indented text a right royal PITA. I'd guess that the implementors of IE on Windows ran into the same sort of problem re-implementing their text controls, and just stopped short of fixing them to parity with the OS-native controls (which in turn have parity with Mac controls, and also DOS editors). IE flouts 20+ years of XP editor behaviour; I wouldn't suggest emulating it. Could we try Shanjian's patch (in nightly binaries, assuming there are no other issues), and see about keeping the caret visible, before considering further another implementation that would violate other expectations about spaces at the wrap-column?

Maybe one of the editor folks could chime in here about pinning the caret?

Simon is known among the ladies as Mr. Caret (cc'ing).

I think we should do what has been proposed (pin at the end of the line--what Sean Richardson suggested above and jfrancis thought was best).

Can we get this into 0.9.1 or 0.9.2? It's a pretty embarrassing bug.

Ok, unless I wasn't clear, I think that the patch is fine, in spirit, from a layout perspective. I think that the compound test should be moved into an inline function, or even better, re-think the flow-of-control to avoid doing unnecessary tests twice. I'd also like to see some commentary about what's going on. shanjian, could you produce another patch that addresses these issues?

The compound test is an easy issue, but the caret is a troublesome one. I think probably the caret problem should be taken care of elsewhere. Is there anybody volunteer to take a look? If not, I will spend some time on this after I land my new charset detector this week.

To have the caret do special things when drawing in whitespace at the end of a line, we'd need to add a flag to nsIFrame::GetPointFromOffset() that is something like 'aCollapseTrailingLineWhitespace', and then nsTextFrame::GetPointFromOffset() would need to, if the flag was set, do some special magic and pass back an offset for where you want the caret to draw. I think this is pretty do-able.

I proposed a new fix base on the feedback collected. There are 2 changes: 1, Two compound test is combined. 2, Caret position is adjust to make sure it is within frame rect. The 2nd change might be arguable. To return a point outside the frame rect does not make sense, and thus cause the caret to be invisible. So instead of setting flag as Simon suggest, why not just modify the position directly? I can't think of any situation which a out-range position is desired. The modification can happend in nsCaret also. I believe nsTextFrame might be the better place. Chris, Simon, Any suggestion?

Looks find to me.

(cc'ing people who may be familiar with the text measurement stuff.) I think that this patch is a bit different from your first patch. Specifically, if 0 == aTextData.mX but aTextData.mX + width + width > maxWidth we'll break out of the loop. Your original patch didn't do this: if |0 == aTextData.mX|, we _always_ continued on to the next word. But, looking at this more carefully, I wonder if maybe that doesn't matter at all? Specifically, I'm wondering if the real reason this works is that you unconditionally set |endsInWhitespace| to true and advance past the whitespace (by incrementing |aTextData.mOffset|). Specifically, we could probably re-write your second patch as: if (aText.mMeasureText) { // See if the whitespace will fit within |maxWidth|. if (0 == aTextData.mX || !aTextData.mWrapping) { // If we're the first bit of text being placed in the // frame, or if wrapping isn't allowed, then keep on // measuring. aTextData.mX += width; } else if (aTextData.mX + width <= maxWidth) { // The whitespace will fit. aTextData.mX += width; // XXX will it fit again? if (aTextData.mX + width > maxWidth) { // No, let's wrap. break; } } else { // We're not the first bit of text, word wrapping is // allowed, and the whitespace wouldn't fit. We need // to wrap. } } This seems pretty weird to me. It seems like if this had _any_ effect on wrapping the whitespace to the next line, then by adding _enough_ whitespace (so that |aTextData.mX + width * 2 > maxWidth|), we should be able to force a wrap. But we can't! So, does this bit of logic matter _at all_? What happens if we just unconditionally advance |aTextData.mX| and _never_ bother to check if we've blown the |maxWidth|? In other words, a patch like this: @@ -4458,13 +4458,9 @@ } if (aTextData.mMeasureText) { - // See if there is room for the text - if ((0 != aTextData.mX) && aTextData.mWrapping && (aTextData.mX + width > maxWidth)) { - // The text will not fit. - break; - } aTextData.mX += width; } prevColumn = column; column += wordLen; endsInWhitespace = PR_TRUE; I'm sorry if I'm being dense, but I'm wondering if this is more complicated than it need be?

Chris, you raised some good points. I rethought about it and came with a new patch. Yes, after mTextData.mOffset is incremented, the space are considered to be added. Now the remaining issue is whether we need to add "width" to aTextData.mX, and whether the break is necessary. The former is addressed in my new patch and the logic is very clear now. If we are in wrapping mode, we do not want to add "width" to aTextData.mX to avoid H-scroll bar. For the second one, my suggestion is to do the break. That will save a lot of other code run in next loop. So "break" is nice to have but not absolutely necessary.

Great! A couple more comments, mostly stylistic nits. + //#19265 Even if there is not enough space for this "space", we still put it here instead of next line cvs-blame will remember the bug number with your checkin comment, no need to put it here. Also, wrap this line to the 78th column. + if (aTextData.mMeasureText) + { + if (aTextData.mWrapping) + { Follow local custom, which places brace on same line as ``if''. Also, put a comment here: something like, ``if we're wrapping, then don't add the whitespace width to the x-offset unless the whitespace will fit within maxWidth.'' + } + else + { Again, local custom has else on same line with braces. Another comment, something like, ``if we're not wrapping, then always advance the x-offset regardless of maxWidth. + if (aTextData.mX >= maxWidth) + break; Are you _sure_ this is right? I'm looking at ~line 4528, and it looks like the normal text measurement will only break out if |aTextData.mWrapping| is set. If this is the right thing to do, then definitely add a comment here that says something like ``even though we're wrapping, we'll break of out the measurement loop early because the next word frame would've done so anyway.'' + } //(aTextData.mMeasureText) No comment here, please. (Again, follow local custom.) Make those changes, and r=waterson for the nsTextFrame changes. Thanks for all your work on this.

*** Bug 83097 has been marked as a duplicate of this bug. ***

move to moz0.9.2. who else should review the code. Marc Attinasi?

Looks fine to me! [s]r=attinasi

*** Bug 83899 has been marked as a duplicate of this bug. ***

a= asa@mozilla.org for checkin to the trunk. (on behalf of drivers)

fix checked in.

*** Bug 85108 has been marked as a duplicate of this bug. ***

verified fixed win2k - 2001061504 linux - 2001061508 macos - 2001061508

i'm confused about this bug. In a 6/13 tip build, well after this was fixed, i still get leading spaces in lines in a fixed width text area (a la bugzilla text area like I am typing in now). Isn't that supposed to be fixed by this bug? To be precise: if the last word on a line ends flush to the right boundary of the text area, then typing a space and the next word will put the space before the next word on the next line. Do I need to open a new bug?

Joe: I just tried this on Win32 with the text field that I'm typing into right now, and I don't see the problem: the cursor stays pinned to the right edge of the screen until I type a character other than a single-space. Is this problem Mac-only?

wfm in a current Mac build. The caret behaviour is slightly odd, but I no longer see leading spaces.

okee dokee, this is a test of the emergency text area system. Had this been a REAL emergency.... WOHOO! It worked. Ok, it worksforme with a mac debug build from 6/18/01. I guess either my NS6 opt bits from sweetlou are either misdated (they claim to be 6/13/01) or the fix went in between then and now. Sorry for false alarm. It's great to see this working.

The patch doesn't address the case where the frame is right-to-left. Opened bug 89253

*** Bug 89795 has been marked as a duplicate of this bug. ***

*** Bug 90069 has been marked as a duplicate of this bug. ***