Issue metadata
Sign in to add a comment
|
Dragging and dropping text that includes a colon navigates instead of searches |
||||||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/50.0.2661.94 Safari/537.36 Steps to reproduce the problem: 1. Select the following text - Prototype: a JavaScript framework 2. Drag it into the tab strip. What is the expected behavior? A new tab (or an existing tab) with search results for "Prototype: a JavaScript framework". What went wrong? It apparently tries to navigate to that text, as if it were a URL. It lower-cases the capital P at the beginning of the text (prototype: a JavaScript framework). Did this work before? N/A Chrome version: 50.0.2661.94 Channel: stable OS Version: 6.1 (Windows 7, Windows Server 2008 R2) Flash Version: Shockwave Flash 21.0 r0 Selecting the text within the omnibox and pressing Enter would indeed search and not navigate, so the logic is there already, it just does not apply to drag and drop. Also, when you right click on the selected text, it shows "Search Google for "Prototype: a JavaScript framework"" and not "Go to Prototype: a JavaScript framework".
,
May 10 2016
I believe all of these problems should've been fixed recently by the same change that fixed bug 606341 . Can someone please verify? (I don't have a current checkout.)
,
May 10 2016
Fix for bug 606341 only changed url_fixer logic with trying to substitute ';' to ':' so this is the different kind of bug and I can reproduce it in latest canary build. Maybe I can look at it in my spare time.
,
May 10 2016
,
May 24 2016
This problem occurs because when text gets dragged and dropped on a toolbar, at first it tries to be parsed as url and only if that doesn't succeed, autocomplete starts. Win, Linux: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/views/frame/browser_root_view.cc&q=browser_root_&sq=package:chromium&type=cs&l=105 Mac: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/cocoa/url_drop_target.mm&sq=package:chromium&type=cs&l=87&q=url_drop_t It historically works this way, I wasn't able to find any reasoning behind it (url dropping logic dates back to 2009). I suggest, we do autocomplete and select top match on drag and drop. If this sounds reasonable, I will create a patch.
,
May 24 2016
CC pkasting, for insight on why the code is this way. He certainly reviewed this code ( https://codereview.chromium.org/202010 ) by sky@: "Fixes regression introduced in dnd refactoring. I removed a check for the URL that I shouldn't have (it won't be needed at some point, but it is now)." and pkasting@ likely rewrote the original code that sky@ refers to.
,
May 24 2016
Are you saying the call to GetURLAndTitle() returns a URL? Normally I would think that means the clipboard explicitly contains a URL object and not just a text string. Is that not true? My worry here is that if we were to remove that, and someone dragged in something that was a link whose text string was not the link URL, we might end up searching for the text string instead of navigating to the underlying link.
,
May 25 2016
1) I rechecked on all platforms, on Linux this problem does not reproduce. My bad. 2) @pkasting, you are right. GetURLAndTitle does return a URL and this is at least weird. Here is how it happens: Windows: if type isn't url OSExchangeDataProviderWin::GetURLAndTitle calls GetPlainTextURL, which calls GURL parser, to determine if text is URL: https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/dragdrop/os_exchange_data_provider_win.cc&sq=package:chromium&type=cs&l=421 https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/dragdrop/os_exchange_data_provider_win.cc&sq=package:chromium&type=cs&l=231 Mac: Same thing, pasteboard util will try to parse text: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/mozilla/NSPasteboard%2BUtils.mm&sq=package:chromium&type=cs&l=264&q=NsPastebo Linux: This bug doesn't reproduce because GetURLAndTitle doesn't attempt to parse text. It just checks for paste type: https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/dragdrop/os_exchange_data_provider_aurax11.cc&sq=package:chromium&type=cs&l=244&rcl=1464117624
,
May 25 2016
I won't comment on the Mac code, but that seems very wrong on Windows. I think we should simply not return a URL object if the clipboard didn't contain one, though we'd have to audit the current callers to see what kind of effect this would have. The alternative is to fix things up more using the autocomplete machinery, but I think that's worse.
,
May 25 2016
Will it (not returning a URL object if the clipboard did not contain one) make dragging non-linkified URLs not navigate anymore? Assuming Monorail does not linkify the following - abc.xyz - will dragging abc.xyz to the tab strip still navigate there? Same question for (the surely linkified in Monorail) http://abc.xyz? Or will you simply use the same logic that the context menu uses for "Search for" and "Go to"? (Which answers "yes" to my two questions)
,
May 25 2016
1) @pkasting, if I understood you correctly, you suggest to remove attempts to parse url, at least in Windows code. I'll look into consequences of doing so tomorrow. If it seems so that it won't break existing logic, will this be an acceptable solution? 2) @phistuck, it will work same way, as if one typed it in omnibox. Because http://abc.xyz and abc.xyz are most likely to be urls, one'll get a navigation to a website.
,
May 25 2016
Yes, as long as there aren't bad consequences, I think removing the existing "let GURL parse the input" code on Windows should fix the issue here while still letting dragged-in text act as if you did paste-and-search/paste-and-go in the omnibox.
,
May 26 2016
So, I've checked. I think, we'll need an extra function to do URL/Title in text parcing These places are somewhat in question: Win/ Linux: What with dragging text-file urls? https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/views/tabs/tab_strip.cc&q=GetURLAndTitle&sq=package:chromium&l=1619&ct=rc&cd=13&dr=C Home button: parsing urls seems like a good idea: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/views/toolbar/home_button.cc&q=GetURLAndTitle&sq=package:chromium&l=172&ct=rc&cd=17&dr=C Bookmark, I suggest parsing urls: https://code.google.com/p/chromium/codesearch#chromium/src/components/bookmarks/browser/bookmark_node_data_views.cc&q=GetURLAndTitle&sq=package:chromium&l=60&ct=rc&cd=23&dr=C Mac: Adding a bookmark, I suggest parsing urls: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view_cocoa.mm&q=getURLs&sq=package:chromium&dr=C&l=270 https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_view.mm&q=getURLs&sq=package:chromium&l=138&ct=rc&cd=29&dr=C File urls, I do not know what to do: https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/dragdrop/cocoa_dnd_util.mm&q=getURLs&sq=package:chromium&l=32&ct=rc&cd=8&dr=C https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/cocoa/drag_util.mm&sq=package:chromium&dr=C&q=PopulateURLAndTitleFromPasteboard&l=101 https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/cocoa/url_drop_target.mm&q=GetFileURLFromDropData&sq=package:chromium&type=cs&l=60
,
May 26 2016
,
May 28 2016
,
Jun 3 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dc27d8d8169bfafc95df1eaf5359efaf54a18957 commit dc27d8d8169bfafc95df1eaf5359efaf54a18957 Author: dyaroshev <dyaroshev@yandex-team.ru> Date: Fri Jun 03 12:15:29 2016 Removing parsing of text from pasteboard. This patch modifies logic for drag and dropping text that can be parsed as URL. Previously pasteboard would unconditionally answer that it contains URL. For some cases it is better to process such text using PasteAndGo logic. R=mark@chromium.org, pkasting@chromium.org BUG=610620 Review-Url: https://codereview.chromium.org/2014733003 Cr-Commit-Position: refs/heads/master@{#397689} [modify] https://crrev.com/dc27d8d8169bfafc95df1eaf5359efaf54a18957/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_view.mm [modify] https://crrev.com/dc27d8d8169bfafc95df1eaf5359efaf54a18957/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_folder_view_unittest.mm [modify] https://crrev.com/dc27d8d8169bfafc95df1eaf5359efaf54a18957/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view_cocoa.mm [modify] https://crrev.com/dc27d8d8169bfafc95df1eaf5359efaf54a18957/chrome/browser/ui/cocoa/bookmarks/bookmark_bar_view_cocoa_unittest.mm [modify] https://crrev.com/dc27d8d8169bfafc95df1eaf5359efaf54a18957/chrome/browser/ui/cocoa/drag_util.mm [modify] https://crrev.com/dc27d8d8169bfafc95df1eaf5359efaf54a18957/chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm [modify] https://crrev.com/dc27d8d8169bfafc95df1eaf5359efaf54a18957/chrome/browser/ui/cocoa/url_drop_target.mm [add] https://crrev.com/dc27d8d8169bfafc95df1eaf5359efaf54a18957/chrome/browser/ui/cocoa/url_drop_target_unittest.mm [modify] https://crrev.com/dc27d8d8169bfafc95df1eaf5359efaf54a18957/chrome/chrome_tests_unit.gypi [modify] https://crrev.com/dc27d8d8169bfafc95df1eaf5359efaf54a18957/content/browser/web_contents/web_drag_dest_mac.mm [modify] https://crrev.com/dc27d8d8169bfafc95df1eaf5359efaf54a18957/third_party/mozilla/NSPasteboard+Utils.h [modify] https://crrev.com/dc27d8d8169bfafc95df1eaf5359efaf54a18957/third_party/mozilla/NSPasteboard+Utils.mm [modify] https://crrev.com/dc27d8d8169bfafc95df1eaf5359efaf54a18957/third_party/mozilla/README.chromium [modify] https://crrev.com/dc27d8d8169bfafc95df1eaf5359efaf54a18957/ui/base/clipboard/clipboard_util_mac_unittest.mm [modify] https://crrev.com/dc27d8d8169bfafc95df1eaf5359efaf54a18957/ui/base/dragdrop/cocoa_dnd_util.mm
,
Jun 3 2016
Looks like the patch is for Macintosh, while the issue is reported for Windows. Just saying.
,
Sep 9 2016
The views version https://codereview.chromium.org/2322253004/
,
Nov 5 2016
,
Jun 15 2017
Will reassess this next year. This is an issue but apparently fixing it will take a major refactoring. (The natural solution, expressed on the linked changelist, will add unnatural dependencies between components.)
,
Dec 20 2017
,
Jan 9 2018
The NextAction date has arrived: 2018-01-09
,
Dec 11
Issue 892990 has been merged into this issue.
,
Dec 11
Still happens; see merged-in bug. Assigning to same owner as other bug.
,
Jan 14
|
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by phistuck@chromium.org
, May 10 2016Status: Untriaged (was: Unconfirmed)