Closed Bug 597331 Opened 14 years ago Closed 14 years ago

Reframing a textarea sets the caret position to the end of its contents

Categories

(Core :: DOM: Editor, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- final+

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

(Keywords: regression, testcase)

Attachments

(1 file, 2 obsolete files)

STR:

1. Load https://bug596710.bugzilla.mozilla.org/attachment.cgi?id=476169.
2. Click on the right side of the first line.  The caret seems to be on the end of that line.
3. Press the right arrow key (or the left arrow key).  The caret suddenly jumps to the end of the textarea.
blocking2.0: --- → ?
crash-stats tells me that this has happened three times on 4.0b7pre.  It doesn't seem to be a good candidate to block the relase at this point.  Plus we probably need a testcase if we ever want to make any progress here.
blocking2.0: ? → ---
Keywords: testcase-wanted
This is a regression from bug 593211.

changeset:   53719:1e0a9cf6dc53
user:        Ehsan Akhgari <ehsan@mozilla.com>
date:        Wed Sep 08 20:15:24 2010 -0400
summary:     Bug 593211 - Part 1: Back out the code parts of bug 585922 r,a=roc
Blocks: 593211
No longer blocks: 240933
blocking2.0: --- → ?
Oh, and comment 1 should be ignored, as it was targeted at another bug.
This is in fact a regression from bug 221820.  It happens because when we reframe the textarea on the style change, we try to set the focus even if it's already focused, which repositions the selection to the end of the textarea.
Blocks: 221820
No longer blocks: 593211
Attached patch Patch (v1) (obsolete) — Splinter Review
Attachment #478091 - Flags: review?(roc)
Actually this patch is not good enough, since it regresses this test <http://mxr.mozilla.org/mozilla-central/source/layout/forms/test/test_bug446663.html?force=1>, possibly among others.  I guess we really want bug 536421 as the correct fix (because even with this patch, we bring the behavior on part with 1.9.2 where we lose the caret until the control is refocused.)  That bug should preserve the selection state across reframes, and I think we should address this after that bug is resolved.
Depends on: 536421
Attachment #478091 - Attachment is obsolete: true
roc: the summary of this bug was completely wrong.  Do you still think that it should block?  (Personally I think so, just wanted to make sure that you haven't been misled by the incorrect summary.)
Summary: Clicking on a text area to the right of a line doesn't correctly position the caret at the end of that line → Reframing a textarea sets the caret position to the end of its contents
I can reproduce not only 
2. Click on the right side of the first line.  The caret seems to be on the end
of that line.

but also
2. click any position
(In reply to comment #9)
> I can reproduce not only 
> 2. Click on the right side of the first line.  The caret seems to be on the end
> of that line.
> 
> but also
> 2. click any position

That's right, and that's why the summary was misleading.  In fact, the only thing about hitting an arrow key is that the onkeydown handler for the textarea causes it to be reframed, and this problem can happen in any way that reframing is triggered.  See the testcase inside my patch for another example.
Whiteboard: [waiting on bug 536421]
Attached patch Patch (v2) (obsolete) — Splinter Review
This patch makes sure that the selection offsets survive reframing, and are used to restore the selection to the previous location after reframing.
Attachment #482673 - Flags: review?(bzbarsky)
Whiteboard: [waiting on bug 536421] → [has patch][needs review bz]
Comment on attachment 482673 [details] [diff] [review]
Patch (v2)

Oh, very cute.  r=me

Man, is having this persistent data structure around nice... ;)
Attachment #482673 - Flags: review?(bzbarsky) → review+
I know, right?!  ;-)
Whiteboard: [has patch][needs review bz] → [needs landing]
So I guess we don't correctly deal with selections having more than one range in a lot of places then?
Well, I guess not, but in this particular case, I'm not sure that I care all that much.  The underlying API doesn't support multiple range selections either...
Attached patch Patch (v3)Splinter Review
Actually, version 2 of this patch had a serious problem.  nsTextControlFrame::GetSelectionRange calls EnsureEditorIsInitialized, which means that we'd init the editor right after we had destroyed it.  Bad bad stuff.  We need to grab the selection range right before calling nsTextEditorState::DestroyEditor, and only if mEditorInitialized is true, so that unbinding the editor frame wouldn't have the side effect of initializing the editor.
Attachment #482673 - Attachment is obsolete: true
Attachment #482845 - Flags: review?(bzbarsky)
Whiteboard: [needs landing] → [has patch][needs review bz]
Comment on attachment 482845 [details] [diff] [review]
Patch (v3)

That all needs to go into a nice comment right before the mEditorInitialized check, so someone doesn't helpfully reorder the statements at a later date!
Attachment #482845 - Flags: review?(bzbarsky) → review+
(In reply to comment #18)
> Comment on attachment 482845 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=482845
> Patch (v3)
> 
> That all needs to go into a nice comment right before the mEditorInitialized
> check, so someone doesn't helpfully reorder the statements at a later date!

Will do.
Whiteboard: [has patch][needs review bz] → [needs landing]
http://hg.mozilla.org/mozilla-central/rev/178f26e21cfc
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla2.0b8
Depends on: 605138
Depends on: 606430
Depends on: 605484
Target Milestone: mozilla2.0b8 → mozilla2.0b7
Depends on: 612271
No longer depends on: 612271
Depends on: 629878
Depends on: 637671
Keywords: verifyme
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.