Closed
Bug 512295
Opened 15 years ago
Closed 14 years ago
Cursor becomes stuck when going down in a contenteditable <div> containing a multi-line <p>
Categories
(Core :: DOM: Editor, defect)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a2
Tracking | Status | |
---|---|---|
blocking1.9.2 | --- | - |
status1.9.2 | --- | wontfix |
status1.9.1 | --- | unaffected |
People
(Reporter: justin.lebar+bug, Assigned: MatsPalmgren_bugz)
References
Details
(Keywords: regression, testcase)
Attachments
(3 files, 1 obsolete file)
131 bytes,
text/html
|
Details | |
16.79 KB,
patch
|
roc
:
review+
christian
:
approval1.9.2.7-
|
Details | Diff | Splinter Review |
3.03 KB,
patch
|
Details | Diff | Splinter Review |
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•15 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
Assignee | ||
Comment 2•15 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•14 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•14 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•14 years ago
|
||
The change which caused the third bullet point appears to be http://hg.mozilla.org/mozilla-central/rev/c6c685aa0379
Assignee | ||
Updated•14 years ago
|
Blocks: 495385
status1.9.1:
--- → unaffected
Flags: in-testsuite?
Keywords: regressionwindow-wanted → testcase
OS: Linux → All
Hardware: x86 → All
Assignee | ||
Comment 6•14 years ago
|
||
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•14 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•14 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•14 years ago
|
||
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•14 years ago
|
||
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•14 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•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/83eef73d2ff7
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a2
ok
Comment 17•14 years ago
|
||
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 18•14 years ago
|
||
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 19•14 years ago
|
||
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•14 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•14 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-
You need to log in
before you can comment on or make changes to this bug.
Description
•