Charles Jardine Reporter
	Description • 20 years ago

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

In Firefox 1.0, when text arrived in a text box vie drag and drop,
the text box does not receive the focus.

On contrast, in IE, the box does receive the focus, and the dropped
text is selected.

The IE behaviour is better because:

1. IE preserves the invarinat that a text box's value can be changed
by the operator only when the box has focus.

2. Firefox allows the operator to submit a form containing a changed
text box value without triggering any of the event handlers associated
with the box.

This second aspect of the Firefox behaviou is liable to break and
JavaScript application whihc depends on the event handleres of individual
fomr elements.

Reproducible: Always

Steps to Reproduce:
I will add an HTML page to this bug which demonstrates the problem.

	Gervase Markham [:gerv]
	Comment 2 • 19 years ago

This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/

	Gervase Markham [:gerv]
	Comment 3 • 19 years ago

This bug has been automatically resolved after a period of inactivity (see above
comment). If anyone thinks this is incorrect, they should feel free to reopen it.

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 19 years ago

No, this is a real bug.  Ideally, we would just fire onchange when the drop
happened.  Can we do that?

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 19 years ago

Trying to not lose track of this for 1.9.  The platform aspect is important, as
pointed out in comment 0.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 6 • 19 years ago

What it sounds like IE does makes a lot of sense. That is, fire focus, paste
text, fire onchange.

I'm not sure if selecting the text between pasting and firing onchange is very
practical. Possibly we should follow platform standards there, if there are any.

	Charles Jardine Reporter
	Comment 7 • 19 years ago

Simply firing onchange after the drop has occured would be better than
nothing, but not ideal. It would not allow the operator to further edit
the dropped material before commiting to the change. It would break
the rule that, for a text box, onchange only occurs when the box blurs.

I wish to argue that the IE behaviour is desirable. To be precise, in IE6,
after the drop, the box into which the text was dropped has the focus,
the dropped text is selected, and the focus handler has fired. The onchange
handler has not fired, but will when the box next blurs.

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 19 years ago

So the question is whether we want this focus-stealing behavior for all editors
or just for text/password inputs and textareas...

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 9 • 19 years ago

That and if we should select the dropped content. Actually, when I think about
it it makes a lot of sense to select the contents since the user will then
easily see the result of his operation. This is especially usefull if the user
misstakenly dropped something in the wrong place.

This would be an argument for doing the same in all editors IMHO.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 19 years ago

OK, so who knows editor well enough to try to do something like that?  ;)

	Martijn Wargers (dead)
	Comment 11 • 19 years ago

Well, I tried this. It shifts the focus, but not allways to the dropped location. I'm not sure why if focuses other elements sometimes/ a lot of times.

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 19 years ago

Is the newSelectionParent the anonymous div inside the textbox or something?  And is this really something the _editor_ should be doing?  That is, should this code perhaps live in nsTextControlFrame?

	Martijn Wargers (dead)
	Comment 13 • 19 years ago

Well, this patch actually works, and selects the dragged text afterwards.
Really not sure if it is the right way to fix it, though.
It doesn't work for designmode iframes/editors, I suppose something similar would be needed then in nsHTMLDataTransfer.cpp

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 14 • 19 years ago

This seems IMHO like it should be done by the editor code that recieves the drop rather then the transfer object.

	Martijn Wargers (dead)
	Comment 15 • 19 years ago

Well, all the functions in nsPlaintextDataTransfer.cpp are methods of nsPlaintextEditor, so doesn't that mean it is editor code that I'm changing?

	Boris Zbarsky [:bzbarsky]
	Comment 16 • 19 years ago

This seems like the right general thing... Mats, Aaron, would you be willing to check over the focus-changing part of this?

	Kathleen :Brade
	Comment 17 • 19 years ago

This seems related to bug 128953 but perhaps only in that I expect both to have patches in nsPlaintextDataTransfer.cpp (and maybe nsHTMLDataTransfer.cpp).  I'm happy to review any editor changes.

	Martijn Wargers (dead)
	Comment 18 • 19 years ago

Comment on attachment 206954 [details] [diff] [review]
This one works v1

Great, thanks Brade!

I know I need to fix up nsHTMLDataTransfer.cpp also, but first I want to know if this is the right approach.

	Martijn Wargers (dead)
	Comment 19 • 19 years ago

So maybe bug 128066 could also get fixed by this.

	Martijn Wargers (dead)
	Comment 20 • 19 years ago

The patch isn't really ready for review. Brade, I just would like to have your thoughts on it.
I need also change the patch to do what was mentioned in bug 248862, comment c9   (second part of that comment), because that's just way more simpler code.

	Martijn Wargers (dead)
	Comment 21 • 19 years ago

Ok, I moved the stuff into InsertTextFromTransferable, I think it actually makes more sense there. With a aDestinationNode check I can see if this is a drag or not.
I also patched nsHTMLDataTransfer.cpp, but there I still have some problems.
- I can't really focus the designMode iframe/editor yet, it looks like it gets focus, but it doesn't really have it.
- I dragged a link into the designMode iframe, but the dragged node didn't bemome selected (afaik, it works for other circumstances).

	Kathleen :Brade
	Comment 22 • 19 years ago

Comment on attachment 208197 [details] [diff] [review]
patchv2

A few quick comments:
 * Please add more comments :-)

 * I don't like the early return; I'd rather see something like "if (aDestinationNode) { stuff here } return rv;"
   The early return should only be for failure.

 * The editor has its own call to GetSelection (rather than going through the frames and such); I'm not sure if that is beneficial/helpful.

Ideas for things to test:
 * drag and drop within a document (drop before and after the dragged text)
 * drag and drop from another editor into the text field
 * drag and drop from same browser (non-editor) window
 * drag and drop from a different browser window
 * drag and drop from another application (non-Mozilla based)
 * copy-paste in each of the above situations

	Martijn Wargers (dead)
	Comment 23 • 19 years ago

Still not ready.

In this patch, I've moved the focusing code when dropping, into the eventstatemanager. Is that a good idea (I think so). This code works fine, but I'm not sure if this is correct code.
One issue with this is that it can focus xul elements that should not be focusable, but that could also be bug 324156, I'm not sure.

> A few quick comments:
>  * Please add more comments :-)
Will do, when/if I am able to make a patch ready for review (in this patch I've already added some XXX comments)

>  * I don't like the early return; I'd rather see something like "if
> (aDestinationNode) { stuff here } return rv;"
>    The early return should only be for failure.
Fixed.

>  * The editor has its own call to GetSelection (rather than going through the
> frames and such); I'm not sure if that is beneficial/helpful.
I've done that (it's cleaner), but with this I get some selection issues in special cases afterwards, like not selecting the dropped text. I think I get it mostly when dragging links/images inside the plaintexteditor.

	Kathleen :Brade
	Comment 24 • 19 years ago

Comment on attachment 209114 [details] [diff] [review]
patch3

A few quick comments...

You'll need to find someone else (different module owner) for the nsEventStateManager.cpp changes.

I'm not sure I agree with pulling the "stuffToPaste" out of the if blocks.  Instead you should add your code after the InsertTextAt call:
  if (NS_SUCCEEDED(rv) && aDestinationNode) {
  ...

Do you really want to know where the caret was in the previous selection or where the caret was during the drop event?  (I'm not sure what you're trying to do in this block of text.)

In nsHTMLDataTransfer.cpp, remove the blank line you  added (around line 378).  Also another blank line around 780.

I'm a little concerned about the outright removal of selection->Collapse call (around line 780); in particular, it was collapsing 1 beyond the selection offset.  Was that wrong or is there a special case that it is handling?

Rather than fix the comment (around line 1630) I'd rather make it compatible with other applications.  Is that possible?

	Martijn Wargers (dead)
	Comment 25 • 19 years ago

(In reply to comment #24)
> I'm not sure I agree with pulling the "stuffToPaste" out of the if blocks. 
> Instead you should add your code after the InsertTextAt call:
>   if (NS_SUCCEEDED(rv) && aDestinationNode) {
Ok, will do that.

> Do you really want to know where the caret was in the previous selection or
> where the caret was during the drop event?  (I'm not sure what you're trying to do in this block of text.)
Yes, see bug 312675 for example. It could be that the caret remains visible, while you have a selection (layout.selection.caret_style = 1), and when you drag -drop a selection, you don't want it to suddenly appear at the other side of the selection.

> In nsHTMLDataTransfer.cpp, remove the blank line you  added (around line 378). 
> Also another blank line around 780.
Error in the patch, will fix it.

> I'm a little concerned about the outright removal of selection->Collapse call
> (around line 780); in particular, it was collapsing 1 beyond the selection
> offset.  Was that wrong or is there a special case that it is handling?
Sorry, that's also an error in the patch, will fix it.

> Rather than fix the comment (around line 1630) I'd rather make it compatible
> with other applications.  Is that possible?
The other applications on windows are not compatible with each other. Editpad seems to remove the existing selection, while HTML-Kit collapses the existing selection before dropping.
IE6 also collapses the selection, but it does that at the moment when you're dragging into the window (it decollapses the selection, when you drag again out of the window, quite nifty).
So maybe we should do appr. something like IE6? It would also be more compatible with what Mozilla does now. But I guess there are also platform specific rules here, perhaps. Maybe just file a new bug on this?

	Martijn Wargers (dead)
	Comment 26 • 19 years ago

Patch3 is causing a crash when a textarea something like this: onfocus="this.value='foo-bar'".

	Martijn Wargers (dead)
	Comment 27 • 19 years ago

A more elaborate testcase

	Martijn Wargers (dead)
	Comment 28 • 19 years ago

For some reason, the dragdrop code in eventstatemanager isn't called when dropping something in a designmode iframe.
So with this patch I just reverted back to putting the focus on drop code inside nsplaintextdatatransfer.cpp and nshtmldatatransfer.cpp, and now it's working just fine for designMode iframes.

This patch works correctly now for textarea's and text inputs, the dropped text is now always selected.
For designMode iframes it's not working 100% correctly. It works 100% correctly for text, but for special cases the selecting after drop code doesn't work well.

I'll list current problems with the patch here:
- when something is selected in a textarea/text input, and then you drag something from outside the textarea/text input into it, then the selection after drop is not correct. I would like to solve this by collapsing the existing selection in the textarea before dropping text (IE6 has the nifty solution I mentioned in comment 25). This would also be what designMode iframes are doing currently.
- Double click in link to select, then dragdrop somewhere else, the dropped link   is not selected.
- Start selecting somewhere and end with an image, dragdrop this selection somewhere, the resulting selection is more than what it was.

	Martijn Wargers (dead)
	Comment 29 • 19 years ago

Actually, I think the first issue in comment 28 would be fixed by bug 128953.

	Martijn Wargers (dead)
	Comment 30 • 19 years ago

This fixes the second issue in comment 28.
The third issue I can't seem to fix. It's really strange. When setting the code to collapse on either side, that is working fine, but when using extend, I suddenly get a weird selection. I get a feeling this is somehow a (different) bug in Mozilla. Something involved with replaced elements, because I get the same issue with a text input.

Btw, I found another bug: Dragdropping multiline text from the textarea to the designMode iframe and from the dropped text, the first line is not selected.

	Francis
	Comment 31 • 18 years ago

has this bug (and appropriate fix) been approved for any release of firefox and/or seamonkey? I'm hitting bug 128066 which Martijn says (and from what I read I agree) is related to it.

	Charles Jardine Reporter
	Comment 32 • 18 years ago

I am the original poster of this bug. It is now more than two years old.

I am still suffering from the same problem. If I drop some text into a 
text box in a form, none of the event handlers associated with the
box fire.

I have a system for updating databases which depends on using AJAX
to process changes to form widgets as the occur, in the correct order.
The bug breaks this system. Changes made by drag-and-drop are noticed
too late, if at all. I cannot program round the bug, because the my
JavaScript cannot get control at an appropriate time.

So, I would be very interested in a fix. I don't like having to tell my
users that the system works better with IE than it does with FireFox :-(

	Boris Zbarsky [:bzbarsky]
	Comment 33 • 18 years ago

Francis, there is no fix yet.  Martijn proposed a patch, but it doesn't fix things completely apparently...  See comment 30.

Charles, the problem is that editor is effectively not being worked on for about 6 years now, because everyone who knew anything about it moved on to other things about then.  So fixes to it happen rather slowly.  :(

	Mats Palmgren (inactive)
	Comment 34 • 15 years ago

I propose that we split this bug in two parts.
1. focus the drop target
2. the selection adjustments

This patch fixes the first part.  Together with the fix in
bug 522787 the events are as follows:  drop, focus, input,
and then when the control loses focus we get: change, blur.
(thus fixing bug 128066 as well)

	Mats Palmgren (inactive)
	Comment 36 • 15 years ago

Comment on attachment 409262 [details] [diff] [review]
Patch6

This patch + bug 522787 rev.2 passed regression tests on TryServer.

	Boris Zbarsky [:bzbarsky]
	Comment 37 • 15 years ago

Comment on attachment 409262 [details] [diff] [review]
Patch6

enn is a better reviewer for this (and in particular for where that gtk chunk should live, or whether it should even exist)...

	Neil Deakin
	Comment 38 • 15 years ago

Comment on attachment 409262 [details] [diff] [review]
Patch6

>+  NS_PRECONDITION(aContent, "");
>+
>+  nsCOMPtr<nsIContent> newFocus = aContent->FindFirstNonNativeAnonymous();

What element is this looking for? Can you add a comment here?


>+#ifdef MOZ_WIDGET_GTK2
>+        // GTK lowered the window when the drag started and will raise it
>+        // on drop, but that hasn't happened yet.  nsFocusManager::SetFocus()
>+        // requires an active window to have any effect so raise the window now.
>+        nsCOMPtr<nsPIDOMWindow> window = newFocus->GetCurrentDoc()->GetWindow();
>+        nsCOMPtr<nsIWebNavigation> webnav = do_GetInterface(window);
>+        nsCOMPtr<nsIDocShellTreeItem> dsti = do_QueryInterface(webnav);
>+        if (dsti) {
>+          nsCOMPtr<nsIDocShellTreeItem> root;
>+          dsti->GetRootTreeItem(getter_AddRefs(root));
>+          nsCOMPtr<nsPIDOMWindow> rootWindow = do_GetInterface(root);
>+          fm->WindowRaised(rootWindow);
>+        }
>+#endif

This should definitely not be calling WindowRaised. That will make the focus manager think a window that isn't actually raised is active, which I'm sure will cause confusion in some way later on.

What is this gtk specific part trying to fix?