Closed Bug 613807 Opened 14 years ago Closed 14 years ago

Caret is moved at second row when undoing after committing IME text

Categories

(Core :: DOM: Editor, defect)

x86
All
defect
Not set
normal

Tracking

()

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

People

(Reporter: alice0775, Assigned: ehsan.akhgari)

References

()

Details

(Keywords: inputmethod, regression)

Attachments

(1 file)

Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101120 Firefox/4.0b8pre ID:20101120044537

Caret is moved at next line when undoing after committing IME text.

*This does not happen on Firefox3.6.13pre.
*This happens on WindowsXP+MS-IME2002 and Windows7+ATOK2006/Microsoft IME (I do not try with other IME)
*This happens on ubuntu8.04+SCIM

Reproducible: Always

Steps to Reproduce:
1. Start minefield with new profile
2. Open data:text/html;charset=utf-8,<textarea><%2Ftextarea>
3. Click and focus textarea
4. IME on
5. Input something and commit
6. Undo (ctrl+z)

Actual Results:
 Caret is moved at second row

Expected Results:
 Caret should be moved at first row

Regression Window:
Works:
http://hg.mozilla.org/mozilla-central/rev/c0dbdafa583c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101104 Firefox/4.0b8pre ID:20101104134944
Fails:
http://hg.mozilla.org/mozilla-central/rev/52a6a18eeb61
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101104 Firefox/4.0b8pre ID:20101104165457
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c0dbdafa583c&tochange=52a6a18eeb61
blocking2.0: --- → ?
OS: Linux → All
The following cset causes the problem,
ed0befc22bb7	Ehsan Akhgari — Bug 389321 - Part 3: Use a centralized algorithm for caret positioning; r=roc a=blocking-betaN+
Blocks: 389321
Assignee: nobody → ehsan
blocking2.0: ? → final+
So, I tried debugging this, and it seems that after the undo step, the text frame gets a Y offset for some reason.  Here's how the text frame inside the anonymous div looks before undo:

Text(0)@0x125424460 [run=0x11e891260][0,1,T]  next=0x126746208 {60,0,780,960} [state=00000000c0604010] [content=0x11aa12490] sc=0x1267460a8 pst=:-moz-non-element<
  "\u3107"
>

And here's how it look after the undo step:

Text(0)@0x125424460 [run=0x11adf7b20][0,0,T]  next=0x126746208 {60,600,0,0} [state=0000000040404010] [content=0x11aa12490] sc=0x1267460a8 pst=:-moz-non-element<
  ""
>

(note the Y offset of 600.)

I tried setting a breakpoint on nsTextFrame::Reflow to see what's happening, but for some reason, the breakpoint is not triggered at all.  This is really strange...
Hmm, am I missing something?  That nsTextFrame::Reflow breakpoint doesn't even get triggered when I navigate to google.com!  Now, surely there's some text reflow going on somewhere...
OK, I feel dumb!  I meant to set the breakpoint on ReflowText...  :/
Masayuki, I tested your patch in bug 613810, and it solves this bug as well.  Can you please write a test for this bug (or tell me how I can simulate IME transactions in a mochitest so that I can write the test myself)?  Thanks!
Depends on: 613810
I guess that if my patch fixes this bug, the *cause* of this bug is invalid. The cause of bug 613810 is, IME handling code breaks the undo stack. Therefore, the DOM tree in the editor was broken by undoing or redoing with the broken stack.

# I guess, maybe, a bogus node which is a placeholder of caret is broken?

So, I think that you don't need to write the testcases for this bug. And I'll post a new patch today.
(In reply to comment #7)
> I guess that if my patch fixes this bug, the *cause* of this bug is invalid.
> The cause of bug 613810 is, IME handling code breaks the undo stack. Therefore,
> the DOM tree in the editor was broken by undoing or redoing with the broken
> stack.

Quite possible.

> # I guess, maybe, a bogus node which is a placeholder of caret is broken?

Hmm, no, the selection is on the BR node both with and without your patch, it's the layout that's different, as far as I can tell.

Placeholder transactions do all sorts of magic to make sure that the reframing happens at the right time, the caret is displayed at the right time, the selection is set up on the right frame, etc. (<http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.cpp#905> and so forth).  So if you get yourself into the situation where you don't start a placeholder transaction at the right time, you can get yourself into all sorts of trouble.

> So, I think that you don't need to write the testcases for this bug. And I'll
> post a new patch today.

I guess a test might not be strictly needed, but still, I'd feel better if we have one, so I guess I'll write one!
Attached patch test caseSplinter Review
Attachment #493090 - Flags: review?(roc)
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/48202671f152
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: mozilla2.0 → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: