Closed Bug 633044 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Editor, defect)

defect
Not set

Tracking

()

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

People

(Reporter: jarben, Assigned: ehsan)

References

Details

(Keywords: regression, testcase, Whiteboard: [softblocker])

Attachments

(3 files, 4 obsolete files)

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
It could be a regression from Bug 389321
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
Blocks: 389321
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Version: unspecified → Trunk
Assignee: nobody → ehsan
blocking2.0: --- → final+
Whiteboard: [softblocker]
Attached file Minimal testcase (obsolete) —
Keywords: testcase
Attached file 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.
Attachment #511628 - Attachment is obsolete: true
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?
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.
OS: Windows 7 → All
Hardware: x86_64 → All
Attached patch Patch (v1) (obsolete) — Splinter Review
Tests forthcoming.
Attachment #512372 - Flags: review?(roc)
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.
(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.
Attached patch Patch (v2) (obsolete) — Splinter Review
Your comment made me think of a better fix.
Attachment #512372 - Attachment is obsolete: true
Attachment #512402 - Flags: review?(roc)
Attachment #512372 - Flags: review?(roc)
Whiteboard: [softblocker] → [softblocker][needs landing]
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.
Whiteboard: [softblocker][needs landing] → [softblocker][needs new patch]
Actually, I spoke too soon.  It's the change in the IsEmpty semantics which is causing this side effect.
Attached patch Patch (v3) (obsolete) — Splinter Review
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.
Attachment #512402 - Attachment is obsolete: true
Attachment #512538 - Flags: review?(roc)
Whiteboard: [softblocker][needs new patch] → [softblocker][has patch]
I think we should make ClearMetrics non-static and set mAscent to 0 in there. Great catch!
Attached patch Patch (v4)Splinter Review
Good suggestion.
Attachment #512538 - Attachment is obsolete: true
Attachment #512579 - Flags: review?(roc)
Attachment #512579 - Flags: approval2.0?
Attachment #512538 - Flags: review?(roc)
Attachment #512579 - Flags: review?(roc)
Attachment #512579 - Flags: review+
Attachment #512579 - Flags: approval2.0?
Attachment #512579 - Flags: approval2.0+
Whiteboard: [softblocker][has patch] → [softblocker][needs landing]
http://hg.mozilla.org/mozilla-central/rev/76d33291395a
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12
Whiteboard: [softblocker][needs landing] → [softblocker]
Thanks for your prompt fix, our appliction tested in FFbeta12 and works as expected:)
You need to log in before you can comment on or make changes to this bug.