[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

VERIFIED FIXED in mozilla1.8beta4

Status

()

Core
Keyboard: Navigation
VERIFIED FIXED
13 years ago
13 years ago

People

(Reporter: Uri Bernstein (Google), Assigned: Eyal Rozenberg)

Tracking

({regression, verified1.8})

1.8 Branch
mozilla1.8beta4
regression, verified1.8
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

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

Comment 1

13 years ago
Created attachment 193044 [details]
test-case

Self-explaining.

Comment 2

13 years ago
Bad regression, we should back out bug 299239 and try an alternative fix.
Depends on: 299239
(Assignee)

Comment 3

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

Comment 4

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

Comment 5

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

Comment 6

13 years ago
Created attachment 193151 [details] [diff] [review]
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).
Attachment #193151 - Flags: superreview?(roc)
Attachment #193151 - Flags: review?(uriber)
(Reporter)

Comment 7

13 years ago
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.
Attachment #193151 - Flags: review?(uriber) → review+
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).
(Reporter)

Comment 9

13 years ago
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.
Attachment #193151 - Flags: superreview?(roc)
Attachment #193151 - Flags: superreview+
Attachment #193151 - Flags: approval1.8b4?
(Reporter)

Comment 11

13 years ago
If anything, this is blocking 299239, not depending on it.
Blocks: 299239
No longer depends on: 299239

Comment 12

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

Comment 13

13 years ago
(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

13 years ago
please get this in on the trunk and we'll re-evaluate for approval after trunk
landing and verification.
Whiteboard: waiting for dbaron's analysis
Checking in layout/generic/nsTextFrame.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v  <--  nsTextFrame.cpp
new revision: 1.517; previous revision: 1.516
done
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Flags: blocking1.8b4?
Resolution: --- → FIXED

Updated

13 years ago
Flags: blocking1.8b4? → blocking1.8b4+

Comment 16

13 years ago
let's get this verified on the trunk and if dbaron likes the patch, we can
approve for the branch.
(Reporter)

Comment 17

13 years ago
(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").
Status: RESOLVED → VERIFIED
Attachment #193151 - Flags: approval1.8b4? → approval1.8b4+
Whiteboard: waiting for dbaron's analysis
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
Target Milestone: --- → mozilla1.8beta4
verified on Firefox 1.4 -mozilla1.8 branch- Win, Lin and Mac : 2005-09-07
Keywords: fixed1.8 → verified1.8
(Reporter)

Updated

13 years ago
Depends on: 307934
(Reporter)

Comment 20

13 years ago
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.
You need to log in before you can comment on or make changes to this bug.