304891 - Using ctrl+right arrow in textarea, first word after hard line break is skipped

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Uri Bernstein]

Steps to reproduce:
1. In any textarea (such as the one on this page), type a few words, then Enter,
then a few more words.
2. Place the caret on the first line
3. Use ctrl+right arrow to move across words
4. after reaching the beginning of the last word on the first line, hittingh
ctrl+right again skips the first word on the second line, and places the caret
at the beginning of the second word on the second line.

This should be reproducable on Windows and Linux with default settings. On Mac
you need to set layout.word_select.eat_space_to_next_word to true.

This is a regression from bug 256252.

Comment 1

[Ginn Chen]

It seems testcases in bug 256252 are not enough.
They didn't cover textareas.
In webpage, it is fine.

BTW:
On Linux and Unix, the default setting of layout.word_select.eat_space_to_next_word is false.

Comment 2

[Uri Bernstein]

"left"=>"right" in summary. That should show me not to file bugs at 2 AM.

Comment 3

[Ginn Chen]

After a rough trace, I found the problem is how the text is handled by nsTextFrame.

For example, put next 2 lines into a txt file.
abc def
ghi jkl

Load the txt file, turn caret browsing on, put caret before 'd', press ctrl+right.
tx.GetNextWord is called by nsTextFrame::PeekOffset.
The wordlen result is 3, that is 'def'.
Next, GetNextWord is called again, the wordlen result is 1, a white space.
Then, aPos->mEatingWS is set true, call GetFrameFromDirection, and aPos->mResultFrame-
>PeekOffset() to interate PeekOffset.
Finally, caret goes to the position before 'g'.

HTML is different.
Try following code in a HTML file.
abc def<br>
ghi jkl<br>
Also put caret before 'd' and press ctrl+right.
tx.GetNextWord is called by nsTextFrame::PeekOffset.
The wordlen result is 3, that is 'def'.
The GetFrameFromDirection is called and aPos->mResultFrame->PeekOffset() to interate PeekOffset.
Next, GetNextWord is called, the wordlen result is 1, a white space, (I don't know why there is a white 
space in this text frame.)
aPos->mEatingWS is set true, GetNextWord is called again, the wordlen result is 3, that is 'ghi'.
Finally, caret goes to the position before 'g'.

This bug only happen in textarea.
Same testcase.
tx.GetNextWord is called by nsTextFrame::PeekOffset.
The wordlen result is 3, that is 'def'.
The GetFrameFromDirection is called and aPos->mResultFrame->PeekOffset() to interate PeekOffset.
Next, GetNextWord is called, the wordlen result is 3, that is 'ghi'.
aPos->mEatingWS is false now, GetNextWord is called again, the wordlen result is 1, a whitespace.
aPos->mEatingWS is true now, GetNextWord is called again, the wordlen result is 3, 'jkl'.
And the caret is put before 'j'.

So the problem is nsTextFrame::PeekOffset doesn't notice a whitespace when line breaks in textarea.
I wonder why GetNextWord return a whitespace in webpages but not in textarea.
Any idea?

Comment 4

[Uri Bernstein]

The word-moving (ctrl-arrow) code is truly a riddle(1) wrapped in a mystery(2)
inside an enigma(3). I've been trying to figure it out for the last couple of
hours and all I can say is that it is a wonder to me that it even works most of
the time.
I'll keep trying, but I'm really close to giving up.

(1) nsFrame::GetFrameFromDirection()
(2) nsFrame::PeekOffset()
(3) nsTextFrame::PeekOffset()

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Possibly we should just back out 256252. Which bug is worse, this one or that one?

Comment 6

[Uri Bernstein]

(In reply to comment #5)
> Possibly we should just back out 256252. Which bug is worse, this one or that one?

I would (hesitantly) say this one is worse, simply because editing text in a
textarea is (i think) much more common than using caret browsing or design mode.

That said, the patch to bug 256252 actually fixed a whole bunch of problems.
Perhaps we can back out only parts of it (which caused this regression) and get
to keep some of the fixes? Ginn - do you think that's possible?

Comment 7

[Ginn Chen]

Attachment: patch v0

Let BRFrame mark itself as whitespace in this case.
Patch nsFrame::PeekOffset() so that the caret will not be locked in BR.

I have done a rough test, it doesn't break bug 256252.
Uri, would you please doublecheck?

roc, do you have comments on this patch?

Comment 8

[Uri Bernstein]

(In reply to comment #7)
> Uri, would you please doublecheck?

Yes, this seems to fix the problem. I checked both TEXTAREAs and HTML (in caret
mode), with both settings of layout.word_select.eat_space_to_next_word, going
both backwards and forwards. I did not find any regressions caused by this patch.

I tried the same idea yesterday (setting aPos->mEatingWS = PR_TRUE in
BRFrame::PeekOffset), but I didn't manage to fix the problems this created like
you did.

I still think this code is terribly hackish and should be rewritten, but for the
Ff 1.5 release I think this patch is good enough.

While testing, I did find two issues (both when
layout.word_select.eat_space_to_next_word == true):

1. The problem (as described in comment #0) still exists with RTL text in an RTL
textbox (when using ctrl+left-arrow, the first [rightmost] word on the next line
is skipped). However, this problem existed before bug 256252 was fixed, so it's
not a regression from that bug.

2. When using ctrl+right-arrow on the last word of a wrapping line (no hard BR),
either in a textbox or in HTML, the caret appears at the end of the line instead
of on the first word in the next line (i.e., the hint is wrong). This is a
regression from Firefox 1.0, but it was not caused either by the patch to bug
256252 or by this patch.

I'll file separate bugs for these two issues.

Comment 9

[Ginn Chen]

Uri, I can't reproduce the second issue you mentioned (ctrl+right with wrapping lines) with nightly build.
Can you provide a testcase when you filing it?

The PeekOffset and GetPrevNext... code is buggy but works like a miracle.
I spent monthes on it, but didn't find a way to let it more readable.

BTW: If you test caret movement with other products, e.g. Microsoft's Office, you may also find some bugs.

Comment 10

[Uri Bernstein]

(In reply to comment #9)
> Uri, I can't reproduce the second issue you mentioned (ctrl+right with
wrapping lines) with nightly build.
> Can you provide a testcase when you filing it?

This regressed in yesterday's (2005-08-17) build, as a result of fixing bug
299239. I'm filing a bug on that right now, and I'll CC you (I will also attach
a testcase). I now realize it has nothing to do with this bug - I just happened
to find it when testing this one.

Comment 11

[Uri Bernstein]

> 1. The problem (as described in comment #0) still exists with RTL text in an RTL
> textbox (when using ctrl+left-arrow, the first [rightmost] word on the next line
> is skipped). However, this problem existed before bug 256252 was fixed, so it's
> not a regression from that bug.

Forget about this (for now). This was actually fixed as a side-effect Ginn's fix
to bug 295142, but I didn't have that fix in my debug build when testing the
current patch.

Comment 12

[Uri Bernstein]

Comment on attachment 193043 [details] [diff] [review]
patch v0

r+, per comment #8.

Comment 13

[Uri Bernstein]

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

Comment 14

[Uri Bernstein]

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

Comment 15

[Uri Bernstein]

Hmmm... this somehow got forgotten, I guess. Requesting blocking1.8b4 to get it
on the radar. 

This is a regression since Firefox 1.0, and we have a patch.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 193043 [details] [diff] [review]
patch v0

rubber-stamping

Comment 17

[Ginn Chen]

Checking in nsBRFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBRFrame.cpp,v  <--  nsBRFrame.cpp
new revision: 1.51; previous revision: 1.50
done
Checking in nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.591; previous revision: 3.590
done
Checking in nsTextTransformer.h;
/cvsroot/mozilla/layout/generic/nsTextTransformer.h,v  <--  nsTextTransformer.h
new revision: 1.43; previous revision: 1.42
done

Comment 18

[Mike Schroepfer]

Comment on attachment 193043 [details] [diff] [review]
patch v0

Approved for 1.8b5 per bug meeting

Comment 19

[Ginn Chen]

Checking in nsBRFrame.cpp;
/cvsroot/mozilla/layout/generic/nsBRFrame.cpp,v  <--  nsBRFrame.cpp
new revision: 1.50.28.1; previous revision: 1.50
done
Checking in nsFrame.cpp;
/cvsroot/mozilla/layout/generic/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.574.2.4; previous revision: 3.574.2.3
done
Checking in nsTextTransformer.h;
/cvsroot/mozilla/layout/generic/nsTextTransformer.h,v  <--  nsTextTransformer.h
new revision: 1.41.12.1; previous revision: 1.41
done