Closed
Bug 948839
Opened 11 years ago
Closed 11 years ago
Drag and Drop broken
Categories
(Core :: DOM: Events, defect)
Tracking
()
VERIFIED
FIXED
mozilla29
Tracking | Status | |
---|---|---|
firefox27 | --- | unaffected |
firefox28 | + | verified |
firefox29 | - | verified |
People
(Reporter: alice0775, Assigned: masayuki)
References
Details
(Keywords: regression)
Attachments
(1 file)
(deleted),
patch
|
smaug
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Steps To Reproduce #1:
1. Select text
2. Drag the selected text
3. Drop on <input> or <textarea>
Actual Results:
Search by default search engine
Expected Results:
Dragged test should be copied to input/textarea
Steps To Reproduce #2:
1. Type some text in <input> or <textarea>
2. Select text inside the <input> or <textarea>
3. Drag the selected text
4. Drop on the <input> or <textarea>
Actual Results:
Search by default search engine
Expected Results:
Dragged test should be moved
Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/06b3a7aea2c0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131208154802
Bad:
http://hg.mozilla.org/mozilla-central/rev/551efcc4de6f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131208185601
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=06b3a7aea2c0&tochange=551efcc4de6f
Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/046cf0c4f858
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131207204100
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2aaff66026ba
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131208075200
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=046cf0c4f858&tochange=2aaff66026ba
Reporter | ||
Comment 1•11 years ago
|
||
I wonder that no automatic test is provided.
Reporter | ||
Comment 2•11 years ago
|
||
Steps To Reproduce #3:
1. Drag alink
2. Drop on the <input> or <textarea>
Actual Results:
Link opened in current tab
Expected Results:
Link url should be copied to input/textarea
Reporter | ||
Comment 3•11 years ago
|
||
Steps To Reproduce #4:
1. Drag a file from explorer
2. Drop on the <input type="file"> ("Browse.." button or "No file selected" label)
Actual Results:
file opened in current tab
Expected Results:
file path should be copied to <input type="file">
Assignee | ||
Comment 4•11 years ago
|
||
I've checked the removed code again.
http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsDOMEvent.cpp?rev=5721f317bab1#442
> 442 // Need to set an extra flag for drag events.
> 443 if (mEvent->eventStructType == NS_DRAG_EVENT && IsTrusted()) {
> 444 nsCOMPtr<nsINode> node = do_QueryInterface(mEvent->currentTarget);
> 445 if (!node) {
> 446 nsCOMPtr<nsPIDOMWindow> win = do_QueryInterface(mEvent->currentTarget);
> 447 if (win) {
> 448 node = win->GetExtantDoc();
> 449 }
> 450 }
> 451 if (node && !nsContentUtils::IsChromeDoc(node->OwnerDoc())) {
> 452 mEvent->mFlags.mDefaultPreventedByContent = true;
> 453 }
> 454 }
Perhaps, we misunderstand this code. mDefaultPreventedByContent is wrong name. mDefaultPreventedByContent became true when the event is consumed *on* content.
I guess that we should add mDefaultPreventedOnContent to the WidgetDragEvent and recover the code with it. How do you think, smaug?
Flags: needinfo?(bugs)
Assignee | ||
Updated•11 years ago
|
status-firefox27:
--- → unaffected
status-firefox28:
--- → affected
status-firefox29:
--- → affected
Comment 5•11 years ago
|
||
hmm, what is calling preventDefault()? The original idea for that code was that if content calls
preventDefault() we handle that case differently.
Does chrome add drag listener to content?
But
Flags: needinfo?(bugs)
Comment 6•11 years ago
|
||
er,
But for FF28 adding mDefaultPreventedOnContent to WidgetDragEvent should be ok.
And sorry, I should have looked at this more when I suggested using mDefaultPreventedByContent.
Assignee | ||
Comment 8•11 years ago
|
||
The callers of preventDefault() are:
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditorEventListener.cpp?rev=c9879b47c32c#624
http://mxr.mozilla.org/mozilla-central/source/layout/forms/nsFileControlFrame.cpp?rev=2ae6663f558c#193
I think that they should mark special flag for dragover event themselves. However, this mechanism may be used by others including add-ons. So, I believe that we cannot take this approach for fixing this.
Assignee | ||
Comment 9•11 years ago
|
||
(In reply to Alice0775 White from comment #1)
> I wonder that no automatic test is provided.
It's difficult to test of the behavior of DnD especially at dragging special object...
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Assignee | ||
Comment 10•11 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #5)
> hmm, what is calling preventDefault()? The original idea for that code was
> that if content calls
> preventDefault() we handle that case differently.
> Does chrome add drag listener to content?
Yes, the callers of preventDefault() is C++ handler (and can be chrome's event listener). Even in such cases, if the currentTarget of the dragover event belongs content, mDefaultPreventedByContent was set true.
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #8346419 -
Flags: review?(bugs)
Assignee | ||
Comment 12•11 years ago
|
||
Comment on attachment 8346419 [details] [diff] [review]
Patch
oops, I forgot to change IPC.
Attachment #8346419 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 13•11 years ago
|
||
Comment on attachment 8346419 [details] [diff] [review]
Patch
Sorry for the spam. There is no code for WidgetDragEvent in nsGUIEventIPC.h :-(
Attachment #8346419 -
Flags: review- → review?(bugs)
Updated•11 years ago
|
Attachment #8346419 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 14•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 18•11 years ago
|
||
Masayuki, we want this to Aurora too, right?
Assignee | ||
Comment 19•11 years ago
|
||
Absolutely. But I want to wait a couple of days for confirming no regression on m-c.
Assignee | ||
Updated•11 years ago
|
Assignee | ||
Comment 21•11 years ago
|
||
Comment on attachment 8346419 [details] [diff] [review]
Patch
[Approval Request Comment]
Bug caused by (feature/regressing bug #):
bug 930374. It removed necessary code accidentally.
User impact if declined:
User cannot drop files/URLs on <input type="file">, <input type="text">, etc. The dropped item always causes open in the tab.
Testing completed (on m-c, etc.):
Landed on the last week.
Risk to taking this patch (and alternatives if risky):
No idea because this patch just restores the removed code.
String or IDL/UUID changes made by this patch:
No.
Attachment #8346419 -
Flags: approval-mozilla-aurora?
Updated•11 years ago
|
Updated•11 years ago
|
Attachment #8346419 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 22•11 years ago
|
||
Comment 23•11 years ago
|
||
Alice, please confirm this is working for you now in Firefox 28 and 29.
Flags: needinfo?(alice0775)
Comment 24•11 years ago
|
||
This broke for me upgrading to 28.0a2. I can't find a nightly regression window, but I can reproduce it with Aurora on two different computers. This one is using 20131219004003 and the other is still on 20131214004001. This one was still on 27.0a2 20131206004003 and was working fine, broke after updating.
This should have the dataloss keyword, I have been hitting this while composing messages for work, needing to move some text around, and then I'm navigated away. When I hit back the form data is gone :(
Reporter | ||
Comment 25•11 years ago
|
||
(In reply to Majken Connor [Kensie] from comment #24)
> This broke for me upgrading to 28.0a2. I can't find a nightly regression
> window, but I can reproduce it with Aurora on two different computers. This
> one is using 20131219004003 and the other is still on 20131214004001. This
> one was still on 27.0a2 20131206004003 and was working fine, broke after
> updating.
>
> This should have the dataloss keyword, I have been hitting this while
> composing messages for work, needing to move some text around, and then I'm
> navigated away. When I hit back the form data is gone :(
The nightly build of Aurora28.0a2 is not released yet.
Maybe it avairable today's nightly(2013-Dec-20).
Flags: needinfo?(alice0775)
Reporter | ||
Comment 26•11 years ago
|
||
Correcting:
The nightly build of Aurora28.0a2 which is included this patch is not released yet.
Maybe it avairable today's nightly(2013-Dec-20).
And also you can try latest tinderbox build(include this patch) in http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-aurora-win32/
Reporter | ||
Comment 27•11 years ago
|
||
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #23)
> Alice, please confirm this is working for you now in Firefox 28 and 29.
I cannot reproduce the problem with STR#1-#4.
And I verified that the problem was fixed.
http://hg.mozilla.org/mozilla-central/rev/5c7fa2bfea8b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20131219030202
http://hg.mozilla.org/releases/mozilla-aurora/rev/40c4cb09bce5
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:28.0) Gecko/20100101 Firefox/28.0 ID:20131220004002
Comment 28•11 years ago
|
||
(In reply to Majken Connor [Kensie] from comment #24)
> This broke for me upgrading to 28.0a2. I can't find a nightly regression
> window, but I can reproduce it with Aurora on two different computers. This
> one is using 20131219004003 and the other is still on 20131214004001. This
> one was still on 27.0a2 20131206004003 and was working fine, broke after
> updating.
This should be fixed in Firefox Nightly 29.0a1 as of 2013-12-13 and Firefox Aurora 28 as of 2013-12-20. I'm marking this verified fixed based on Alice's testing but please do reopen if this isn't working for you after updating to today's Aurora.
Comment 29•11 years ago
|
||
Thanks, just updated, it is working for me as well.
You need to log in
before you can comment on or make changes to this bug.
Description
•