500905 - scroll and caret position of a textarea resets during reflow

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Radek 'sysKin' Czyz]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090626 Firefox/3.6a1pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090626 Minefield/3.6a1pre

See attached testcase. I'm not sure if it's the reflow that's causing it, but something is.


Reproducible: Always

Steps to Reproduce:
1. Open attached testcase
2. Type something into text area
3. Use left arrow to move the caret back and attempt to type something in the middle

Actual Results:  
When the "preview" box under text area updates (on every keypress), textarea scrolling is reset to zero and caret is put at the end, making editing impossible.


Expected Results:  
Preview has no effect.


This is a reduced testcase from a big forum, where this kind of logic is used to preview the post as it's being typed or edited.

This regressed ~2-4 days ago. I will attempt to get a detailed regression rage.

Comment 1

[Radek 'sysKin' Czyz]

Attachment: testacse

Comment 2

[Yansky]

Possibly same as this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=496649

I can reproduce this error using Minefield 1.9.2a1pre build 20090626, but the error doesn't seem to occur when using Minefield 1.9.2a1pre build 20090611. 

So the bug was introduced after build 20090611.

Comment 3

[Yansky]

Could be related to the patch that landed in the 20090618 Trunk: 
http://www.squarefree.com/burningedge/2009/06/18/2009-06-18-trunk-builds/
https://bugzilla.mozilla.org/show_bug.cgi?id=178324

Comment 4

[Timothy Nikkel (:tnikkel)]

Very likely related to bug 496649, as the key thing in this testcase is the <p> tags in the innerHTML of the span. Except this case is a recent regression, whereas bug 496649 goes back at least as far as Firefox 2.

Regression range for this bug http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c575412d976a&tochange=5fe89f2c22f0

Comment 5

[Boris Zbarsky [:bzbarsky]]

Probably a regression from bug 495385.  I bet we're reframing on every char.

Bug 500556 might help.

Comment 6

[Boris Zbarsky [:bzbarsky]]

On the other hand, this text should be white-space:pre or some such and hence not get the "is it whitespace?" bits.  Why is it getting them at all, assuming that's what happens?

Comment 7

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

Here's the stack for the reconstruction of the textarea:

0  nsBoxFrame::Init (this=0x172b8dc, aContent=0x1026aa60, aParent=0x173cb54, aPrevInFlow=0x0) at /Users/roc/mozilla-checkin/layout/xul/base/src/nsBoxFrame.cpp:203
#1  0x01eb74c9 in nsCSSFrameConstructor::InitAndRestoreFrame (this=0x10259300, aState=@0xbfffbd98, aContent=0x1026aa60, aParentFrame=0x173cb54, aPrevInFlow=0x0, aNewFrame=0x172b8dc, aAllowCounters=1) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:4704
#2  0x01ebf18c in nsCSSFrameConstructor::ConstructFrameFromItemInternal (this=0x10259300, aItem=@0xd1f4b00, aState=@0xbfffbd98, aParentFrame=0x173cb54, aFrameItems=@0xbfffb2e0) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:3927
#3  0x01ebf912 in nsCSSFrameConstructor::ConstructFramesFromItem (this=0x10259300, aState=@0xbfffbd98, aIter=@0xbfffaff4, aParentFrame=0x173cb54, aFrameItems=@0xbfffb2e0) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:5577
#4  0x01ebfaa3 in nsCSSFrameConstructor::ConstructFramesFromItemList (this=0x10259300, aState=@0xbfffbd98, aItems=@0xbfffb1a8, aParentFrame=0x173cb54, aFrameItems=@0xbfffb2e0) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:9458
#5  0x01ec0b6f in nsCSSFrameConstructor::ProcessChildren (this=0x10259300, aState=@0xbfffbd98, aContent=0x1026aa20, aStyleContext=0x172b314, aFrame=0x173cb54, aCanHaveGeneratedContent=1, aFrameItems=@0xbfffb2e0, aAllowBlockStyles=1) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:9571
#6  0x01ec4171 in nsCSSFrameConstructor::ConstructTableCell (this=0x10259300, aState=@0xbfffbd98, aItem=@0xd19aa70, aParentFrame=0x172b290, aDisplay=0x172b384, aFrameItems=@0xbfffb764, aNewFrame=0xbfffb3b4) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:2338
#7  0x01ebef75 in nsCSSFrameConstructor::ConstructFrameFromItemInternal (this=0x10259300, aItem=@0xd19aa70, aState=@0xbfffbd98, aParentFrame=0x172b290, aFrameItems=@0xbfffb764) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:3888
#8  0x01ebf912 in nsCSSFrameConstructor::ConstructFramesFromItem (this=0x10259300, aState=@0xbfffbd98, aIter=@0xbfffb494, aParentFrame=0x172b290, aFrameItems=@0xbfffb764) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:5577
#9  0x01ebfaa3 in nsCSSFrameConstructor::ConstructFramesFromItemList (this=0x10259300, aState=@0xbfffbd98, aItems=@0xbfffb648, aParentFrame=0x172b290, aFrameItems=@0xbfffb764) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:9458
#10 0x01ec0b6f in nsCSSFrameConstructor::ProcessChildren (this=0x10259300, aState=@0xbfffbd98, aContent=0x1026a8e0, aStyleContext=0x172af50, aFrame=0x172b290, aCanHaveGeneratedContent=1, aFrameItems=@0xbfffb764, aAllowBlockStyles=0) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:9571
#11 0x01ec43bd in nsCSSFrameConstructor::ConstructTableRow (this=0x10259300, aState=@0xbfffbd98, aItem=@0x9225dc0, aParentFrame=0x172b0d8, aDisplay=0x172b188, aFrameItems=@0xbfffbc28, aNewFrame=0xbfffb824) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:2196
#12 0x01ebef75 in nsCSSFrameConstructor::ConstructFrameFromItemInternal (this=0x10259300, aItem=@0x9225dc0, aState=@0xbfffbd98, aParentFrame=0x172b0d8, aFrameItems=@0xbfffbc28) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:3888
#13 0x01ebf912 in nsCSSFrameConstructor::ConstructFramesFromItem (this=0x10259300, aState=@0xbfffbd98, aIter=@0xbfffb904, aParentFrame=0x172b0d8, aFrameItems=@0xbfffbc28) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:5577
#14 0x01ebfaa3 in nsCSSFrameConstructor::ConstructFramesFromItemList (this=0x10259300, aState=@0xbfffbd98, aItems=@0xbfffbab8, aParentFrame=0x172b0d8, aFrameItems=@0xbfffbc28) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:9458
#15 0x01ec0b6f in nsCSSFrameConstructor::ProcessChildren (this=0x10259300, aState=@0xbfffbd98, aContent=0x1026a8a0, aStyleContext=0x172afe8, aFrame=0x172b0d8, aCanHaveGeneratedContent=1, aFrameItems=@0xbfffbc28, aAllowBlockStyles=0) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:9571
#16 0x01ebf426 in nsCSSFrameConstructor::ConstructFrameFromItemInternal (this=0x10259300, aItem=@0xd644f90, aState=@0xbfffbd98, aParentFrame=0x172aeb0, aFrameItems=@0xbfffbe20) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:3972
#17 0x01ebf912 in nsCSSFrameConstructor::ConstructFramesFromItem (this=0x10259300, aState=@0xbfffbd98, aIter=@0xbfffbd14, aParentFrame=0x172aeb0, aFrameItems=@0xbfffbe20) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:5577
#18 0x01ebfaa3 in nsCSSFrameConstructor::ConstructFramesFromItemList (this=0x10259300, aState=@0xbfffbd98, aItems=@0xbfffbde0, aParentFrame=0x172aeb0, aFrameItems=@0xbfffbe20) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:9458
#19 0x01ec6c48 in nsCSSFrameConstructor::ContentInserted (this=0x10259300, aContainer=0x1026a800, aChild=0x1026a8a0, aIndexInContainer=1, aFrameState=0x1026a150) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:6775
#20 0x01ec80f5 in nsCSSFrameConstructor::RecreateFramesForContent (this=0x10259300, aContent=0x1026a8a0) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:9080
#21 0x01ec860e in nsCSSFrameConstructor::WipeContainingBlock (this=0x10259300, aState=@0xbfffbfe8, aContainingBlock=0x172aa90, aFrame=0x172b0d8, aItems=@0xbfffc030, aIsAppend=0, aPrevSibling=0x172b290) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:11118
#22 0x01ec6bbe in nsCSSFrameConstructor::ContentInserted (this=0x10259300, aContainer=0x1026a8a0, aChild=0x1026ac70, aIndexInContainer=2, aFrameState=0x1026a150) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:6763
#23 0x01ec80f5 in nsCSSFrameConstructor::RecreateFramesForContent (this=0x10259300, aContent=0x1026ac70) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:9080
#24 0x01ec860e in nsCSSFrameConstructor::WipeContainingBlock (this=0x10259300, aState=@0xbfffc238, aContainingBlock=0x172aa90, aFrame=0x1725210, aItems=@0xbfffc280, aIsAppend=0, aPrevSibling=0x17253dc) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:11118
#25 0x01ec6bbe in nsCSSFrameConstructor::ContentInserted (this=0x10259300, aContainer=0x1026ac70, aChild=0x1026ad80, aIndexInContainer=3, aFrameState=0x1026a150) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:6763
#26 0x01ec80f5 in nsCSSFrameConstructor::RecreateFramesForContent (this=0x10259300, aContent=0x1026ad80) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:9080
#27 0x01ec82fc in nsCSSFrameConstructor::ReframeContainingBlock (this=0x10259300, aFrame=0x173cc0c) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:11272
#28 0x01ec8d98 in nsCSSFrameConstructor::MaybeRecreateContainerForFrameRemoval (this=0x10259300, aFrame=0x172cb30, aResult=0xbfffc4f0) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:9007
#29 0x01ec76f6 in nsCSSFrameConstructor::ContentRemoved (this=0x10259300, aContainer=0x1026ae00, aChild=0xd6bbe50, aIndexInContainer=0, aFlags=nsCSSFrameConstructor::REMOVE_CONTENT, aDidReconstruct=0xbfffc598) at /Users/roc/mozilla-checkin/layout/base/nsCSSFrameConstructor.cpp:7196
#30 0x01f2eee4 in PresShell::ContentRemoved (this=0x170aa00, aDocument=0x1447800, aContainer=0x1026ae00, aChild=0xd6bbe50, aIndexInContainer=0) at /Users/roc/mozilla-checkin/layout/base/nsPresShell.cpp:5052
#31 0x0221b186 in nsNodeUtils::ContentRemoved (aContainer=0x1026ae00, aChild=0xd6bbe50, aIndexInContainer=0) at /Users/roc/mozilla-checkin/content/base/src/nsNodeUtils.cpp:167
#32 0x022054e9 in nsGenericElement::doRemoveChildAt (aIndex=0, aNotify=1, aKid=0xd6bbe50, aParent=0x1026ae00, aDocument=0x1447800, aChildArray=@0x1026ae1c) at /Users/roc/mozilla-checkin/content/base/src/nsGenericElement.cpp:3326
#33 0x02205635 in nsGenericElement::RemoveChildAt (this=0x1026ae00, aIndex=0, aNotify=1) at /Users/roc/mozilla-checkin/content/base/src/nsGenericElement.cpp:3256
#34 0x0219b7bb in nsContentUtils::SetNodeTextContent (aContent=0x1026ae00, aValue=@0x5c09c0, aTryReuse=0) at /Users/roc/mozilla-checkin/content/base/src/nsContentUtils.cpp:3781
#35 0x022ba7bc in nsGenericHTMLElement::SetInnerHTML (this=0x1026ae00, aInnerHTML=@0xbfffc894) at /Users/roc/mozilla-checkin/content/html/content/src/nsGenericHTMLElement.cpp:688

We're setting the innerHTML of the <span> to a <p>, so that causes reframing of the containing block (the table cell), which triggers reframing of the row (don't know why), which triggers reframing of the table body which is element for the table itself.

Several problems here:
-- the caret position doesn't survive reframing of the textarea
-- we should be able to reframe just the children of the table cell instead of the table cell itself
-- I don't see why reframing the cell should reframe the row
-- I don't see how this is connected to bug 495385

Comment 8

[Boris Zbarsky [:bzbarsky]]

> we should be able to reframe just the children of the table cell instead of
> the table cell itself

dbaron and I have been thinking about doing this, yeah.  We should try to just do it.

> -- I don't see why reframing the cell should reframe the row
> -- I don't see how this is connected to bug 495385

These are almost certainly related.  That said, I would have expected bug 500467 to fix this.  At least for the "remove the cell" case.  I suppose it might still be a problem for the "insert the cell" case.  Maybe we need a similar parent frame type check there?

Comment 9

[Boris Zbarsky [:bzbarsky]]

This is a "remove for reconstruct", so I bet that's exactly the issue.  I should be able to put a patch together for this tomorrow.

Comment 10

[Timothy Nikkel (:tnikkel)]

Should we maybe make testarea implement nsIStatefulFrame so that in any other cases of reconstruction we save the scroll/caret position?

Comment 11

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

Yes, that would be cool.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

A few things going on here:

1)  Modified our existing crashtest to test inserts, not just removes.  This
    immediately showed the O(N^2) behavior due to reframing on every insert.
2)  Fixed WipeContainingBlock to be smarter about trailing whitespace so as not
    to actually trigger reframes here.  That got me down to about 30% slower
    than before bug 495385 landed.
3)  Changed ContentAppended and ContentInserted to check parent type before
    trying to even create the items for those whitespace nodes.  That got back
    the 30% loss, so now we're as fast as nightlies from May or November on
    that testcase.

I did check that the patch also fixes the testcase in this bug.  That's not tested by the crashtest, since that could also pass if we did lazy reframes...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/8a901fb8e927

Timothy, that sounds like a good idea; file a bug on that?

Comment 14

[Timothy Nikkel (:tnikkel)]

(In reply to comment #13)
> Timothy, that sounds like a good idea; file a bug on that?

I think we may as well use bug 496649 as the bug for that.