Closed Bug 336408 Opened 15 years ago Closed 14 years ago

In designmode, caret moves incorrectly near whitespace just before <br> or line wrap

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta2

People

(Reporter: uriber, Assigned: roc)

References

Details

(Keywords: regression, testcase, Whiteboard: [dbaron-1.9:RuCo])

Attachments

(4 files, 2 obsolete files)

In designmode, when a line ends with a trailing space (followed by <br>), placing the caret at the end of line and pressing left-arrow moves the caret over the trailing space and the last non-space character. Then, pressing right arrow places the caret visually after the trailing space, but logically before it.

This regressed between 2006-04-20 and 2006-04-21, likely due to the fix for bug 132561.

Testcase coming up.
Attached file testcase
Instructions included.
Flags: blocking1.9a1?
Keywords: testcase
I see this in gmail's compose window too.

1. Enter Gmail
2. Click Compose
3. Enter "Firefox " (with trailing space) in message compose window
4. Hit Enter
5. Press Left arrow key once

Expected
Caret appears after "Firefox " (note space), eg:
Firefox |

Actual
Caret appears after "Firefox" (note no space), eg:
Firefox|
still not working?  is this mac only?
Blocks: cairomac
(In reply to comment #3)
> still not working?  is this mac only?
> 

Still broken in:
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060526 Minefield/3.0a1 ID:2006052605

This is a non-Cairo build, so I don't think this should block 334732.
This was regressed by 132561 which was not platform-specific, so I'd be surprised if this was Mac only. Can someone please confirm on a different platform?
Hmm...  So what code actually moves the caret?  In this case, the problem is that we really do NOT want to be rendering the space, in general.  I'm really not sure how to make that play nice with "wysiwyg" editing.  :(

I can confirm that this is a problem on Linux too.  This has absolutely nothing to do with cairo.  chofmann, did you add the cairo dep to the wrong bug or something?
No longer blocks: cairomac
OS: Mac OS X 10.4 → All
Hardware: Macintosh → All
Attached image Bidi screenshot
The problem is even more compounded in the bidi case, where the trailing space is not necessarily at the visual end of the line.
This screenshot depicts the following (logical) situation (capitals represent RTL chars):

fooABC |
DEF

While HTML dictates that the space should not be rendered, this is not acceptable while editing. The (human) editor in this case would certainly expect a space to appear, and the RTL text to be shifted to the right accordingly.
So, in this case, the problem can not be fixed by simply fixing the position of the caret.
OK.  So we should either back out bug 132561 or make our editing less wysiwyg and show the end-of-line spaces there, or something....

Is this also a problem when typing BiDi text into a textarea, btw?
(In reply to comment #6)
> The problem is even more compounded in the bidi case, where the trailing space
> is not necessarily at the visual end of the line.

That seems to be a bug in itself: trailing space should always be at the visual end of the line (even in a case like this where there is right-to-left text on the next line within the same paragraph).
(In reply to comment #7)
> Is this also a problem when typing BiDi text into a textarea, btw?
> 

No, because textarea is white-space:pre (-moz-pre-warp, actually), where all spaces are always rendered.

(In reply to comment #8)
> That seems to be a bug in itself: trailing space should always be at the visual
> end of the line (even in a case like this where there is right-to-left text on
> the next line within the same paragraph).
> 

I think this is bug 229367, which is arguably invalid/wontfix.
Bug 229367 talks about punctuation characters. Whitespace is a different question, and not invalid as I understand the UBA.
I meant to include a link to support my statement about the UBA: http://www.unicode.org/reports/tr9/tr9-15.html#L1
(In reply to comment #11)
> I meant to include a link to support my statement about the UBA:
> http://www.unicode.org/reports/tr9/tr9-15.html#L1
> 

So, of course, you're correct. I filed bug 339786 for this.
>  chofmann, did you add the cairo dep to the wrong bug or
something?

yep, sorry.
This fixes most of the problem: you still can't place the caret after the trailing space (except immediately after you typed it), but at least the caret is always rendered at the real insertion point.

For background on the touched code see bug 129560 comment #14. The fix there wasn't perfect, because it assumed that the caret is after the space whenever it was after the last rendered character. But, of course, it could be after the last non-space, but before the space itself. In this case (which was the case here) the |if| should yield false, and the shift-by-space-width-to-the-right code should not be executed.

Note that I also had to fix the typo I mentioned in bug 129560 comment 24 for this to work (at least I'm pretty sure it was a typo).

Asking review from rbs, since he was the last to touch this code (in bug 129560).
Attachment #236435 - Flags: review?(rbs)
I just noticed that this bug also affects spaces on wrapped lines (at the point where the line wraps). This regressed one day earlier than the regression date for <br>s, due to the checkin for bug 235223.
Blocks: 235223
Summary: In designmode, caret moves incorrectly near whitespace just before <br> → In designmode, caret moves incorrectly near whitespace just before <br> or line wrap
> Note that I also had to fix the typo I mentioned in bug 129560 comment 24 for
> this to work (at least I'm pretty sure it was a typo).

How come I missed your comment about the typo?!?

> it assumed that the caret is after the space whenever
> it was after the last rendered character.

That was before the other "regressions" :-) (Why should the caret be in between letters if it is at the end of the line... it is a contradiction.)

BTW, I am wondering about bug 235223 (and bug 235223?) - Perhaps we only need that trimming in that case when text-align = right (treated as we do when maxWidth is reached), and this might change the whole picture, and possibly avoid the bug to begin with?
Both bug numbers in comment 16 are the same....

We should have the whitespace trimming no matter what text-align is; the whitespace can be detected easily no matter what the alignment is (e.g. with borders).
Oops, I meant that I was wondering about bug 132561 (and bug 235223? -- which was a reaction to it).

What I am suggesting is that there might perhaps be a different way to solve bug 132561 without the domino effect that we seem to be reacting to (then, we wouldn't just have a partial fix to a problem that wouldn't be there). We already know how to trim (in nsTextFrame) when maxWidth is reached, and that's the angle/option I am coming from. Perhaps there isn't another way to fix it. But if we put that option on the table, then we could think differently/afresh, which is what I am suggesting uriber to consider.

==========
While on this topic, I added the XXX comment that was removed in attachment 218946 [details] [diff] [review]. At the time, what I observed was that:
-  //XXX callers need to safeguard themselves against empty frames, I noted that
-  //the caret can be locked when leftarrow'ing in: <span>...</span>\n<br>line

That is, say feed in composer

<b>Hello World</b>\n
<br>
Hello Again World|

and start moving the caret (|) to the left, then it could get stucked/locked on the empty frame (<br>). And we were getting there because the caller was passing the empty brFrame -- if I remember correctly. I suspected at the time that the lock could be avoided by not calling the method when rect.width is 0 in the first place (i.e., if the caller was to check/know that the width is 0, it could deduce what it wanted the OUT params to be without getting there).
===========
(In reply to comment #18)
> What I am suggesting is that there might perhaps be a different way to solve
> bug 132561 without the domino effect that we seem to be reacting to 

I don't see a different way to fix bug 132561, but I admit that I really don't know that code very well. If you or bz can come up with an alternative that doesn't cause regressions, I wouldn't mind, of course.

I think I might be able to fix the rest of this bug by tweaking PeekOffset() to allow moving before and after the trimmed space. However, I'd rather do that after (some version of) my patch for bug 300131 lands. 

> 
> ==========
> While on this topic, I added the XXX comment that was removed in attachment
> 218946

I tried your testcase on a recent SeaMonkey nightly and I couldn't reproduce the problem you describe. In any event, I think that cases of the caret getting "stuck" are/were due to issues in PeekOffset, not in GetPointFromOffset (which only determines the geometric location where the caret is drawn, not its logical position). It's possible that this problem was corrected by some previous fix to PeekOffset.
Comment on attachment 236435 [details] [diff] [review]
[checked in] partial fix

r=rbs

(Your comment about the  origin of the caret getting stuck is right, it wasn't due to GetPointFromOffset() per se. Perhaps, a calling sequence involving the two. I can't quite recollect what was up. Anyway, nice to hear that it is gone.)

I thought carefully about this bug, and your patch seemed indeed as the solution that involves less surgery.

Note for future reference (when others try to make some sense of this code which is convoluted but necessary):
We start with

hitLength = ip[inOffset] - mContentOffset. 

This can end up with the _same_ actual value in two cases: when the caret ("inOffset") is:

1# on the character just before a space that has been trimmed, and 

2# on the trimmed space itself (remember that while it is trimmed in the rendered text on the screen, it isn't trimmed in the DOM text node where the "inOffset" comes from).

So what we want to do here is differentiate the two cases and add the width 
of the space back _only_ in the case #2 (so that caret uses the augmented width and
is painted visually on the spot of the trimmed space.) To do so we use the test

 ip[inOffset] == ip[inOffset-1]?

When this is true, we know that PrepareUnicodeText() did indeed collapse the DOM text character at inOffset, et voila. This, together with (TEXT_TRIMMED_WS & mState), which happens only at the end of line, we know for sure that we can add the width of the trimmed space back.
Attachment #236435 - Flags: review?(rbs) → review+
Attachment #236435 - Flags: superreview?(roc)
Comment on attachment 236435 [details] [diff] [review]
[checked in] partial fix

This code is changing in the new textframe so I'll have to check things carefully there. It is also getting simpler, thankfully.
Attachment #236435 - Flags: superreview?(roc) → superreview+
Comment on attachment 236435 [details] [diff] [review]
[checked in] partial fix

Checking in layout/generic/nsTextFrame.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v  <--  nsTextFrame.cpp
new revision: 1.595; previous revision: 1.594
Attachment #236435 - Attachment description: partial fix → [checked in] partial fix
Flags: blocking1.9a1? → blocking1.9+
Blocks: 363624
Blocks: 378061
Please also see Thunderbird compose bug 354778. Will the final check-in here likely fix that bug?
Blocks: 354778
Whiteboard: [dbaron-1.9:RuCo]
Okay, it's trivial to change textframe so that left- and right-arrow treat the trailing trimmed space as a real character. This fixes Uri's testcase, basically. This works only because new-textframe and mrbkap's caret work made life much better.

However, there's still at least one serious bug. Namely, pressing down-arrow or option-right-arrow (on Mac) leaves the caret at the end of the <body>. Then nsFrameSelection::GetFrameForNodeOffset decides to use the position of the <br> as the caret position. This, unfortunately, is before that trailing space. We really want it to be after that trailing space.

I'm not sure of the best way to fix this. Maybe the best way is to have nsFrameSelection::GetFrameForNodeOffset use the frame before the <br> instead, if there is one on the same line?
Assignee: nobody → roc
Attached patch fix (obsolete) — Splinter Review
This is a more complete fix. There are two parts:

-- In nsTextFrame::PeekOffsetCharacter, we refrain from trimming trailing whitespace at the end of the line just by changing the flags passed to GetTrimmedOffsets. This allows the caret to be moved to the point after that whitespace. nsTextFrame::GetPointFromOffset will return a suitable position for the caret even though it's outside the frame, and thanks to mrkbap's caret work in 1.9, we'll display the caret just fine.

-- A related problem immediately shows up: various caret movement operations, such as down-arrow, can position the caret at the <br> on a line. The <br> is positioned assuming trailing whitespace is trimmed, so positioning the caret at the <br> actually places it *before* where it appears when we position it at the end of trailing whitespace. This is trickier to fix. I've modified nsCaret::GetCaretFrameForNodeOffset so that if there's a text frame which is logically at the end of the line and it's before the caret frame in the line --- i.e. the caret is logically at the end of the line --- then we position the caret in that text frame, at its end, thus after any trailing whitespace.

Simon can check the textframe change, Blake the rest, thanks!
Attachment #286655 - Flags: superreview?(mrbkap)
Attachment #286655 - Flags: review?(smontagu)
Whiteboard: [dbaron-1.9:RuCo] → [dbaron-1.9:RuCo][needs review]
Attached patch fix v2 (obsolete) — Splinter Review
Added some tests and fixed a bug in the recursive call to CheckForTrailingTextFrameRecursive.
Attachment #286662 - Flags: superreview?(mrbkap)
Attachment #286662 - Flags: review?(smontagu)
Comment on attachment 286662 [details] [diff] [review]
fix v2

r=me on the textframe changes. I assume this supersedes the earlier patch?
Attachment #286662 - Flags: review?(smontagu) → review+
Comment on attachment 286655 [details] [diff] [review]
fix

oops yeah
Attachment #286655 - Attachment is obsolete: true
Attachment #286655 - Flags: superreview?(mrbkap)
Attachment #286655 - Flags: review?(smontagu)
Comment on attachment 286662 [details] [diff] [review]
fix v2

Nit: Prevailing style in nsCaret.cpp is { on its own line after if, etc.

sr=mrbkap with that fixed.
Attachment #286662 - Flags: superreview?(mrbkap) → superreview+
Whiteboard: [dbaron-1.9:RuCo][needs review] → [dbaron-1.9:RuCo][needs landing]
With mrbkap's changes
Attachment #286662 - Attachment is obsolete: true
Checking in layout/base/nsCaret.cpp;
/cvsroot/mozilla/layout/base/nsCaret.cpp,v  <--  nsCaret.cpp
new revision: 1.182; previous revision: 1.181
done
Checking in layout/generic/nsTextFrame.h;
/cvsroot/mozilla/layout/generic/nsTextFrame.h,v  <--  nsTextFrame.h
new revision: 3.13; previous revision: 3.12
done
Checking in layout/generic/nsTextFrameThebes.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrameThebes.cpp,v  <--  nsTextFrameThebes.cpp
new revision: 3.119; previous revision: 3.118
done
Checking in layout/generic/test/Makefile.in;
/cvsroot/mozilla/layout/generic/test/Makefile.in,v  <--  Makefile.in
new revision: 1.14; previous revision: 1.13
done
RCS file: /cvsroot/mozilla/layout/generic/test/test_character_movement.html,v
done
Checking in layout/generic/test/test_character_movement.html;
/cvsroot/mozilla/layout/generic/test/test_character_movement.html,v  <--  test_character_movement.html
initial revision: 1.1
done
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [dbaron-1.9:RuCo][needs landing] → [dbaron-1.9:RuCo]
Target Milestone: --- → mozilla1.9 M10
Depends on: 403004
Depends on: 403048
You need to log in before you can comment on or make changes to this bug.