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.

Self-explaining.

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

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.

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.

> 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.

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 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.

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).

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 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.

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

(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.

(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.

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

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

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

(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").

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

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

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.