Closed
Bug 280635
Opened 20 years ago
Closed 5 years ago
Text box receiving text via drag-and-drop does not get focus
Categories
(Core :: DOM: Editor, defect)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla73
People
(Reporter: cj10, Assigned: masayuki)
References
(Blocks 3 open bugs)
Details
(Keywords: regression, testcase)
Attachments
(5 files, 5 obsolete files)
1.14 KB,
text/html
|
Details | |
1.15 KB,
text/html
|
Details | |
29.78 KB,
patch
|
Details | Diff | Splinter Review | |
6.95 KB,
patch
|
enndeakin
:
review-
|
Details | Diff | Splinter Review |
3.75 KB,
text/html
|
Details |
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.
Reporter | ||
Comment 1•20 years ago
|
||
Updated•20 years ago
|
Assignee: firefox → mozeditor
Component: General → Editor
Product: Firefox → Core
QA Contact: general → bugzilla
Version: unspecified → Trunk
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/
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.
Status: UNCONFIRMED → RESOLVED
Closed: 19 years ago
Resolution: --- → EXPIRED
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?
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9a1?
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.
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.
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...
Keywords: helpwanted
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.
Comment 10•19 years ago
|
||
OK, so who knows editor well enough to try to do something like that? ;)
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.
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?
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
Attachment #206501 -
Attachment is obsolete: true
This seems IMHO like it should be done by the editor code that recieves the drop rather then the transfer object.
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?
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?
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.
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.
Attachment #206954 -
Flags: review?(brade)
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.
Updated•19 years ago
|
Attachment #206954 -
Flags: review?(brade)
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).
Attachment #206954 -
Attachment is obsolete: true
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
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.
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?
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?
Comment 26•19 years ago
|
||
Patch3 is causing a crash when a textarea something like this: onfocus="this.value='foo-bar'".
Comment 27•19 years ago
|
||
A more elaborate testcase
Attachment #208197 -
Attachment is obsolete: true
Attachment #209114 -
Attachment is obsolete: true
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.
Comment 29•19 years ago
|
||
Actually, I think the first issue in comment 28 would be fixed by bug 128953.
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.
Attachment #213985 -
Attachment is obsolete: true
Updated•18 years ago
|
Flags: blocking1.9a1? → blocking1.9-
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.
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 :-(
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. :(
Updated•18 years ago
|
QA Contact: bugzilla → editor
Updated•18 years ago
|
Assignee: mozeditor → nobody
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)
Assignee: nobody → matspal
Attachment #409262 -
Flags: review?(bzbarsky)
Updated•15 years ago
|
Comment 35•15 years ago
|
||
Comment 36•15 years ago
|
||
Comment on attachment 409262 [details] [diff] [review]
Patch6
This patch + bug 522787 rev.2 passed regression tests on TryServer.
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)...
Attachment #409262 -
Flags: review?(bzbarsky) → review?(enndeakin)
Updated•15 years ago
|
Attachment #409262 -
Flags: review?(enndeakin) → review-
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?
Comment 39•5 years ago
|
||
Bug 1597829 might fix this issue.
Assignee | ||
Comment 40•5 years ago
|
||
Fixed by bug 1597829. Please file new bugs if you find remaining problems.
Assignee: mats → masayuki
Status: NEW → ASSIGNED
status-firefox71:
--- → wontfix
status-firefox72:
--- → wontfix
status-firefox73:
--- → fixed
status-firefox-esr68:
--- → wontfix
Target Milestone: --- → mozilla73
Assignee | ||
Updated•5 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 19 years ago → 5 years ago
Resolution: --- → FIXED
Comment 41•5 years ago
|
||
Bugbug thinks this bug is a regression, but please revert this change in case of error.
Keywords: regression
Updated•5 years ago
|
QA Whiteboard: [qa-73b-p2]
You need to log in
before you can comment on or make changes to this bug.
Description
•