New issue
Advanced search Search tips

Issue 610620 link

Starred by 9 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-01-09
OS: Windows
Pri: 3
Type: Bug


Show other hotlists

Hotlists containing this issue:
Hotlist-1


Sign in to add a comment

Dragging and dropping text that includes a colon navigates instead of searches

Project Member Reported by phistuck@gmail.com, May 10 2016

Issue description

UserAgent: 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".
 
Components: -UI UI>Browser>TabStrip UI>Browser>Omnibox
Status: Untriaged (was: Unconfirmed)
Cc: a-...@yandex-team.ru
Components: -UI>Browser>TabStrip
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.)

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.

Labels: -Pri-2 Pri-3
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.
Cc: pkasting@chromium.org
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.
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.
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

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.

Comment 10 by phistuck@gmail.com, 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)
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.
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.
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
Components: Blink>DataTransfer
Project Member

Comment 16 by bugdroid1@chromium.org, 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

Looks like the patch is for Macintosh, while the issue is reported for Windows. Just saying.
Components: -Blink>DataTransfer UI>Browser>TabStrip
Labels: Hotlist-Polish
NextAction: 2018-01-09
Status: Available (was: Untriaged)
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.)

Cc: jdonnelly@chromium.org
 Issue 794576  has been merged into this issue.
The NextAction date has arrived: 2018-01-09
Cc: manukh@chromium.org viswa.karala@chromium.org davidbienvenu@chromium.org
 Issue 892990  has been merged into this issue.
Owner: manukh@chromium.org
Status: Assigned (was: Available)
Still happens; see merged-in bug. Assigning to same owner as other bug.
Status: Started (was: Assigned)

Sign in to add a comment