256252 - ctrl+right arrow sometimes skip a word

Status: RESOLVED FIXED

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

Description

[Ginn Chen]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a3) Gecko/20040818
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a3) Gecko/20040818

go to http://www.mozilla.org/start/1.7/
turn on caret browsing
put caret in "Getting" of "Getting Started"
Press ctrl+right, caret move to end of word "Getting".
Press ctrl+right again, caret goes to next line


Reproducible: Always
Steps to Reproduce:

Comment 1

[Ginn Chen]

Attachment: test case

simple test case

Comment 2

[Radovan Bukoci]

Maybe, this bug would dissapear, if Ctrl+right would be stoping at beginings of
words (not at ends) (I wrote about this in discussion to bug 142120). But this
is only my suggestion.

Comment 3

[Ginn Chen]

(In reply to comment #2)
> Maybe, this bug would dissapear, if Ctrl+right would be stoping at beginings 
of
> words (not at ends) (I wrote about this in discussion to bug 142120). But this
> is only my suggestion.

Yes, it would disappear, but it's not linux style, try gedit, emacs.

Comment 4

[Ginn Chen]

Attachment: patch

"aPos->mEatingWS = isWhitespace;" is overwriting "aPos->mEatingWS =
!sWordSelectEatSpaceAfter;"
so I move it into else-clause.

Comment 5

[Ginn Chen]

Comment on attachment 157138 [details] [diff] [review]
patch

Aaron, would you please test it on Windows to make sure I'm not breaking?
Thanks.

Comment 6

[[e-mail]]

Can't you test it by changing the work breaking preference,
layout.word_select.stop_at_punctuation?

Comment 7

[Ginn Chen]

(In reply to comment #6)
> Can't you test it by changing the work breaking preference,
> layout.word_select.stop_at_punctuation?

It doesn't work well for me.
Maybe bug 193025.

Comment 8

[[e-mail]]

(In reply to comment #7)
>(In reply to comment #6)
>>Can't you test it by changing the work breaking preference,
>>layout.word_select.stop_at_punctuation?
>It doesn't work well for me.
>Maybe bug 193025.
Ah, I see what you mean, the preference only seems to work for ctrl+left, not
ctrl+right. Perhaps another manifestation of the same bug?

Comment 9

[[e-mail]]

In fact, neither my windows or linux nightlies ctrl+right correctly. Not only do
they both show this bug, neither of them respects the word select preference.

ctrl+left is fine on both platforms, both preference settings.

Comment 10

[Aaron Leventhal]

Comment on attachment 157138 [details] [diff] [review]
patch

* I can't reproduce the original bug on Windows
* I can't see any bad effects from the patch on Windows, with either
punctuation setting
* There is a different problem on Windows. Ctrl+right stops twice before the
first character of every line. However, that seems to happen with or without
this patch. Do we have a bug filed for that?

Comment 11

[Ginn Chen]

Ctrl+right still has some problem even with this patch. I'm on Linux now.
1) It will stop at first letter of next line.
2) If there's some   ctrl+right will stop several times.

Comment 12

[Ginn Chen]

Attachment: patch v2

This patch works quite fine for me on Linux.
The other 2 problems are solved.
But I don't know if it works on Windows, and RTL language.
I'll try to test it tommrrow.

I really don't know why eSelectWord + eDirNext need set Amount to NoAmount.
It will cause a stop at line beginning.

Comment 13

[[e-mail]]

Hmm... this patch does seem to help on Linux, but not on Windows.

CC'ing someone who worked on this before to see if they can shed any light.

Comment 14

[Ginn Chen]

http://bugzilla.mozilla.org/attachment.cgi?id=146241&action=view
This test case still fails... blank <B></B> causes trouble.
I'm afraid to fix it will cause regression.

Comment 15

[Ginn Chen]

I'm trying to fix windows.
It seems that GetPrevWord/GetNextWord are not symmetry with win style.

Comment 16

[Ginn Chen]

I'm trying to simplify nsTextFrame::PeekOffset's eSelectWord section.
It needn't to be as complex as current.

But because we couldn't get information about where there's a punctuation at
begin/end of a frame, we don't know whether we need stop.
Example:
abc+abc+<b>efg</b>
abc+abc<b>efg</b>
are always having same stoppings.
This is boring.

Because the word is crossing nsFrames, and in PeekOffset() we go to next nsFrame
by recursion, nsTextTransformer is treating them as 2 words.

On linux, we usually set "stop_at_punctuation" to false, we don't have this
issue, we just care about white_space.

What's your thoughts, Neil?

Comment 17

[Ginn Chen]

Attachment: patch v3

this patch works for me on both Windows and Linux;
because of bug 237228, user can't switch to stop at beginning of next word or
end of this word now.
for abc+abc+<b>efg></b>, abc+abc<b>efg></b> issue, I just follow current
behaviour of ctrl+left.

Comment 18

[Ginn Chen]

Comment on attachment 158293 [details] [diff] [review]
patch v3

review? roc

Comment 19

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

I'm lost. Can you carefully explain what was wrong with the logic in the
existing code and how you've fixed it?

Comment 20

[Ginn Chen]

Attachment: linux/unix test cases

linux/unix test cases
(set layout.word_select.eat_space_to_next_word to false)

Comment 21

[Ginn Chen]

Attachment: windows test cases

windows test cases
(set layout.word_select.eat_space_to_next_word to true)

Comment 22

[Ginn Chen]

roc,
I've created test cases to show what's the problems with exsiting code.
I'll explain my patch later.

But without getting bug 237228 fixed.
We need to test it on windows/linux separately.
So I suggest to get bug 237228 fixed before this one.
(and the patches have conflicts)

BTW: I found a regression of my last patch with www.nytimes.com.
caret get stuck before a certain image when pressing ctrl+right.
It is caused by removing aPos->mAmount = eSelectNoAmount; in
nsFrame::GetFrameFromDirection.
I've a workaround in nsFrame::PeekOffset.
I'll post it later.

Comment 23

[Ginn Chen]

get roc in.

patch v3 didn't solve Case 6:empty block "<b></b>" in word
I'll fix it later.

Comment 24

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

Comment on attachment 158293 [details] [diff] [review]
patch v3

Minusing to get it off my queue until it gets fixed

Comment 25

[Ginn Chen]

Attachment: patch v4

after apply this patch, all test cases work well on both linux and windows.

Comment 26

[Ginn Chen]

description of my patch,

Part1 nsFrame::PeekOffset
aPos->mAmount equals to eSelectCharcter or eSelectWord.
Here is the logic, caret enters non-textframe after leaving a textframe.
The non-textframe usually is an image or blankblock(<b></b>, <b><i></i></b>).
I removed the comparision between newOffset and aPos->mStartOffset,
And bring the else-clause ahead.
Because newOffset is index of mContent in its parent.
    nsIContent* newContent = mContent->GetParent();
    PRInt32 newOffset = newContent->IndexOf(mContent);
It doesn't make sense to compare it with aPos->mStartOffset.
We should just assign it to aPos->mContentOffset according to mDirection.

FYI:
In bug 240665, I added return NS_OK; into else-clause.
Sometimes, just press arrowkey again, caret may get through.
(the runpath is get aPos->mContentOffset assigned by else-clause,
 next time it will go into then-caluse)
But it's not the approach. We still have bug 259984.
After this patch, bug 259984 will gone.

Part2 nsFrame::GetFrameFromDirection
Because we've fixed Part1, we don't need set aPos->mAmount to eSelectNoAmount 
now.

Part3: nsTextFrame::PeekOffset
The most complexe part. I wish I can help you understand the code.
We've 2 styles,
style 1) wordSelectEatSpaceAfter is true, we'll stop at beginning of next word.
style 2) wordSelectEatSpaceAfter is false, we'll stop at end of word.
aPos->mEatingWS is a mark of we're eating space after a word.
The logic for style 1), 
go ahead to find whitespace, then set aPos->mEatingWS to true.
if aPos->mEatingWS is true, and we find !isWhitespace, stop immediately.
The logic for style 2),
go ahead to find !isWhitespace, then set aPos->mEatingWS to true.
(we think we've already eaten enough spaces)
if aPos->mEatingWS is true, and we find space agian, stop immediately.

Comment 27

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

Comment on attachment 159703 [details] [diff] [review]
patch v4

I have to say I don't really understand this code. Maybe you're the only one
who does :-). But as far as I understand it, it looks OK.

Comment 28

[Kyle Yuan]

Fixed.

Checking in nsFrame.cpp;
/cvsroot/mozilla/layout/html/base/src/nsFrame.cpp,v  <--  nsFrame.cpp
new revision: 3.521; previous revision: 3.520
done
Checking in nsTextFrame.cpp;
/cvsroot/mozilla/layout/html/base/src/nsTextFrame.cpp,v  <--  nsTextFrame.cpp
new revision: 1.479; previous revision: 1.478
done

Comment 29

[Ginn Chen]

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

Comment 30

[Uri Bernstein]

This caused bug 304891.

Comment 31

[Uri Bernstein]

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

Comment 32

[Uri Bernstein]

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

Comment 33

[Wayne Mery (:wsmwk)]

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

Comment 34

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381