Closed Bug 522787 Opened 10 years ago Closed 10 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta4-fixed
status1.9.1 --- wanted

People

(Reporter: alice0775, Assigned: mats)

References

Details

(Keywords: dataloss, regression)

Attachments

(5 files, 2 obsolete files)

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.
Depends on: 113934
Version: unspecified → Trunk
Interesting.  This seems like it should be a general issue with form controls....
Status: UNCONFIRMED → NEW
Component: Document Navigation → Layout: Form Controls
Ever confirmed: true
QA Contact: docshell → layout.form-controls
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).
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?
OK, this seems to be a regression between 3.0 and 3.5.
No longer depends on: 113934
Summary: Text which was drag-dropped into TEXTAREA/INPUT is lost, after detach tab. → Text which was drag-dropped into TEXTAREA/INPUT is lost, after frame reconstruct
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....
Blocks: 424698
Yeah, this seems really bad to me. Boris, can you take it since the regression is yours? Or should peterv have it?
blocking1.9.1: --- → ?
Flags: blocking1.9.2+
Did you want this bug to block 3.6b1 ? It's appearing in that query right now.
No, blocking the release is fine. Does setting P2 accomplish that?
Priority: -- → P2
Keywords: regression
blocking1.9.1: ? → ---
Keywords: dataloss
(I can reproduce this on a linux mozilla-central nighly build. Platform --> All)
OS: Windows Vista → All
Hardware: x86 → All
Attached file Testcase #2 (obsolete) —
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...
Blocks: 128066
Status: NEW → ASSIGNED
This won't cause any problems when, say, changing the value of an input (either focused or unfocused) from script, will it?
Attached file Testcase #3
Attachment #408756 - Attachment is obsolete: true
Attached patch wip1 (obsolete) — Splinter Review
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.
Attached patch Patch rev. 2Splinter Review
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...
Attachment #409069 - Attachment is obsolete: true
Attachment #409070 - Flags: review?(bzbarsky)
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 on attachment 409070 [details] [diff] [review]
Patch rev. 2

... passed regression tests on TryServer.
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?
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.)
> 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....
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...
... that test actually tests some scripted changes too.  I'll see if I can
add some more cases.
Add more tests to content/html/content/test/test_bug388558.html
Now also tests input events, and scripted changes from onfocus/
oninput/onblur.
Comment on attachment 409070 [details] [diff] [review]
Patch rev. 2

Olli, maybe you could review this instead of Boris?
Attachment #409070 - Flags: review?(Olli.Pettay)
Mats, did you test undo/redo handling?
I think undo/redo should work just fine, but better to test.
I'll test once my build is ready.
Attachment #409070 - Flags: review?(Olli.Pettay) → review+
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 :(
Whiteboard: [needs review] → [needs landing]
http://hg.mozilla.org/mozilla-central/rev/cce7954f1b99

The tests still need to be landed.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 192 landing]
(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)
You need to log in before you can comment on or make changes to this bug.