633044 - After inserting and deleting a text node, caret visually jumps into a wrong position

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jaroslav Benc]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10) Gecko/20100101 Firefox/4.0b10
Build Identifier: 4.0b10

If you attach a keypress event handler, insert a text node and delete it straight away, caret visually jumps into a wrong position which is confusing when editing. If you start typing caret goes back and characters are displayed as expected. 

This was introduced in FF4beta8 (built 2010-11-10-05) 

I haven't found any workaround yet so would be great if this is considered with higher priority as we'd have to warn users to not upgrade to FF4 until this is fixed. Also it would help if you could suggest if this bug is likely to be fixed. 

Reproducible: Always

Steps to Reproduce:
1. Open the example page attached to this bug in FFbeta8+
2. press home or up key 2x

Actual Results:  
Note that caret jumped into the second line, actuall position is correct when starting typing

Expected Results:  
Caret should visually stay on its actuall position. 

This is a last nighlty build when this bug wasn't introduced:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2010/11/2010-11-09-04-tracemonkey/, built from http://hg.mozilla.org/tracemonkey/rev/d988fa5af845

This is a first nightly build when this bug was introduced: 
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2010/11/2010-11-10-05-tracemonkey/, built from http://hg.mozilla.org/tracemonkey/rev/2fd60328c2b0

Changeset:
http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=d988fa5af845&tochange=2fd60328c2b0

Comment 1

[Jaroslav Benc]

Attachment: a test case of an editor using designmode, press home or up key 2x and notice caret has moved visually to a wrong place.

Comment 2

[Jaroslav Benc]

It could be a regression from Bug 389321

Comment 3

[Alice0775 White]

Regression window(cached m-c hourly):
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

Comment 4

[(no longer active)]

Attachment: Minimal testcase

Comment 5

[(no longer active)]

Attachment: Simpler test case

When we first initialize the document, the selection is at the body element with offset 0, and there is a textnode in the document with content "\n".  The first time you press up, the selection is anchored to the text node with offset 0.  The second time you press up, the insertNode call splits the textnode into two text nodes, one empty and one with "\n" and injects the new textnode in between them and then removes it.  Now we have two textnodes in our document, with the selection anchored to the first (empty) one.

This version of the test case plays back the effect of the first up key (the selection change), and therefore demonstrates the bug using one single up keystroke.

Comment 6

[(no longer active)]

The incorrect caret position is a result of the empty text frame being mispositioned.  Here is the bogus frame tree: http://pastebin.mozilla.org/1046325.

The y position of the first textframe (0x128553398) is bogus (816).  It should be 96.

roc: do you have any idea why this would happen?

Comment 7

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

I don't think its position is bogus. It looks like it has been baseline-aligned with the rest of the line. Arguably it should have a smaller y and a nonzero height. It might be safer to instead special-case empty text frames in caret positioning.

Comment 8

[(no longer active)]

Attachment: Patch (v1)

Tests forthcoming.

Comment 9

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

I presume the empty frame reaches this code:

  if (completedFirstLetter && transformedCharsFit == 0 && !usedHyphenation) {
    aMetrics.ascent = 0;
    aMetrics.height = 0;

It seems to me that the right thing to do would be to set mAscent to 0 here too. Then GetCaretBaseline/GetBaseline would return 0, and we'd get the right results.

Comment 10

[(no longer active)]

(In reply to comment #9)
> I presume the empty frame reaches this code:
> 
>   if (completedFirstLetter && transformedCharsFit == 0 && !usedHyphenation) {
>     aMetrics.ascent = 0;
>     aMetrics.height = 0;
> 
> It seems to me that the right thing to do would be to set mAscent to 0 here
> too. Then GetCaretBaseline/GetBaseline would return 0, and we'd get the right
> results.

Hmm, firstly, completedFirstLetter is false, so I'm not sure why you think we'll end up in that branch.  In fact, we'll end up in the last else branch.

And furthermore, we do |mAscent = aMetrics.ascent;| after the entire if/else branch, so if we would end up in that branch, mAscent would indeed be 0.

Comment 11

[(no longer active)]

Attachment: Patch (v2)

Your comment made me think of a better fix.

Comment 12

[(no longer active)]

So, I'm thinking that maybe setting mAscent to 0 like that is not the right thing to do, because we use mAscent for a lot more than just computing the caret baseline.

This patch broke <http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/general/test_focusrings.xul> because a |synthesizeMouse(elem, 8, 8)| call was clicking in the wrong element.  I investigated, and it seems like this patch is breaking things like data:text/html,<style>*{-moz-appearance: none;}</style><input type=button>.  With this patch, the above page would result in a button which is less than 8 pixels tall, presumably because the 0 mAscent changes the height of the inner text frame in an unexpected way.  There may be a lot more similar cases like this that we just don't catch in our test suite.

So I guess I'm going to go back to overriding the GetCaretBaseline function here, as it seems like a safer choice.

Comment 13

[(no longer active)]

Actually, I spoke too soon.  It's the change in the IsEmpty semantics which is causing this side effect.

Comment 14

[(no longer active)]

Attachment: Patch (v3)

Actually all the other stuff that I had been looking into before were red herrings.  Here is the real problem.  In two cases, when we reflowed a textframe, mAscent would remain equal to the previous value.  We should set mAscent to zero in those cases, and this is what the current patch does.

Comment 15

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

I think we should make ClearMetrics non-static and set mAscent to 0 in there. Great catch!

Comment 16

[(no longer active)]

Attachment: Patch (v4)

Good suggestion.

Comment 17

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/76d33291395a

Comment 18

[Jaroslav Benc]

Thanks for your prompt fix, our appliction tested in FFbeta12 and works as expected:)