Closed Bug 1552814 Opened 5 years ago Closed 5 years ago

When drag-dropping notes in Google Keep Notes, they open instead of just dropping

Categories

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

Product:
Core
Component:
DOM: Copy & Paste and Drag & Drop
Version:
66 Branch
Platform:
Desktop
All
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla71
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- fixed

People

(Reporter: ravenak2, Assigned: smaug)

References

(Depends on 1 open bug, Regression,
URL
)

Details

(Keywords: regression, Whiteboard: [sci-exclude])

Attachments

(3 files)

6.gif
570.65 KB, image/gif
Details
Self-contained testcase
724 bytes, text/html
Details
Bug 1552814, make it possible to start dnd from dragable element even if DataTransfer is empty, r=NeilDeakin
47 bytes, text/x-phabricator-request
Details | Review
Attached image 6.gif —

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:66.0) Gecko/20100101 Firefox/66.0

Steps to reproduce:

  • Go to https://keep.google.com/ in Firefox on PC and log into your Google account.
  • Try to drag-and-drop a note to a different position.

Actual results:

The note will move, and on dropping, it will also open.

Expected results:

The note should only have dropped, not opened. Does not happen in Google Chrome.

Component: Untriaged → General
OS: Unspecified → Windows 7
Component: General → Untriaged

Hi @AdHoc, I've tested the issue and here are the results:
[Platform affected]: Windows 7, 10, Ubuntu 18.04 and Mac OS X
[FF versions affected]: release 67.0, beta 68.0b3, nightly 69.0a1

  • on all cases the issue can be reproduced.
    I will set a component to it and if isn't a proper one please fell free to change it.
    Notes: this issue happens only with text added notes, exception make picture notes. It can be confirmed that on other browser such Chrome the issue won't occur.
Status: UNCONFIRMED → NEW
status-firefox67: --- → affected
status-firefox68: --- → affected
status-firefox69: --- → affected
Component: Untriaged → Drag and Drop
Ever confirmed: true
OS: Windows 7 → All
Product: Firefox → Core
Hardware: Unspecified → Desktop

Drag and drop API isn't used here. Would need a minimal testcase to identify where this bug belongs, although it is more likely a site issue.

Component: Drag and Drop → General

The priority flag is not set for this bug.
:tvandermerwe, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(tvandermerwe)

Setting as P3 for now by redirecting to Gijs to have closer look.

Flags: needinfo?(tvandermerwe) → needinfo?(gijskruitbosch+bugs)
Priority: -- → P3

I'm not the right person for this type of issue - hopefully our web compat team can take a look.

Component: General → Desktop
Flags: needinfo?(gijskruitbosch+bugs)
Priority: P3 → --
Product: Core → Web Compatibility
Version: 66 Branch → Firefox 66

Ksenia, could you take a look?

URL: https://keep.google.com/
Flags: needinfo?(kberezina)
Priority: -- → P3

This issue still reproduces as of today.

Emailed Google about this issue.

Response from Google:

Following up on this - after some investigation, it seems this was caused by a behavior change in Firefox.

Sample fiddle: https://jsfiddle.net/m4wu2qs6/

If you drag an item and then drop it, on Firefox 60.9.0esr, the following set of events are fired:

  1. mousedown
  2. dragstart
  3. mouseup (to indicate the drag was complete)

On Firefox 68, the following set of events are fired:

  1. mousedown
  2. dragstart
  3. mouseup
  4. click <- This is what causes the issue on our end

On Chrome 77:

  1. mousedown
  2. dragstart
  3. dragend

A number of products within Google seem to have been hit by the same issue and fixed it by preventing any click that happens shortly after the mouseup that indicated drag end on Firefox. We're looking into doing something similar on Keep as well.

A fix for this issue will start appearing in production on September 30th.

Neil: should we have a follow-up issue to change our dnd implementation for what was identified in the fiddle / comment #9 ?

Flags: needinfo?(enndeakin)

The behavior change from comment 9 happens in this commit range: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e27e7c02c708b052a53e39d35d81d8318d8730f1&tochange=5c892a6147ae856f208e77c265eda4d7b677ac52

In there, the only change to ESM is bug 1089326 which is a very plausible regressor: we are now firing a click at the common ancestor of the mousedown and mouseup elements, when we did not use to.

That said, the old behavior looks broken to me too: I would have expected a dragend if we dispatched dragstart...

Flags: needinfo?(masayuki)
Flags: needinfo?(bugs)
Regressed by: 1089326

(In reply to :Gijs (he/him) from comment #11)

Neil: should we have a follow-up issue to change our dnd implementation for what was identified in the fiddle / comment #9 ?

Yes, we shouldn't be firing click there. We can just use this bug I think.

Flags: needinfo?(enndeakin)
Component: Desktop → Drag and Drop
Product: Web Compatibility → Core
Version: Firefox 66 → 66 Branch

Hmm, this must be an undefined edge case between UI Events (defining click event) and Living Standard (defining D&D). I feel that click event should not be fired if D&D is handled. Since the operation is obviously D&D, not clicking any element. However, there is not such definition:
https://w3c.github.io/uievents/#event-type-click
https://html.spec.whatwg.org/multipage/dnd.html
Perhaps, this should be a spec bug. Olli, what do you think?

Flags: needinfo?(masayuki)

It is draggable which behaves oddly, less so click.

Depends on: 1585606

FWIW, I'm looking this, and the issue, at least with the testcase, is that draggable=true without event listener, which explicitly would add some data to drag, doesn't trigger dnd since we explicitly return early when there isn't data. Chrome seems to trigger dnd in such case and per spec it seems to be allowed.
Locally letting dnd to be triggered makes the testcase work like in Chrome.

Chrome and old Edge at least seem to have this behavior, and this way the testcase on the bug doesn't trigger click anymore since
we enter dnd mode and get dragleave etc. events.

Manually tested on linux and Windows, and annyg tested on Mac

A brief scan suggests this is a better fix than the one in 1352852. I'm assuming this won't have the issues that patch had.

Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c9b71d1747ea
make it possible to start dnd from dragable element even if DataTransfer is empty, r=NeilDeakin

Backed out changeset c9b71d1747ea (Bug 1552814) for assertion failures on nsBaseDragService.cpp

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=3d0ff856543c67800892069f34a2f18c4e437a85&tochange=b739370f346d53aafce4709bb5ce8dc040edca2f&selectedJob=270085928

Backout link: https://hg.mozilla.org/integration/autoland/rev/b739370f346d53aafce4709bb5ce8dc040edca2f

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=270085928&repo=autoland&lineNumber=3147

[task 2019-10-07T16:00:49.902Z] 16:00:49 INFO - TEST-START | dom/events/test/test_dragstart.html
[task 2019-10-07T16:00:50.019Z] 16:00:50 INFO - GECKO(1159) | ++DOMWINDOW == 11 (0x7fc322a6d000) [pid = 1236] [serial = 343] [outer = 0x7fc32a2bd100]
[task 2019-10-07T16:00:50.035Z] 16:00:50 INFO - GECKO(1159) | [Child 1236, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /builds/worker/workspace/build/src/dom/base/ThirdPartyUtil.cpp, line 217
[task 2019-10-07T16:00:50.036Z] 16:00:50 INFO - GECKO(1159) | [Child 1236, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /builds/worker/workspace/build/src/dom/base/ThirdPartyUtil.cpp, line 217
[task 2019-10-07T16:00:50.051Z] 16:00:50 INFO - GECKO(1159) | [Child 1236, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /builds/worker/workspace/build/src/dom/base/ThirdPartyUtil.cpp, line 217
[task 2019-10-07T16:00:50.234Z] 16:00:50 INFO - GECKO(1159) | MEMORY STAT | vsize 2620MB | residentFast 225MB | heapAllocated 40MB
[task 2019-10-07T16:00:50.250Z] 16:00:50 INFO - GECKO(1159) | --DOMWINDOW == 10 (0x7fc325338400) [pid = 1236] [serial = 338] [outer = (nil)] [url = http://mochi.test:8888/tests/SimpleTest/iframe-between-tests.html]
[task 2019-10-07T16:00:50.251Z] 16:00:50 INFO - TEST-OK | dom/events/test/test_dragstart.html | took 349ms
[task 2019-10-07T16:00:50.273Z] 16:00:50 INFO - GECKO(1159) | Assertion failure: !xpc::IsInAutomation() (About to start drag-drop native loop on which will prevent later tests from running properly.), at /builds/worker/workspace/build/src/widget/nsBaseDragService.cpp:234
[task 2019-10-07T16:01:10.190Z] 16:01:10 INFO - GECKO(1159) | #01: nsBaseDragService::InvokeDragSessionWithRemoteImage(nsINode*, nsIPrincipal*, nsIContentSecurityPolicy*, nsIArray*, unsigned int, mozilla::dom::RemoteDragStartData*, mozilla::dom::DragEvent*, mozilla::dom::DataTransfer*) [widget/nsBaseDragService.cpp:364]
[task 2019-10-07T16:01:10.190Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.191Z] 16:01:10 INFO - GECKO(1159) | #02: mozilla::EventStateManager::DoDefaultDragStart(nsPresContext*, mozilla::WidgetDragEvent*, mozilla::dom::DataTransfer*, bool, nsIContent*, mozilla::dom::Selection*, mozilla::dom::RemoteDragStartData*, nsIPrincipal*, nsIContentSecurityPolicy*) [dom/events/EventStateManager.cpp:2147]
[task 2019-10-07T16:01:10.191Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.192Z] 16:01:10 INFO - GECKO(1159) | #03: mozilla::EventStateManager::GenerateDragGesture(nsPresContext*, mozilla::WidgetInputEvent*) [dom/events/EventStateManager.cpp:1948]
[task 2019-10-07T16:01:10.192Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.193Z] 16:01:10 INFO - GECKO(1159) | #04: mozilla::EventStateManager::PreHandleEvent(nsPresContext*, mozilla::WidgetEvent*, nsIFrame*, nsIContent*, nsEventStatus*, nsIContent*) [dom/events/EventStateManager.cpp:0]
[task 2019-10-07T16:01:10.193Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.194Z] 16:01:10 INFO - GECKO(1159) | #05: mozilla::PresShell::EventHandler::DispatchEvent(mozilla::EventStateManager*, mozilla::WidgetEvent*, bool, nsEventStatus*, nsIContent*) [layout/base/PresShell.cpp:7798]
[task 2019-10-07T16:01:10.194Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.195Z] 16:01:10 INFO - GECKO(1159) | #06: mozilla::PresShell::EventHandler::HandleEventWithCurrentEventInfo(mozilla::WidgetEvent*, nsEventStatus*, bool, nsIContent*) [layout/base/PresShell.cpp:7767]
[task 2019-10-07T16:01:10.195Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.195Z] 16:01:10 INFO - GECKO(1159) | #07: mozilla::PresShell::EventHandler::HandleEventUsingCoordinates(nsIFrame*, mozilla::WidgetGUIEvent*, nsEventStatus*, bool) [layout/base/PresShell.cpp:6726]
[task 2019-10-07T16:01:10.195Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.199Z] 16:01:10 INFO - GECKO(1159) | #08: mozilla::PresShell::EventHandler::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*) [layout/base/PresShell.cpp:6531]
[task 2019-10-07T16:01:10.199Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.199Z] 16:01:10 INFO - GECKO(1159) | #09: mozilla::PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*) [layout/base/PresShell.cpp:6457]
[task 2019-10-07T16:01:10.199Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.199Z] 16:01:10 INFO - GECKO(1159) | #10: nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*) [view/nsViewManager.cpp:750]
[task 2019-10-07T16:01:10.199Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.199Z] 16:01:10 INFO - GECKO(1159) | #11: mozilla::PresShell::DispatchSynthMouseMove(mozilla::WidgetGUIEvent*) [layout/base/PresShell.cpp:3665]
[task 2019-10-07T16:01:10.199Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.199Z] 16:01:10 INFO - GECKO(1159) | #12: mozilla::PresShell::ProcessSynthMouseMoveEvent(bool) [layout/base/PresShell.cpp:5463]
[task 2019-10-07T16:01:10.199Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.199Z] 16:01:10 INFO - GECKO(1159) | #13: mozilla::PresShell::nsSynthMouseMoveEvent::WillRefresh(mozilla::TimeStamp) [layout/base/PresShell.h:1951]
[task 2019-10-07T16:01:10.199Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.199Z] 16:01:10 INFO - GECKO(1159) | #14: nsRefreshDriver::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) [mfbt/WeakPtr.h:316]
[task 2019-10-07T16:01:10.199Z] 16:01:10 INFO - GECKO(1159) | #13: mozilla::PresShell::nsSynthMouseMoveEvent::WillRefresh(mozilla::TimeStamp) [layout/base/PresShell.h:1951]
[task 2019-10-07T16:01:10.199Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.199Z] 16:01:10 INFO - GECKO(1159) | #14: nsRefreshDriver::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) [mfbt/WeakPtr.h:316]
[task 2019-10-07T16:01:10.199Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.199Z] 16:01:10 INFO - GECKO(1159) | #15: mozilla::RefreshDriverTimer::TickRefreshDrivers(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) [layout/base/nsRefreshDriver.cpp:344]
[task 2019-10-07T16:01:10.199Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.199Z] 16:01:10 INFO - GECKO(1159) | #16: mozilla::RefreshDriverTimer::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) [layout/base/nsRefreshDriver.cpp:369]
[task 2019-10-07T16:01:10.199Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.203Z] 16:01:10 INFO - GECKO(1159) | #17: mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) [layout/base/nsRefreshDriver.cpp:729]
[task 2019-10-07T16:01:10.203Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.203Z] 16:01:10 INFO - GECKO(1159) | #18: mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() [layout/base/nsRefreshDriver.cpp:526]
[task 2019-10-07T16:01:10.204Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.204Z] 16:01:10 INFO - GECKO(1159) | #19: nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:1225]
[task 2019-10-07T16:01:10.204Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.205Z] 16:01:10 INFO - GECKO(1159) | #20: NS_ProcessNextEvent(nsIThread*, bool) [xpcom/threads/nsThreadUtils.cpp:486]
[task 2019-10-07T16:01:10.205Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.205Z] 16:01:10 INFO - GECKO(1159) | #21: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:88]
[task 2019-10-07T16:01:10.206Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.206Z] 16:01:10 INFO - GECKO(1159) | #22: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:315]
[task 2019-10-07T16:01:10.206Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.207Z] 16:01:10 INFO - GECKO(1159) | #23: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:291]
[task 2019-10-07T16:01:10.207Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.207Z] 16:01:10 INFO - GECKO(1159) | #24: nsBaseAppShell::Run() [widget/nsBaseAppShell.cpp:139]
[task 2019-10-07T16:01:10.208Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.208Z] 16:01:10 INFO - GECKO(1159) | #25: nsAppStartup::Run() [toolkit/components/startup/nsAppStartup.cpp:277]
[task 2019-10-07T16:01:10.208Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.209Z] 16:01:10 INFO - GECKO(1159) | #26: XREMain::XRE_mainRun() [toolkit/xre/nsAppRunner.cpp:4600]
[task 2019-10-07T16:01:10.209Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.210Z] 16:01:10 INFO - GECKO(1159) | #27: XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) [toolkit/xre/nsAppRunner.cpp:4735]
[task 2019-10-07T16:01:10.210Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.210Z] 16:01:10 INFO - GECKO(1159) | #28: XRE_main(int, char**, mozilla::BootstrapConfig const&) [toolkit/xre/nsAppRunner.cpp:4816]
[task 2019-10-07T16:01:10.210Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.210Z] 16:01:10 INFO - GECKO(1159) | #29: _fini
[task 2019-10-07T16:01:10.210Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.210Z] 16:01:10 INFO - GECKO(1159) | #30: libc.so.6 + 0x20830
[task 2019-10-07T16:01:10.210Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.210Z] 16:01:10 INFO - GECKO(1159) | #31: _fini
[task 2019-10-07T16:01:10.210Z] 16:01:10 INFO -
[task 2019-10-07T16:01:10.210Z] 16:01:10 INFO - GECKO(1159) | ExceptionHandler::GenerateDump cloned child 1343
[task 2019-10-07T16:01:10.210Z] 16:01:10 INFO - GECKO(1159) | ExceptionHandler::SendContinueSignalToChild sent continue signal to child
[task 2019-10-07T16:01:10.211Z] 16:01:10 INFO - GECKO(1159) | ExceptionHandler::WaitForContinueSignal waiting for continue signal...
[task 2019-10-07T16:01:10.211Z] 16:01:10 INFO - GECKO(1159) | [GFX1-]: Receive IPC close with reason=AbnormalShutdown
....

It looks like Google's work around for this is live. I'm unable to reproduce the original issue on keep.

Flags: needinfo?(kberezina)
Whiteboard: [sci-exclude]

Good, but need to fix this anyhow.
Looking at the xpc::IsInAutomation() failure.

Flags: needinfo?(bugs)

ah, need to update one test, which is missing a preventDefault() in one case.

Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/402db246b36e
make it possible to start dnd from dragable element even if DataTransfer is empty, r=NeilDeakin

https://hg.mozilla.org/mozilla-central/rev/402db246b36e

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox71: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Assignee: nobody → bugs
QA Whiteboard: [qa-71b-p2]

Is this something we should consider for ESR68 uplift or can we live without it until the next release next year? It would need rebasing if the answer is yes to uplifting.

Flags: needinfo?(bugs)

We could consider this for esr once it has gotten some more real world testing.

Flags: needinfo?(bugs)

Any thoughts on this since comment 27/28?

URL: https://keep.google.com/ → https://keep.google.com/
Flags: needinfo?(bugs)

Hmm, that patch does require quite some updates to apply to esr68. I'll take a look

status-firefox-esr68: affected → wontfix
Flags: needinfo?(bugs)
Depends on: 1629578
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: