512295 - Cursor becomes stuck when going down in a contenteditable <div> containing a multi-line <p>

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Justin Lebar (not reading bugmail)]

Attachment: Testcase

STR:
 * Load attached page
 * Place the cursor after the A
 * Press <down> twice on the keyboard
 * Press <up> on the keyboard

Actual result:
 Cursor doesn't move up.

Expected result:
 Cursor should move up.

Reproducible:
  Always.

Comment 1

[Justin Lebar (not reading bugmail)]

NB: The cursor also gets stuck if you

 * Load attached page
 * Place the cursor after the A
 * Press <up> twice on the keyboard

Comment 2

[Mats Palmgren (inactive)]

The bug does not occur in 1.9.1, but it does occur in 1.9.2.
In 1.9.1 the caret is placed above/below the text, the "getting stuck" bug
is probably a regression from fixing that problem.

Comment 3

[Justin Lebar (not reading bugmail)]

It looks like there are at least two separate regression ranges here.

* In the 2009-06-20 nightly and earlier, the caret was placed above/below the text.

* Starting in the 2009-06-21 nightly, <up> moves the caret to the top left of the container, and <down> moves the caret below the container.  The cursor doesn't get stuck after pressing <up> on the first line, but it does take two <down>s or three <right>s to get the cursor to move again.  It's almost as though the cursor is logically above the line but being drawn at the beginning of the line.

* Starting in the 2009-06-24 nightly, we get the behavior we currently have.

I'll try and ID the associated changesets.

Comment 4

[Justin Lebar (not reading bugmail)]

hg bisect says the change that caused the second bullet point above was http://hg.mozilla.org/mozilla-central/rev/94a7af7250dd

Comment 5

[Justin Lebar (not reading bugmail)]

The change which caused the third bullet point appears to be http://hg.mozilla.org/mozilla-central/rev/c6c685aa0379

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

The selection/caret code depends on having frames so don't inhibit
frame creation for editable content, for now.

Comment 7

[Mats Palmgren (inactive)]

Using blocking flag to say status1.9.2=affected

Comment 8

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

 nsCSSFrameConstructor::FrameConstructionItem::IsWhitespace() const
 {
   NS_PRECONDITION(!mContent->GetPrimaryFrame(), "How did that happen?");
   if (!mIsText) {
     return PR_FALSE;
   }
   mContent->SetFlags(NS_CREATE_FRAME_IF_NON_WHITESPACE |
                      NS_REFRAME_IF_WHITESPACE);
-  return mContent->TextIsOnlyWhitespace();
+  return !NeedTextFrameFor(mContent);

I don't think IsWhitespace should depend on IsEditable. Should we call this IsIgnorableWhitespace or something like that?

I think this means we need to reframe if the editability of the text changes.

What parf of selection/caret depends on having a textframe here? That seems ... annoying.

Comment 9

[Mats Palmgren (inactive)]

For the record, it was nsFrameSelection::GetFrameForNodeOffset() that bails
when not finding a frame for the text node here:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp#2070

Comment 10

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

This is more targeted fix at the specific bug.  In PresShell::CompleteMove()
there's a special hack for non-document-root editable elements.
It comes from bug 386300, bug 394264, bug 106855.  I doesn't seem necessary
anymore; we can use GetExtremeCaretPosition() as we do for the root element.

Comment 11

[Mats Palmgren (inactive)]

Attachment: wdiff of the PresShell::CompleteMove() fix

Comment 12

[Mats Palmgren (inactive)]

Patch rev. 2 also fixes bug 482484.

Comment 13

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

Comment on attachment 424783 [details] [diff] [review]
Patch rev. 2

Nice!

+		test_reftests_with_caret.html \
+		     bug106855-1.html \
+		     bug106855-2.html \
+		     bug106855-1-ref.html \
+		     bug482484.html \

The indenting here is messed up somehow.

Comment 14

[Mats Palmgren (inactive)]

> The indenting here is messed up somehow.

I intentionally indented the files associated with the test_... file
above.  It makes it easy to see which additional (non-test) files
goes with which test.  I think we should do this more. Typically
it looks something like this:
    test_bug1.html \
         bug1-iframe.html \
    test_bug2.html \
         bug2-iframe.html \
         bug2-image.png \

Comment 15

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/83eef73d2ff7

Comment 16

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

ok

Comment 17

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

Roc: can you nominate the appropriate patch for 1.9.2.2?

Comment 18

[Johnathan Nightingale [:johnath]]

Comment on attachment 424783 [details] [diff] [review]
Patch rev. 2

Didn't make 1.9.2.2, moving flag forward.

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 424783 [details] [diff] [review]
Patch rev. 2

Clearing old approval requests now that 1.9.2.4 has shipped. If you believe this patch is still necessary on the 1.9.2 branch please re-request approval along with a risk/benefit analysis explaining why we need it.

Comment 20

[Mats Palmgren (inactive)]

Comment on attachment 424783 [details] [diff] [review]
Patch rev. 2

Low-risk regression fix.

Comment 21

[LegNeato]

Comment on attachment 424783 [details] [diff] [review]
Patch rev. 2

There looks like a fair bit of change here for not much reward. Let's just fix this on trunk and not 1.9.2.