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

RESOLVED FIXED

Status

()

Core
Keyboard: Navigation
--
minor
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Uri Bernstein (Google), Assigned: Ginn Chen)

Tracking

({fixed1.8, regression})

Trunk
fixed1.8, regression
Points:
---
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

4.66 KB, patch
Uri Bernstein (Google)
: review+
Mike Schroepfer
: approval1.8b5+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
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.

Updated

12 years ago
Keywords: helpwanted
(Assignee)

Comment 1

12 years ago
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.
Assignee: aaronleventhal → ginn.chen
(Reporter)

Comment 2

12 years ago
"left"=>"right" in summary. That should show me not to file bugs at 2 AM.
Summary: Using ctrl+left arrow in textarea, first word after hard line break is skipped → Using ctrl+right arrow in textarea, first word after hard line break is skipped
(Assignee)

Comment 3

12 years ago
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?
(Reporter)

Comment 4

12 years ago
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()
Possibly we should just back out 256252. Which bug is worse, this one or that one?
(Reporter)

Comment 6

12 years ago
(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?
(Assignee)

Comment 7

12 years ago
Created attachment 193043 [details] [diff] [review]
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?
(Reporter)

Comment 8

12 years ago
(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.
(Assignee)

Comment 9

12 years ago
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.
(Reporter)

Comment 10

12 years ago
(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.
(Reporter)

Comment 11

12 years ago
> 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.
(Assignee)

Updated

12 years ago
Attachment #193043 - Flags: superreview?(roc)
Attachment #193043 - Flags: review?(uriber)
(Reporter)

Comment 12

12 years ago
Comment on attachment 193043 [details] [diff] [review]
patch v0

r+, per comment #8.
Attachment #193043 - Flags: review?(uriber) → review+
(Reporter)

Comment 13

12 years ago
*** Bug 289708 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 14

12 years ago
*** Bug 279959 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 15

12 years ago
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.
Flags: blocking1.8b4?

Updated

12 years ago
Severity: normal → minor
Flags: blocking1.8b4? → blocking1.8b4+

Updated

12 years ago
Whiteboard: [needs SR roc]
Comment on attachment 193043 [details] [diff] [review]
patch v0

rubber-stamping
Attachment #193043 - Flags: superreview?(roc) → superreview+
(Reporter)

Updated

12 years ago
Attachment #193043 - Flags: approval1.8b5?
(Assignee)

Comment 17

12 years ago
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
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [needs SR roc]

Comment 18

12 years ago
Comment on attachment 193043 [details] [diff] [review]
patch v0

Approved for 1.8b5 per bug meeting
Attachment #193043 - Flags: approval1.8b5? → approval1.8b5+
(Assignee)

Comment 19

12 years ago
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
Keywords: helpwanted → fixed1.8
(Reporter)

Updated

11 years ago
Blocks: 344417
You need to log in before you can comment on or make changes to this bug.