MacViews: Bookmark manager dragging issues |
|||||||||||||||||||||||
Issue descriptionA 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.
,
Sep 4
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...!!
,
Sep 4
,
Sep 4
,
Sep 4
,
Sep 4
I'm going to work on the issue. But since users have workaround for this use case I remove RBS.
,
Sep 4
,
Sep 4
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.
,
Sep 5
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!
,
Sep 5
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?
,
Sep 5
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?
,
Sep 5
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.
,
Sep 5
,
Sep 5
,
Sep 6
,
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
,
Sep 8
Issue 882148 has been merged into this issue.
,
Sep 11
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?
,
Sep 11
,
Sep 11
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
,
Sep 11
Still repros for me on 71.0.3549.0 macOS 10.13.6
,
Sep 11
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?
,
Sep 11
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)
,
Sep 12
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?
,
Sep 13
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
,
Sep 13
,
Sep 13
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
,
Sep 14
,
Sep 14
,
Sep 14
,
Sep 14
Has the CL landed in canary? Has it been tested and verified? and specifically which CL needs to be merged to M70?
,
Sep 15
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.
,
Sep 18
,
Sep 18
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
,
Sep 28
,
Oct 2
,
Oct 2
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 .
,
Oct 4
891194 is now closed. Please try your patch again; everything should be hunky-dory on the Mac. If not, poke me again.
,
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
,
Nov 1
I think this can be closed. Please reopen if anything else pops up.
,
Jan 2
I am able to reproduce this on Chrome Version 71.0.3578.98 (Official Build) (64-bit). Possible regression?
,
Jan 3
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!
,
Jan 3
Yes, new bug with screencast here: https://bugs.chromium.org/p/chromium/issues/detail?id=918854 . 🙏🙏🙏 |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by calamity@chromium.org
, Sep 3