423523 - Caret can leave div w/ contenteditable = true with repeated arrow keys

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Robbie Paplin]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b4) Gecko/2008030714 Firefox/3.0b4
Build Identifier: Firefox/3.0b4

The page listed above has a DIV with a contenteditable = true attribute. The
DIV contains several spans with a contenteditable = false attribute. When I set the caret to an editable region inside the To field (a/k/a DIV with contenteditable = true), and repeatedly press the left arrow key, the caret doesn't move like I expect it should.


Reproducible: Always

Steps to Reproduce:
1. Set the caret to a semicolon between names in the To... field
2. Press the left arrow key repeatedly until the caret stops moving
3. Note where the caret stops moving
4. Set the caret to a semicolon between names in the To... field (like you did in Step 1)
5. Press the left arrow key repeatedly until the caret stops moving
6. Note where the caret stops moving
Actual Results:  
At step 3, the caret stops moving before it hits the beginning of the contenteditable DIV.

At step 6, the caret keeps moving outside of the contenteditable DIV.

Expected Results:  
The caret should stop moving at the begining of the contenteditable DIV.

The caret should move left one character when the left arrow key is pressed and the caret is in the contenteditable div. If the the caret is to the right of a
non-contenteditable span (a/k/a a resolved address) it should skip over the span (like it skips over a character). The caret should keep moving to the left until it is at the beginning of the contenteditable div. After reaching the div boundary, additional left arrow key events should be ignored (like a <input type=text> or <textarea> element would act).

Comment 1

[Robbie Paplin]

Attachment: Test document to verify caret inside a contenteditable DIV doesn't handle arrow key navigation correctly

Comment 2

[Ria Klaassen (not reading all bugmail)]

Confirmed on Windows XP, latest trunk. Regression window is
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1204075680&maxdate=1204078259
Bug 394473 ?

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

You can also repoduce this by simply double-clicking in the contentEditable div, and then pressing the arrow keys to move caret outside of the contentEditable div.

Here's what I think is happening...

The problem here was not caused directly by the patch for bug 394472 per se, it is just more obvious because of it. 394473 changed the default value for nsCaret::mIgnoreUserModify, which causes the caret to show up here. But the root problem is that the caret itself can leave the contentEditable area. If I back out the patch for 394473, and I position the caret at the far left of the div, I can press the right arrow key X times and the caret stays drawn where it is. However I have to press the right key X+1 times before it moves to the right. So the caret is being moved around, just not drawn as it's moved. 394473 didn't cause the bug it just showed it.

So... nsPresShell::CharacterMove() calls into nsFrameSelection::MoveCaret(), which calls into nsIFrame::PeekOffset() which calls into nsIFrame::GetFrameFromDirection(). The problem is that GetFrameFromDirection() is navigating into the frame before the contentEditable <div>.

When we press the left arrow key for a text input with the caret hard-left, GetFrameFromDirection() actually fails, preventing the caret from moving outside it. I'll see if I can figure out how to get a similar result for a contentEditable div...

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #3)
> and I position the caret at the far left of the
> div, I can press the right arrow key X times

Oops, I meant to say "press the LEFT arrow key X times".

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1

Ok, so the problem here is that when you click on the contentEditable div the second time, nsFrameSelection::HandleClick() is called. This resets the ancestor limiter of the selection. Then when nsFrameSelection::MoveCaret() is called on key-press after the second click, it calls into nsFrameSelection::TakeFocus() which calls into IsValidSelectionPoint(), which sees a null ancestor limiter. Thus it assumes the <body> is then the ancestor limiter, and allows the caret to move outside the <div>. So the fix is simple, don't reset the ancestor limiter in HandleClick().

I'm not sure why we were resetting the ancestor limiter in HandleClick(), but if we do, it means the caret won't be limited to remain in whatever element it was in. I guess this wasn't picked up because the text editor focus handler was setting an ancestor limiter on the first click, and the caret wasn't visible when it left its div until after bug 394473 was checked in (though it was still leaving the div, you just couldn't see that it was).

This patch removes the reset of mAncestorLimiter in nsFrameSelection::HandleClick(). Unfortunately I don't have a mochitest, I tried to make one, but couldn't get the caret to show up inside an iframe in a mochitest with synthesized events. However with this patch, the existing mochitests pass on Mac and Linux.

This is a pretty simple fix, and it fixes a bug which affect potentially any page which contains a content-editable area, it would be good to see if we can get this into FF3.

Comment 6

[Peter Van der Beken [:peterv]]

Comment on attachment 315036 [details] [diff] [review]
Patch v1

The issue with this patch is that when you click in a contenteditable region and then click in a different contenteditable region the caret will be at the start of the new contenteditable region, not where you clicked. Not sure if HandleClick is really the right place to reset mAncestorLimiter, but I don't know of a better place. What does seem to work in my limited testing is only resetting mAncestorLimiter if !IsValidSelectionPoint(this, aNewFocus), but it'd be good if you could verify that that doesn't break anything else :-/.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: v2

(In reply to comment #6)
> What does seem to work in my limited testing is only
> resetting mAncestorLimiter if !IsValidSelectionPoint(this, aNewFocus), but it'd
> be good if you could verify that that doesn't break anything else :-/.
> 

I've tested this approach, and it doesn't seem to break anything. You can focus two different contentEditable areas, and it places the caret correctly for each. I've also included a mochitest, and all mochitests pass on Mac with this.

Comment 8

[Peter Van der Beken [:peterv]]

Comment on attachment 316165 [details] [diff] [review]
v2

Do we really need all the timeouts for the test?

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: V2.1

(In reply to comment #8)
> (From update of attachment 316165 [details] [diff] [review])
> Do we really need all the timeouts for the test?
> 

Um... no. :) Here's v2 patch without excessive timeouts in the mochitest.
(r+/sr+ peterv).

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 316296 [details] [diff] [review]
V2.1

This patch fixes a bug where if you click twice in a contentEditable area, the caret is able to navigate out of the area. It would be embarrassing to ship with this bug, so requesting approval1.9. Patch has mochitest, fairly low risk of regressions.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 316296 [details] [diff] [review]
V2.1

a1.9=beltzner

Comment 12

[Dan POPA]

(In reply to comment #10)
> (From update of attachment 316296 [details] [diff] [review])
> This patch fixes a bug where if you click twice in a contentEditable area, the
> caret is able to navigate out of the area. It would be embarrassing to ship
> with this bug, so requesting approval1.9. Patch has mochitest, fairly low risk
> of regressions.
> 

How about bug 414526 ?

Comment 13

[:Gavin Sharp [email: [e-mail]]]

mozilla/layout/base/tests/Makefile.in 	1.63
mozilla/layout/base/tests/test_bug423523.html 	1.1
mozilla/layout/generic/nsSelection.cpp 	3.311