New issue
Advanced search Search tips

MacViews: Bookmark manager dragging issues

Project Member Reported by lgrey@chromium.org, Aug 28

Issue description

A couple of (probably) related issues:

- Attempting to drag bookmarks out of the web content (to the bookmark bar, or to the desktop) doesn't work. Instead, the drag image is masked by the content frame.

- Under some circumstances, dragging a bookmark in the bookmark manager enters the dragging code without a drag image I'm not familiar enough with how to debug WebUI code to figure out what's going on here but basically:
 - Most of the time, dragging from the Bookmark Manager does *not* go through chrome::DragBookmarks
 - ...but, if you randomly drag bookmarks over and over, at some point, a path gets triggered where it *does* go through chrome::DragBookmarks
 - ...at which point, a drag is initiated with no drag image. AppKit doesn't like this, especially on 10.14
 - This is continued from Issue 876201. The crash is now fixed, but the underlying issue is not.
 
Labels: Needs-Bisect
chrome::DragBookmarks should be triggered when the bookmark is dragged outside the web content area, as we pass off the in-contents drag to the system native drag at that point.

AFAICT, this drag behavior got broken somewhere between 68.0.3440.106 and 69.0.3497.72, for all platforms.

The 'dragstart' event used to be fired only when the mouse was moved outside the content area, due to a e.preventDefault() in mousemove. It seems that the preventDefault no longer prevents the dragstart event from firing, so it fires early.

I suspect some subtle mouse event cancellation changes have happened in the past few milestones.

===================

QA, Can we please get a bisect?

Steps:
1. Create some bookmarks in the bookmark manager
2. Drag bookmarks to bookmarks bar

Works in 68.0.3440.106, broken in 69.0.3497.72
Cc: jmukthavaram@chromium.org nyerramilli@chromium.org
Labels: -Type-Bug -Pri-2 -Needs-Bisect hasbisect-per-revision M-69 Target-70 Target-71 RegressedIn-69 FoundIn-71 FoundIn-70 Target-69 FoundIn-69 OS-Linux OS-Windows Pri-1 Type-Bug-Regression
Owner: nzolghadr@chromium.org
Status: Assigned (was: Available)
Able to reproduce the issue on Windows 10, mac 10.13.6 and debian rodate using chrome reported version #69.0.3497.72 and latest canary #71.0.3541.0 as per C#1.

Steps:
------
1. Create multiple bookmarks on any other folder other than bookmarks bar.
2. Open Bookmark manager & try to drag bookmarks from other folder to bookmark bar
3. Observed that we are unable to drag & paste those to bookmark bar & unable to drag to desktop also

on Stable-68.0.3440.106, We are able to drag to bokkmark bar & desktop successfully.

Bisect Information:
=====================
Good build: 69.0.3476.0 
Bad Build : 69.0.3477.0 

Change Log URL: 
https://chromium.googlesource.com/chromium/src/+/76a593e26e0e5e1d962bbcc95562490e56901787

From the above change log suspecting below change
Change-Id: I71f2c808f224be23ffbc80ee25e2897a9dc9f090
Reviewed-on: https://chromium-review.googlesource.com/1112628

nzolghadr@ - Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner.

Note:
It is not mac views specific issue.Without enabling any flags & without mac views behavior seen this issue on chrome normally.

Thanks...!!
878392-Bad behavior.mp4
2.3 MB View Download
Cc: pbomm...@chromium.org gov...@chromium.org
Labels: ReleaseBlock-Stable
Cc: lgrey@chromium.org ellyjo...@chromium.org
 Issue 879787  has been merged into this issue.
Labels: -ReleaseBlock-Stable
I'm going to work on the issue. But since users have workaround for this use case I remove RBS.
Components: Blink>Input
Cc: nzolghadr@chromium.org
Owner: calamity@chromium.org
Chris, as far as I found the problem is with this line of the code:
https://cs.chromium.org/chromium/src/chrome/browser/resources/md_bookmarks/dnd_manager.js?type=cs&q=startNativeDrag_&sq=package:chromium&g=0&l=498

It is assuming mousemove prevents browser native drag operation. This is a non-standard behavior of Chrome that we removed in the previous patch.
Can you look into this issue and see whether you can fix this in the bookmark code. I couldn't figure how to fix it.
Let me know if you wouldn't be able to fix this and I can try reverting the original patch for one release.
For context, we were using an page-internal mousemove-based drag, which started a native drag when the internal drag moved outside the web contents. This allowed us to have a rich drag image, and a very responsive drag-drop experience (native drag events only fire every 300ms or so).

For some reason, the native drag's setDragImage API doesn't seem to work reliably so we avoided using it. https://codepen.io/webgeeker/full/KBzrxE/ doesn't set the drag image reliably in my testing.

@nzolghadr: I think it would be best to revert the patch for a release while we figure out a long-term fix for this. Thanks!
Is that alright if I only do the revert on 69?
Note that 70 is also branched at this point and in a few weeks is going to hit beta. Will you be able to figure out a way for 70?
My current plan is to just use the native drag and drop APIs. This will result in a laggier DnD experience, but can work with this compatibility patch.

As a blocker, we need the drag image API to work properly. Either way, any fix will be too large to merge to M70, so it will be best to revert there as well.

I'll aim to have a fix in for M71 branch, but I don't have many cycles to work on this.

====

Aside: Does this mean there is no spec-compliant way to defer the dragstart event?

Cc: tommycli@chromium.org
Based on offline group chat,
Per pbommana@ & lgrey@, this is not that common use case. 
Per tommycli@ this should not be an RBS, a P2 type of bug and probably worth a merge to M70. 

So unless this issue is wider spread with high impact and revert is fully safe to merge to M69, we won't be able to take this merge in for M69 respin (if any). Pls target fix/merge for M70. Pls let me know if there is any concern here. 

Blockedon: 880687
Labels: Hotlist-ConOps
Owner: nzolghadr@chromium.org
Status: Started (was: Assigned)
Project Member

Comment 16 by bugdroid1@chromium.org, Sep 6

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/854915e83341ad92449dd13165fccaf4b83317a3

commit 854915e83341ad92449dd13165fccaf4b83317a3
Author: Navid Zolghadr <nzolghadr@chromium.org>
Date: Thu Sep 06 17:41:02 2018

Revert "Remove selection as default action of mousemove"

This CL is not a straight revert of
https://chromium.googlesource.com/chromium/src/+/76a593e26e0e5e1d962bbcc95562490e56901787

but it essentially recovers that functionality.
The revert is needed due to a bug in Chrome bookmark
manager.

Bug:  878392 
Change-Id: Ifa5867691ffe88ed557fb548a5de572ec5236580
Reviewed-on: https://chromium-review.googlesource.com/1210124
Commit-Queue: Navid Zolghadr <nzolghadr@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Ella Ge <eirage@chromium.org>
Reviewed-by: Lan Wei <lanwei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589208}
[modify] https://crrev.com/854915e83341ad92449dd13165fccaf4b83317a3/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/854915e83341ad92449dd13165fccaf4b83317a3/third_party/blink/renderer/core/input/event_handler.cc

 Issue 882148  has been merged into this issue.
This issue if fixed in Canary now. I'm going to merge this to 70. Can someone else also give it a try and verify the fix?
Labels: Merge-Request-70
Project Member

Comment 20 by sheriffbot@chromium.org, Sep 11

Labels: -Merge-Request-70 Merge-Review-70 Hotlist-Merge-Review
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Still repros for me on 71.0.3549.0 macOS 10.13.6
I don't have the Canary on Mac yet. For some reason it quits on me and doesn't come up at this point. I'll wait for the next version. But I see the issue fixed on Linux. Just to confirm I'm testing dragging a bookmark to the url bar. What scenario do you test?
Both dragging to the bookmark bar and to the desktop.

Does Canary launch with --disable-features=NetworkService ? There have been some issues related to it today (though not on launch that I've heard of)
calamity@ I was able to also see some issue on latest Canary on Mac. Although on Linux everything is fine. I don't think this issue is related to my change anymore. Could you take a look?
Owner: calamity@chromium.org
My wild guess is that the drag stack for mac broke when they switched it onto the Views platform.

I have a CL in progress to make the drag fully native.

https://chromium-review.googlesource.com/c/chromium/src/+/1221166
Labels: Merge-Request-70
Project Member

Comment 27 by sheriffbot@chromium.org, Sep 13

Labels: -Merge-Request-70
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: Merge-Request-69
Labels: -Merge-Request-69
Blocking: 346473
Has the CL landed in canary? Has it been tested and verified? and specifically which CL needs to be merged to M70?
Yes, it is in c#16, I have verified the effect and it resolved the issue as far as the effect of the original CL is concerned.
Labels: -Merge-Review-70 Merge-Approved-70
Project Member

Comment 34 by bugdroid1@chromium.org, Sep 18

Labels: -merge-approved-70 merge-merged-3538
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/66b189fb9f657553e89cf0400ac2584859868514

commit 66b189fb9f657553e89cf0400ac2584859868514
Author: Navid Zolghadr <nzolghadr@chromium.org>
Date: Tue Sep 18 14:20:41 2018

Revert "Remove selection as default action of mousemove"

This CL is not a straight revert of
https://chromium.googlesource.com/chromium/src/+/76a593e26e0e5e1d962bbcc95562490e56901787

but it essentially recovers that functionality.
The revert is needed due to a bug in Chrome bookmark
manager.

Bug:  878392 
Change-Id: Ifa5867691ffe88ed557fb548a5de572ec5236580
Reviewed-on: https://chromium-review.googlesource.com/1210124
Commit-Queue: Navid Zolghadr <nzolghadr@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Ella Ge <eirage@chromium.org>
Reviewed-by: Lan Wei <lanwei@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#589208}(cherry picked from commit 854915e83341ad92449dd13165fccaf4b83317a3)
Reviewed-on: https://chromium-review.googlesource.com/1230574
Reviewed-by: Navid Zolghadr <nzolghadr@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#489}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/66b189fb9f657553e89cf0400ac2584859868514/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/66b189fb9f657553e89cf0400ac2584859868514/third_party/blink/renderer/core/input/event_handler.cc

Cc: susan.boorgula@chromium.org
 Issue 890127  has been merged into this issue.
Blockedon: 891194
I have a CL to use native drag up at

https://chromium-review.googlesource.com/c/chromium/src/+/1221166

but it exacerbates an issue with Mac's native bookmark dragging from  Issue 891194 .
Cc: a...@chromium.org
891194 is now closed.

Please try your patch again; everything should be hunky-dory on the Mac. If not, poke me again.
Project Member

Comment 39 by bugdroid1@chromium.org, Oct 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1ebfa2b597832dc94c9f3012871c5a92dd7b35b3

commit 1ebfa2b597832dc94c9f3012871c5a92dd7b35b3
Author: Christopher Lam <calamity@chromium.org>
Date: Tue Oct 09 01:37:19 2018

[MD Bookmarks] Implement drag drop chip in Views.

This CL deletes the JS implementation of the drag drop chip in the
Bookmark Manager and replaces it with a native drag. This decouples
the drag implementation from certain non-standard web drag behavior
which is getting removed.

Bug:  878392 ,  881336 
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: I862f2b675d371ed0a5a60fbbdbfe72a9c02cbd36
Reviewed-on: https://chromium-review.googlesource.com/c/1221166
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Ben Wells <benwells@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Commit-Queue: calamity <calamity@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597767}
[modify] https://crrev.com/1ebfa2b597832dc94c9f3012871c5a92dd7b35b3/chrome/browser/browser_resources.grd
[modify] https://crrev.com/1ebfa2b597832dc94c9f3012871c5a92dd7b35b3/chrome/browser/extensions/api/bookmark_manager_private/bookmark_manager_private_api.cc
[modify] https://crrev.com/1ebfa2b597832dc94c9f3012871c5a92dd7b35b3/chrome/browser/resources/md_bookmarks/BUILD.gn
[delete] https://crrev.com/22ed4853bf2323d2a64ba07f00f9814b51afcd40/chrome/browser/resources/md_bookmarks/dnd_chip.html
[delete] https://crrev.com/22ed4853bf2323d2a64ba07f00f9814b51afcd40/chrome/browser/resources/md_bookmarks/dnd_chip.js
[modify] https://crrev.com/1ebfa2b597832dc94c9f3012871c5a92dd7b35b3/chrome/browser/resources/md_bookmarks/dnd_manager.html
[modify] https://crrev.com/1ebfa2b597832dc94c9f3012871c5a92dd7b35b3/chrome/browser/resources/md_bookmarks/dnd_manager.js
[modify] https://crrev.com/1ebfa2b597832dc94c9f3012871c5a92dd7b35b3/chrome/browser/ui/bookmarks/bookmark_browsertest.cc
[modify] https://crrev.com/1ebfa2b597832dc94c9f3012871c5a92dd7b35b3/chrome/browser/ui/bookmarks/bookmark_drag_drop.cc
[modify] https://crrev.com/1ebfa2b597832dc94c9f3012871c5a92dd7b35b3/chrome/browser/ui/bookmarks/bookmark_drag_drop.h
[modify] https://crrev.com/1ebfa2b597832dc94c9f3012871c5a92dd7b35b3/chrome/browser/ui/views/bookmarks/bookmark_drag_drop_views.cc
[modify] https://crrev.com/1ebfa2b597832dc94c9f3012871c5a92dd7b35b3/chrome/browser/ui/webui/md_bookmarks/md_bookmarks_ui.cc
[modify] https://crrev.com/1ebfa2b597832dc94c9f3012871c5a92dd7b35b3/chrome/common/extensions/api/bookmark_manager_private.json
[modify] https://crrev.com/1ebfa2b597832dc94c9f3012871c5a92dd7b35b3/chrome/test/data/webui/md_bookmarks/dnd_manager_test.js
[modify] https://crrev.com/1ebfa2b597832dc94c9f3012871c5a92dd7b35b3/third_party/closure_compiler/externs/bookmark_manager_private.js
[modify] https://crrev.com/1ebfa2b597832dc94c9f3012871c5a92dd7b35b3/ui/gfx/color_palette.h

Status: Fixed (was: Started)
I think this can be closed. Please reopen if anything else pops up.
I am able to reproduce this on Chrome Version 71.0.3578.98 (Official Build) (64-bit).  Possible regression?
Works in 73 for me, can you file a new bug with some repro instructions and platform details? A screencast would be particularly good. Thanks!
Yes, new bug with screencast here: https://bugs.chromium.org/p/chromium/issues/detail?id=918854 . 🙏🙏🙏

Sign in to add a comment