Closed Bug 19265 Opened 20 years ago Closed 19 years ago

[TEXT] Word-wrap improperly breaks before space following last word [INLINE]

Categories

(Core :: Layout, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla0.9.2

People

(Reporter: warrensomebody, Assigned: shanjian)

References

Details

(Keywords: css-moz, Whiteboard: r/sr=attinasi a=asa)

Attachments

(6 files)

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.
Assignee: beppe → ftang
Status: NEW → ASSIGNED
Target Milestone: M16
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.
Depends on: 14588
Summary: leading spaces handled improperly → Word-wrap improperly breaks before space following last word
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.
No longer depends on: 14588
Attached file test cases
Target Milestone: M16
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+
Assignee: ftang → rickg
Status: ASSIGNED → NEW
*** Bug 20223 has been marked as a duplicate of this bug. ***
Attached file a simplified test case
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
Assignee: rickg → kipp
Giving to kipp as placeholder for now.
*** Bug 19409 has been marked as a duplicate of this bug. ***
Assignee: kipp → beppe
Updating to default Editor Assignee...kipp no longer with us :-(
Assignee: beppe → kipp
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.
Summary: Word-wrap improperly breaks before space following last word → [TEXT] Word-wrap improperly breaks before space following last word
Summary: [TEXT] Word-wrap improperly breaks before space following last word → [TEXT] Word-wrap improperly breaks before space following last word {css-moz}
Based on comments, marking this {css-moz}.
Keywords: css-moz
Summary: [TEXT] Word-wrap improperly breaks before space following last word {css-moz} → [TEXT] Word-wrap improperly breaks before space following last word
*** Bug 30200 has been marked as a duplicate of this bug. ***
mine! mine mine mine!  all mine!  whoo-hoo!
Assignee: kipp → buster
moving all buster m15 bugs to m16.
Target Milestone: M15 → M16
nothing will be done with this for beta2 if it stays on my list.  Setting to 
M17.
Target Milestone: M16 → 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.
Assignee: buster → erik
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.
Status: NEW → ASSIGNED
As per meeting with ChrisD today, taking QA.
QA Contact: sujay → py8ieh=bugzilla
Summary: [TEXT] Word-wrap improperly breaks before space following last word → [TEXT] Word-wrap improperly breaks before space following last word [INLINE]
Mark it as M23 for now.
Target Milestone: M17 → M23
*** 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
Keywords: mostfreq
*** Bug 60931 has been marked as a duplicate of this bug. ***
cc to self
OS: other → All
Hardware: PC → All
*** 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. 
Keywords: 4xp
Not quite sure how I ended up with QA on this, ->sujay@netscape.com...
QA Contact: ian → sujay
this is a layout issue; changing component
Component: Editor → Layout
*** Bug 73462 has been marked as a duplicate of this bug. ***
Gerardo, please assign qa_contact to someone in your group...thanks.
QA Contact: sujay → gerardok
line breaking issue. Reassign to shanjian . This is currently mark as P3. Mark it 
as moz1.0 for now.
Assignee: erik → shanjian
Status: ASSIGNED → NEW
Target Milestone: --- → mozilla1.0
Attached patch proposed patchSplinter Review
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. 

Status: NEW → ASSIGNED
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?
Target Milestone: mozilla1.0 → mozilla0.9.2
Looks fine to me! [s]r=attinasi
Whiteboard: r/sr=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)
Whiteboard: r/sr=attinasi → r/sr=attinasi a=asa
fix checked in. 
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
*** Bug 85108 has been marked as a duplicate of this bug. ***
verified fixed

win2k - 2001061504
linux - 2001061508
macos - 2001061508
Status: RESOLVED → VERIFIED
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. ***
You need to log in before you can comment on or make changes to this bug.