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

RESOLVED FIXED in mozilla1.9.3a2

Status

()

Core
Editor
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: mats)

Tracking

(Depends on: 1 bug, {regression, testcase})

Trunk
mozilla1.9.3a2
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking1.9.2 -, status1.9.2 wontfix, status1.9.1 unaffected)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
Created attachment 396269 [details]
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.
(Reporter)

Comment 1

8 years ago
NB: The cursor also gets stuck if you

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

Updated

8 years ago
Blocks: 496275
(Assignee)

Comment 2

8 years ago
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.
Keywords: regression, regressionwindow-wanted
(Reporter)

Comment 3

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

Comment 4

8 years ago
hg bisect says the change that caused the second bullet point above was http://hg.mozilla.org/mozilla-central/rev/94a7af7250dd
(Reporter)

Comment 5

8 years ago
The change which caused the third bullet point appears to be http://hg.mozilla.org/mozilla-central/rev/c6c685aa0379
(Assignee)

Updated

8 years ago
Blocks: 495385
status1.9.1: --- → unaffected
Flags: in-testsuite?
Keywords: regressionwindow-wanted → testcase
OS: Linux → All
Hardware: x86 → All
(Assignee)

Comment 6

8 years ago
Created attachment 424522 [details] [diff] [review]
Patch rev. 1

The selection/caret code depends on having frames so don't inhibit
frame creation for editable content, for now.
Attachment #424522 - Flags: review?(roc)
(Assignee)

Comment 7

8 years ago
Using blocking flag to say status1.9.2=affected
blocking1.9.2: --- → ?
 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.
(Assignee)

Comment 9

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

Comment 10

8 years ago
Created attachment 424783 [details] [diff] [review]
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.
Assignee: nobody → matspal
Attachment #424522 - Attachment is obsolete: true
Attachment #424783 - Flags: review?(roc)
Attachment #424522 - Flags: review?(roc)
(Assignee)

Comment 11

8 years ago
Created attachment 424786 [details] [diff] [review]
wdiff of the PresShell::CompleteMove() fix
(Assignee)

Comment 12

8 years ago
Patch rev. 2 also fixes bug 482484.
Blocks: 482484
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.
Attachment #424783 - Flags: review?(roc) → review+
(Assignee)

Comment 14

8 years ago
> 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 \
(Assignee)

Comment 15

8 years ago
http://hg.mozilla.org/mozilla-central/rev/83eef73d2ff7
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a2
ok
Roc: can you nominate the appropriate patch for 1.9.2.2?
blocking1.9.2: ? → -
status1.9.2: --- → wanted
Attachment #424783 - Flags: approval1.9.2.2?
Comment on attachment 424783 [details] [diff] [review]
Patch rev. 2

Didn't make 1.9.2.2, moving flag forward.
Attachment #424783 - Flags: approval1.9.2.2? → approval1.9.2.3?
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.
Attachment #424783 - Flags: approval1.9.2.4?
(Assignee)

Comment 20

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

Low-risk regression fix.
Attachment #424783 - Flags: approval1.9.2.7?

Comment 21

8 years ago
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.
Attachment #424783 - Flags: approval1.9.2.7? → approval1.9.2.7-

Updated

8 years ago
status1.9.2: wanted → wontfix
Depends on: 717868
You need to log in before you can comment on or make changes to this bug.