1918907 - Once a bookmark is dragging over itself or outside of browser window while dragging, it cannot be dropped into the content area.

Status: VERIFIED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect, P2)

Description

[Alice0775 White]

Reproduced on Firefox128.3ESR(20240913150006) and Nightly132.0a1(20240914211959) Windows11 and Ubuntu22.04.

Steps to reproduce:

1. Start Firefox with normal sizemode
2. Drag start a bookmark on the bookmarks toolbar.
3. Drag over itself or outside of browser window
4. Then drop it into contentarea

Actual results:
Cannot drop.
Screencast: https://youtu.be/H0Vz0z_kOM0

The following error appears when the bug occurs.

TypeError: can't access property "types", dataTransfer is null 75 ContentAreaDropListener.sys.mjs:235:17
    canDropLink resource://gre/modules/ContentAreaDropListener.sys.mjs:235

Expected results:
Can drop

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=9ac7f1f672d585d3a66062be012715bfafbb1e4d&tochange=01bfc9faa69043308206f123a4f171b426aa2758

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

:handyman, since you are the author of the regressor, bug 1893119, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

[Alice0775 White]

Also, if you pass over a toolbutton while dragging a local file, you will not be able to drop the file into the contentarea.

STR:

1. Drag start a local file from file explorer
2. Drag over on the toolbuttons such as reload button and magnifying glass in the location bar.
3. Try drop it onto contentarea

Screencast: https://youtu.be/YkInQknWmos

Comment 3

[Alice0775 White]

[Tracking Requested - why for this release]: Drag and drop stops working in some case.

Pls backed out the offending patch from 128.3ESR if not fix this bug.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Backing out the regressing change isn't practical at this point due to a large number of dependent changes which landed on top of it.

Comment 5

[David Parks [:handyman]]

It looks like the content process is (properly) releasing its drag session when the drag leaves the content region, but the parent process doesn't stop "tracking" the content drag session because, from its perspective, the content and the drag source bookmark are part of the same widget. Therefore, when the drag returns to the widget, the parent believes content already has a drag session (nsBaseDragSession::MaybeAddBrowser returns false), so it doesn't create one, which is wrong. nsContentUtils::SetDataTransferInEvent then cannot create a DT, and the error results.

My first thought is that the parent should stop tracking the content drag session when the drag leaves content (i.e. when content calls EndDragSession). Right now, that stuff is aligned by the belief that the processes are synchronized by the behavior, but we may actually need to synchronize here (like, by sending an IPDL message when EndDragSession runs in content).

Comment 6

[David Parks [:handyman]]

Attachment: Bug 1918907: Don't clear drag session target BrowserChild on eDragLeave/eDragExit r=m_kato!

The BrowserChild should only be cleared on EndDragSession, which is sent when
the user finishes or cancels dragging. This is reciprocal to it being set in
StartDragSession.

The change to nsDragSessionProxy::EndDragSessionImpl is non-functional -- it
highlights the BrowserChild symmetry above.

Comment 7

[David Parks [:handyman]]

This continues the old behavior (pre-bug 1893119) instead of doing something new and more complicated. This leaves the drag sessions alive (but dormant) when the drag exists their widget.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The bug is marked as tracked for firefox131 (beta) and tracked for firefox132 (nightly). However, the bug still has low severity.

:hsinyi, could you please increase the severity for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 10

[Dianna Smith [:diannaS]]

:m_kato do you think you could take a look at the attached patch? we have one beta left this cycle in order to land this

Comment 11

[Dianna Smith [:diannaS]]

:gstoll do you think you can check if this is good to land now? I noticed handyman is on pto

Comment 12

[Greg Stoll :gstoll]

Yes, I just queued it on Lando, sorry I missed this earlier!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e042c75c31a
Don't clear drag session target BrowserChild on eDragLeave/eDragExit r=m_kato

Comment 14

[Dianna Smith [:diannaS]]

Thank you! Can you also set the nomination for beta/esr. It probably wont make b9 but I Can still uplift before RC week

Comment 15

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/6e042c75c31a

Comment 16

[Greg Stoll :gstoll]

Attachment: Bug 1918907: Don't clear drag session target BrowserChild on eDragLeave/eDragExit r=m_kato!

The BrowserChild should only be cleared on EndDragSession, which is sent when
the user finishes or cancels dragging. This is reciprocal to it being set in
StartDragSession.

The change to nsDragSessionProxy::EndDragSessionImpl is non-functional -- it
highlights the BrowserChild symmetry above.

Original Revision: https://phabricator.services.mozilla.com/D222389

Comment 17

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: drag and drop won't work when dragging in and out of windows
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: yes
• Steps to reproduce for manual QE testing: see STR in bug (and the duplicate bug)
• Risk associated with taking this patch: medium
• Explanation of risk level: changes drag and drop behavior which is notoriously finicky.
• String changes made/needed: no
• Is Android affected?: no

Comment 18

[Greg Stoll :gstoll]

Attachment: Bug 1918907: Don't clear drag session target BrowserChild on eDragLeave/eDragExit r=m_kato!

The BrowserChild should only be cleared on EndDragSession, which is sent when
the user finishes or cancels dragging. This is reciprocal to it being set in
StartDragSession.

The change to nsDragSessionProxy::EndDragSessionImpl is non-functional -- it
highlights the BrowserChild symmetry above.

Original Revision: https://phabricator.services.mozilla.com/D222389

Comment 19

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: drag and drop won't work when dragging in and out of windows
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: yes
• Steps to reproduce for manual QE testing: see STR in bug (and the duplicate bug)
• Risk associated with taking this patch: medium
• Explanation of risk level: changes drag and drop behavior which is notoriously finicky.
• String changes made/needed: no
• Is Android affected?: no

Comment 20

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/3f5e259fbfd9

Comment 21

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/d3fc5b03abb8

Comment 22

[Monica Chiorean]

I was able to reproduce the issue on Win11x64 using Firefox build 132.0a1(20240914211959).
Verified as fixed on Win11x64/Ubuntu 24.04 using Firefox builds: 132.0a1(20240922213737).
Pending verification on 131.0 and 128.3.0.

Comment 23

[Monica Chiorean]

Verified as fixed on Win11x64/Ubuntu 24.04 using Firefox builds 131.0 and 128.3.0esr.