Closed Bug 429166 Opened 12 years ago Closed 9 years ago

Drag-and-drop of text to search bar in another window searches in source window

Categories

(Firefox :: Search, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 4.0b11

People

(Reporter: fehe, Unassigned)

References

Details

(Keywords: regression, Whiteboard: [good first bug])

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041506 Firefox/2.0.0.11
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041506 Firefox/2.0.0.11

When text is dragged from a focused browser windows to the search bar of a non-focused windows, the search is completed in the focused windows instead of the window to which the text was dragged, and the browser becomes focus locked; the only remedy being to restart the browser.

IIRC, this works properly in Fx 2.

Reproducible: Always

Steps to Reproduce:
1. Open two Minefield browser windows
2. Adjust the window positions so that the search bar of the window to be on the bottom is visible.
3. Highlight text, in the window on top, then drag-and-drop the text in the search bar of the non-focused window.

Notice that the search gets completed in the focused windows, instead of the background window.

Notice also that it is now impossible to switch windows.  Focus keeps snapping back to the previously focused window.


Expected Results:  
Fx 2 behavior
Version: unspecified → Trunk
Confirmed with latest trunk on Windows XP.
Regression range:
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2006-12-03+10%3A00&maxdate=2006-12-03+13%3A00
Maybe caused by Bug 351253.
Blocks: 351253
Status: UNCONFIRMED → NEW
Component: General → Search
Ever confirmed: true
Keywords: regression
QA Contact: general → search
Flags: blocking-firefox3?
Mano, Gavin: can you take a look at this?

Focus locks sound bad, but the scenario in which that happens doesn't sound like it's very common. I'm worried about other undiscovered effects, though.
If this regressed in 2006 and we haven't really heard more about it, I don't think this blocks.
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
I think, this problem ist more serious than pointed out in the above written scenario:

There are different ways to walk into this focus lock:

I assume that cookie settings are set to "ask every time".
Way 1:
 - Open any page with a picture and next to it (you need to switch fast) a link pointing to a website that will trigger the dialog to store a cookie
 - Strg+Click on this link to open in in a new tab
 - Now if you start dragging the picture *before* the cookie dialog triggers, focus is lost somewhere between the still active drag and the now triggered dialog (which you cannot answer)
 - firefox continues to work (e.g. downloads continue) but there is no way, to access firefox: You have to kill the process

Way 2:
 - Go to the Library
 - Choose a bookmark that will trigger the cookie dialog on selection
 - Select this bookmark and start dragging it somewhere
 - As soon as the cookie dialog comes up you are no longer able to access firefox (same as above)

Will add a screenshot showing the result of following way 2...
Even though released the drag of the bookmark is still active (left folder pane between "carrier" and "software".

The cookie dialog fired but it is not possible to click any of its buttons.
Attachment #319141 - Attachment description: state after dragging a bookmark before cookie dialog triggers → dragging a bookmark just before cookie dialog triggers
@Thorsten: you're commenting against the wrong bug
Hm, I think it's pretty much the same underlying issue. We just ran into it on different ways.
At least that's my impression and the reason for commenting to your report.
(In reply to comment #7)
> Hm, I think it's pretty much the same underlying issue. We just ran into it on
> different ways.
> At least that's my impression and the reason for commenting to your report.
> 

Focus lock in the original scenario occurs with default Firefox settings (i.e. all cookies accepted, no extensions, and nothing changed whatsoever).  Furthermore, not pop-up dialogs are generated; no hidden dialogs; no feedback required.  As such, I'm still not convinced the issue you describe is the same.
Duplicate of this bug: 440109
(In reply to comment #3)
> If this regressed in 2006 and we haven't really heard more about it, I don't
> think this blocks.
> 

If it's any motivation I just noticed this behavior too. Even though dragging text into another window's search box isn't a regular occurrence, I do do it from time to time. And every time this focus-lock happens.
I forgot to say that I'm running Vista. The affected OS of this bug should probably be expanded to include all Windows OS (instead of limiting it to just Windows XP).
Ok.  I've changed the OS to all, since I see nothing for Windows.  Besides, I highly suspect this affect other platforms as well; but if anyone confirms I'm wrong, I will gladly change it.

A patch for this bug would be greatly appreciated.  Even though I know the bug exists, I keep falling prey to it.  Sometimes it's more convenient to drag text into the visible search bar of another window than to right-click and hunt for the correct search engine in a list of user-added search engines.

Thanks
OS: Windows XP → All
Duplicate of this bug: 452059
I also noticed that the background window (which dropped text to its search box) can't close by close button.
Attached patch proposal patch (obsolete) — Splinter Review
Ensure focus to the window before open search pages.
Since the openUILinkIn function in the utilityOverlay.js uses the top window to find the target window.
In firefox 2, search box handles how to open the search url itself (in new window, in new tab or in current tab etc...). It uses |getBrowser()| function to get the target browser, so it doesn't have this problem.

But in firefox 3, search box utilizes a common utility function |openUILinkIn| which doesn't use (or can't use?) |getBrowser()| but uses the top window from the window mediator as the target window. It causes this bug.
Thanks for looking into this, KENZ! Can we put the focus() call in the onDrop handler to avoid calling it for non-drop searches?
gavin: Yes we can. I put it there just for the clarity.

Today I dived into the deeper part of this bug.
My patch works around the reported problem. But I've not understood why the top window, which shows the search page when this bug happens, goes into invalid, strange state.

I found a XPCOM call with bad arguments. The openUILinkIn() function calls LoadURI function of the top window. (see http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#244 )

This call looks to be intended to call the member function of nsIDocShell, since it passes |allowThirdPartyFixup| as the forth argument. But in fact, nsIDocShell's LoadURI is attributed as [noscript]. So I think it calls nsIWebNavigation's one through GetInterface() of nsGlobalWindow instead.

There is a few problems:
 1. Wrong parameters are passed to the nsIWebNavigation's LoadURI since the caller intended another interface's one.
 2. Even if it called nsIDocShell's LoadURI successfully, the original code is wrong since it passed referrer url as the second argument of the LoadURI.
 3. nsIWebNavigation's LoadURI can't handle firstparty/thirdparty thing. 

For #1, I tweaked parameters some, the "focus lock" problem goes away, but #3 problem prevents me to write a patch to fix this. idea?
I found out I was wrong by investigating more a bit.
The call is not problem. It calls the loadURI() JS (window-)global function in the browser.js. The passed arguments are fine.

The problematic part turned out to be the focusElement() function call after it. But I haven't figured out what is the real cause...
I can't reproduce this anymore (Windows XP).
Duplicate of this bug: 482145
I don't have any focus problems but the search is still performed in the front-most window and not the one where the text is dropped into the search field. Shall we update the summary to reflect the remaining problem?
Hardware: x86 → All
Focus lock still happens for me.  I can't switch to the window having the search bar where I dropped the dragged text.  Focus keeps snapping back to the window from which I dragged the text.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090311 Minefield/3.2a1pre ID:20090311051703
Investigating a bit further, behavior is slightly different now (not as bad but still annoying), which would explain why some of you may not bee seeing the focus issue.  Focus lock is now tab-level instead of window-level.  Specifically:

Drag-dropping text from one window to the search bar of another window causes the search to be completed in the source tab of the source window and focus to be locked _BUT ONLY WHILE THAT SOURCE TAB IS ACTIVE_

If one either closes the source tab or switches to a different tab before attempting to switch to the window that was the drop target, focus is released.  Focus lock will be reasserted if one switches back to the tab that was stealing focus.
I'm a bit irritated by your "focus lock" naming convention. Do you only mean that the source window is still in focus when dropping the text into the search bar of another window?
(In reply to comment #25)
> I'm a bit irritated by your "focus lock" naming convention. Do you only mean
> that the source window is still in focus when dropping the text into the search
> bar of another window?

Why not try the steps out and it will be clear what I mean.  I can't think of any way to explain it any better.
See my comment 22. As said, please answer my question from comment 25.
I read you comment 22 and I explained my observation in comment 24.  When I state in comment 24 that focus is locked, I mean that attempting to switch focus to the other window (where the text was dropped) springs focus back to the active window (from where the text was dragged).

This is on XP.
Duplicate of this bug: 489875
With the landing of Bug 178324, focus is no longer locked, but the search still completes in the source window instead of the target window where the text was dropped.
Updating summary so we know what has to be done on this bug.
Summary: Focus lock on cross-window drag-and-drop of text to search bar → Drag-and-drop of text to search bar in another window searches in source window
Last year, I gave up fixing this for the problem which block the window.

Now the great focus refactoring fixed that! Then I revised my old patch with gavin's comment #17.

There is one tricky part, I introduced a new mTargetWindow field in searchbarDNDObserver object, since the onDrop handler seems to have another JS context, its (global) window.focus() doesn't focus the window of this search binding belongs to.
Attachment #339263 - Attachment is obsolete: true
Kenji, please ask for review from gavin or rflint if you think the patch is ready for.
Comment on attachment 382720 [details] [diff] [review]
Revised patch after about one year...

Thanks henrik. Requesting review...
Attachment #382720 - Flags: review?(gavin.sharp)
Comment on attachment 382720 [details] [diff] [review]
Revised patch after about one year...

I would rather add an optional window argument to that method, or not use it at all. Using openUILink in browser.js makes sense only within menuitems-commands, because these may be used in "app-context" (mac application menu).

In other words, the drop target should be used rather than the "focused" window.
Attachment #382720 - Flags: review?(gavin.sharp) → review-
Comment on attachment 382720 [details] [diff] [review]
Revised patch after about one year...

No need for mTargetWindow - you can just call window.focus() directly in onDrop. I'd change the comment to something like:

Make sure this window is focused, since onTextEntered uses openUILinkIn, which in turn uses the topmost window for the load.
Sorry, I mid-aired with Mano's comment. We could refactor search.xml to avoid using openUILinkIn, but I'm not sure we need to block this patch on that. Arguably it's better to have in-UI link opening go through the same codepaths anyways (though we're not super consistent about that, so maybe that's wishful thinking).
Per comment #35. Tested on Windows Vista and Mac OS X.

Rewriting not to use openUILinkIn() at all is too heavy to me, so if it is necessary, someone please take this.
Attachment #382720 - Attachment is obsolete: true
Attachment #382774 - Flags: review?(gavin.sharp)
I guess that works too, but I'm a bit hesitant to extend openUILinkIn's signature for this case specifically.

I also liked the other fix because it actually raised the dropped-on window, which seems like the behavior we want - why would you drag to a background searchbar and expect it to stay in the background?
Do we do so when you drop a url on the browser? If so, how doesn't it happen for search?
I guess we don't do it for links dragged onto the browser...

Perhaps we should just have openUILinkIn use the current window if (document.documentElement.getAttribute("windowtype") == "navigator:browser") then?
The simplest fix for this would be to avoid openUILinkIn in search.xml and just do what it does for "current" (i.e. loadURI) respectively for "tab" (i.e. gBrowser.loadOneTab).
Sure, I guess we should just do that (despite what I said in comment 37).
Duplicate of this bug: 515434
Duplicate of this bug: 516252
Duplicate of this bug: 527621
Comment on attachment 382774 [details] [diff] [review]
Adding optional parameter to openUILinkIn()

Let's go with Dao's suggested fix in comment 42.
Attachment #382774 - Flags: review?(gavin.sharp) → review-
Whiteboard: [good first bug]
Target Milestone: --- → Firefox 3.6
Duplicate of this bug: 620677
Bug 626148 fixed this.
Status: NEW → RESOLVED
Closed: 9 years ago
Depends on: 626148
Resolution: --- → FIXED
Target Milestone: Firefox 3.6 → Firefox 4.0b11
Verified with Mozilla/5.0 (Windows NT 5.1; rv:2.2a1pre) Gecko/20110410 Firefox/4.2a1pre
Status: RESOLVED → VERIFIED
As long as we do not have automated tests it would be good to have at least a Litmus test for this case.
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.