305083 - [regression] When using ctrl+right-arrow on last word in a wrapping line, caret moves to end of line instead of to beginning of next line

Status: VERIFIED FIXED

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

Description

[Uri Bernstein]

This bug appears only when layout.word_select.eat_space_to_next_word is set to
true, i.e. by default only on Windows.

In a textarea, when text wraps from one line to the next (with no hard BR),
placing the caret on the last word of the overflowing line and pressing
ctrl+right-arrow moves the caret to the end of the line (after the space),
instead of to the beginning of the first word on the next line. The caret is
logically in the correct position, but is displayed wrongly due to a bad hint.

The same happens with HTML in caret browsing mode. I'll attach a testcase soon.

This regressed between 2005-08-16 and 2005-08-17, due to the fix for bug 299239.

Comment 1

[Uri Bernstein]

Attachment: test-case

Self-explaining.

Comment 2

[Ginn Chen]

Bad regression, we should back out bug 299239 and try an alternative fix.

Comment 3

[Eyal Rozenberg]

It's possible that what's happening is that PeekOffset() is being passed a
mPreferLeft==true with mDirection==eDirNext , which is something I had thought
doesn't happen. And the 299239 patch code overrides that.

Comment 4

[Uri Bernstein]

Ginn: I think we should give Eyal a chance to fix this before considering
backing out.

Eyal, unfortunately I'm not currently at a machine on which I can debug.
However, looking at the code (nsSelection.cpp), I can't see how that would happen:

1345     case nsIDOMKeyEvent::DOM_VK_RIGHT : 
1346         InvalidateDesiredX();
1347         pos.mDirection = eDirNext;
1348         tHint = HINTLEFT;//stick to this line

tHint is then assigned to pos.mPreferLeft, and nobody touches either of these
before calling PeekOffset(), as far as I can see.

Comment 5

[Eyal Rozenberg]

> However, looking at the code (nsSelection.cpp), I can't see how that would happen:

It seems GetFrameFromDirection() changes mPreferLeft when it's called by the
PeekOffset() of the first line frame to get the second line frame. I'm thinking
about what I can do with that.

Comment 6

[Eyal Rozenberg]

Attachment: patch

Damn, this mPreferLeft business is a horrible mess. The patch seems to fix the
regression without undoing the effects of the 299239 patch; I've tweaked the
comments a bit too. It seems to me now that, in fact, the PeekOffset() method
does not ever _use_ mPreferLeft, not even through calls to other methods, but
rather only _sets_ it for use by others. In this respect consider the code in
MoveCaret()  (nsSelection.cpp line 1397) which after calling PeekOffset()
resets the hints in some cases because it "might have been reversed by an RTL
frame". Could there not be a way to set the hint just once, outside the call to
nsPeekOffset? I wonder. Anyway, it's all a mess, this code needs refactoring
badly (see specifically bug 300131).

Comment 7

[Uri Bernstein]

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

Oh well. I gave up on understanding how the word-moving code is *supposed* to
work, so I just resorted to doing extensive testing verifying that it does, in
fact, work with this patch (with both settings of
layout.word_select.eat_space_to_next_word). I found no regressions (although
the bidi - or even just RTL - case is still broken).

This is however, far from elegant, and since it leaves bidi word-moving broken
anyway, I would perhaps consider as an alternative just going back to my own
patch from bug 299239 (but adding Eyal's comments). That patch doesn't touch
the "word" case at all, so it might be considered safer.

Comment 8

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

Bug 299239 was not a regression, right? I'm tempted to just back that fix out
and leave it unfixed on branch (and fix it on trunk in 300131).

Comment 9

[Uri Bernstein]

299239 wasn't a regression, yet it was (IMO) the most major bug hindering bidi
editing. That's why it was the first bidi bug I worked on, and that's why I
proposed a hackish (but safe) patch to it - I wanted to make sure it makes it to
1.8.

So I'd really be sorry to lose it. I'd even go as far as saying that bug 299239
is a much more severe bug than this one (which it pretty much cosmetic), so even
weighing in the fact that it only affects bidi, if I had to choose I'd prefer
having it fixed than having this one fixed (of course, I'm biased).

All in all, I think our best option is to accept Eyal's patch for this bug.

Comment 10

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

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

alright. Let's do it. But if the going gets tough, we may have to roll things
back including possibly 299239.

Comment 11

[Uri Bernstein]

If anything, this is blocking 299239, not depending on it.

Comment 12

[Ginn Chen]

(In reply to comment #11)
> If anything, this is blocking 299239, not depending on it.

IMHO, this bug's patch is based on bug 299239, so it depends on bug 299239.

Comment 13

[Uri Bernstein]

(In reply to comment #12)
> (In reply to comment #11)
> > If anything, this is blocking 299239, not depending on it.
> 
> IMHO, this bug's patch is based on bug 299239, so it depends on bug 299239.

The convention is that when a fix to one bug causes a regression, the original
bug is marked as "depending" on the regression (so the regression blocks the
original). 

See for example the "depends on" list on bug 296639, which are mostly
regressions from that bug.

Comment 14

[Asa Dotzler [:asa]]

please get this in on the trunk and we'll re-evaluate for approval after trunk
landing and verification.

Comment 15

[Asaf Romano (gone)]

Checking in layout/generic/nsTextFrame.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v  <--  nsTextFrame.cpp
new revision: 1.517; previous revision: 1.516
done

Comment 16

[Asa Dotzler [:asa]]

let's get this verified on the trunk and if dbaron likes the patch, we can
approve for the branch.

Comment 17

[Uri Bernstein]

(In reply to comment #16)
> let's get this verified on the trunk and if dbaron likes the patch, we can
> approve for the branch.

Verified fixed with
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20050826
Firefox/1.6a1
(with layout.word_select.eat_space_to_next_word set to "true").

Comment 18

[Asaf Romano (gone)]

1.8 Branch:
Checking in layout/generic/nsTextFrame.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v  <--  nsTextFrame.cpp
new revision: 1.513.4.2; previous revision: 1.513.4.1
done

Comment 19

[Tracy Walker [:tracy]]

verified on Firefox 1.4 -mozilla1.8 branch- Win, Lin and Mac : 2005-09-07

Comment 20

[Uri Bernstein]

This caused bug 307934, which is very major. I really should have cought this
earlier (preferably when I reviewed the patch). Sorry.

Comment #10 might apply now, sadly.