522787 - Text which was drag-dropped into TEXTAREA/INPUT is lost, after frame reconstruct

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P2)

Description

[Alice0775 White]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2b2pre) Gecko/20091016 Firefox/3.5.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a1pre) Gecko/20091016 Minefield/3.7a1pre ID:20091016045204

Text which was drag-dropped into TEXTAREA/INPUT is lost, after detach tab.

On the other hand, They are not lost if I enter any key in TEXTAREA/INPUT 



Reproducible: Always

Steps to Reproduce:
1.Start Minefield with new profile
2.Open http://www.mozilla.org/projects/minefield/ 
3.Open http://www.mozilla.org/projects/firefox/prerelease.html
4.Select any text on the web page, and drag drop into TEXTAREA/INPUT
5.Detach tab of the page

Actual Results:  
Text which was drag-dropped into TEXTAREA/INPUT is lost, after detach tab.


Expected Results:  
Text which was drag-dropped into TEXTAREA/INPUT should be preserved, after detach tab.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Interesting.  This seems like it should be a general issue with form controls....

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: testcase showing the problem

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.  Is this an editor bug?  I don't see the text input listener's EditAction() called in this case (so in particular we are NOT firing oninput, and we're not flagging ourselves as having a changed value).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, hmm.  We don't add the nsTextInputListener unless the control has focus.  Dropping doesn't give it focus.

Enn, Peter, any thoughts on how this _should_ work?  Should we just always have the editor observer set up?

Comment 5

[Boris Zbarsky [:bzbarsky]]

OK, this seems to be a regression between 3.0 and 3.5.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Regressed between 2008-07-21-02 and 2008-07-22-02.  Pushlog range (+2 hours on each end):

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-07-21+00&enddate=2008-07-22+04

Looks like a likely regression from bug 424698.  Should we perhaps just be comparing the textframe value to the default value and setting the "value changed" flag based on that?  That seems to fail if the user did some editing but then deleted the changes....

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, this seems really bad to me. Boris, can you take it since the regression is yours? Or should peterv have it?

Comment 8

[Nick Thomas [:nthomas] (UTC+12)]

Did you want this bug to block 3.6b1 ? It's appearing in that query right now.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

No, blocking the release is fine. Does setting P2 accomplish that?

Comment 10

[Daniel Holbert [:dholbert]]

(I can reproduce this on a linux mozilla-central nighly build. Platform --> All)

Comment 11

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Comment 12

[Mats Palmgren (inactive)]

Always having a nsTextInputListener registered seems to work, and it fires
oninput.  I think we should also fire onchange when necessary (bug 128066).
I'll ask for review in a day or two after some testing...

Comment 13

[[e-mail]]

This won't cause any problems when, say, changing the value of an input (either focused or unfocused) from script, will it?

Comment 14

[Mats Palmgren (inactive)]

Attachment: Testcase #3

Comment 15

[Mats Palmgren (inactive)]

Attachment: wip1

This works as I intended, firing oninput and onchange...  but seeing that
IE8, Safari and Opera focus the drop target on a successful drop that seems
to me as a much nicer behavior, and it's filed as bug 280635 already, then
we don't need an explicit onchange either since we'll get it on blur as normal.

Comment 16

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

This fixes the bug and fires oninput too.

(In reply to comment #13)
> This won't cause any problems when, say, changing the value of an input (either
> focused or unfocused) from script, will it?

No problem as a far as I can tell, did you have specific scenario in mind?

The patch is cooking on TryServer now...

Comment 17

[Mats Palmgren (inactive)]

Attachment: mochitest that doesn't work

I tried hard to make a mochitest for this bug, but can't get the drop part
work.  I was able to create a real DataTransfer for the drag I think.
This code is mostly stolen from synthesizeDrop() in SimpleTest/EventUtils.js
(which doesn't seem to work either), and content/events/test/test_dragstart.html

Comment 18

[Mats Palmgren (inactive)]

Comment on attachment 409070 [details] [diff] [review]
Patch rev. 2

... passed regression tests on TryServer.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 409070 [details] [diff] [review]
Patch rev. 2

I assume this doesn't pick up scripted value changes as editor stuff or anything like that?

Comment 20

[Mats Palmgren (inactive)]

No problem as a far as I can tell, testing various scripted changes with
Testcase #3.  Do you have specific scenario in mind?
(Neil asked the same thing in comment 13.)

Comment 21

[Boris Zbarsky [:bzbarsky]]

> Do you have specific scenario in mind?

No, just making sure we don't start firing oninput/onchange for scripted changes. It'd be good to have a mochitest for that if we don't have one already....

Comment 22

[Mats Palmgren (inactive)]

Only found content/html/content/test/test_bug388558.html but it tests
the opposite, that we do get onchange for user input.
I'll write a few simple tests that scripted changes doesn't trigger events...

Comment 23

[Mats Palmgren (inactive)]

... that test actually tests some scripted changes too.  I'll see if I can
add some more cases.

Comment 24

[Mats Palmgren (inactive)]

Attachment: mochitest-388558.diff

Add more tests to content/html/content/test/test_bug388558.html
Now also tests input events, and scripted changes from onfocus/
oninput/onblur.

Comment 25

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

Comment on attachment 409070 [details] [diff] [review]
Patch rev. 2

Olli, maybe you could review this instead of Boris?

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Mats, did you test undo/redo handling?

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

I think undo/redo should work just fine, but better to test.
I'll test once my build is ready.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 409070 [details] [diff] [review]
Patch rev. 2

Would be great to understand the reason for setting observer when focusing, but unfortunately bonsai history seems to end to bug 129909.
Perhaps 129909#c4 didn't actually happen :(

Comment 29

[Boris Zbarsky [:bzbarsky]]

> bonsai history seems to end to bug 129909.

No, #c4 didn't happen.  You can see the pre-rename blame at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla%2Flayout%2Fhtml%2Fforms%2Fsrc%2FAttic%2FnsGfxTextControlFrame2.cpp&rev=&cvsroot=%2Fcvsroot as desired.

Comment 30

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

http://hg.mozilla.org/mozilla-central/rev/cce7954f1b99

The tests still need to be landed.

Comment 31

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

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/262927ab7c68

Comment 32

[Mats Palmgren (inactive)]

(In reply to comment #27)
> I think undo/redo should work just fine, but better to test.

Yes, undo/redo works fine for undoing/redoing the dropped text.
After clicking the button (recreating the frame) the edit history is lost
though (for any edit, not just drop).  That seems like a bug to me,
but expected since the editor is owned by the frame.
(bug 343812 should fix that)