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

Status: VERIFIED FIXED

(Firefox :: Search, defect)

Description

[IU]

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

Comment 1

[Ria Klaassen (not reading all bugmail)]

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.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

If this regressed in 2006 and we haven't really heard more about it, I don't think this blocks.

Comment 4

[Thorsten]

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...

Comment 5

[Thorsten]

Attachment: dragging a bookmark just before cookie dialog triggers

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.

Comment 6

[IU]

@Thorsten: you're commenting against the wrong bug

Comment 7

[Thorsten]

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.

Comment 8

[IU]

(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.

Comment 10

[Vorenus]

(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.

Comment 11

[Vorenus]

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).

Comment 12

[IU]

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

Comment 14

[KENZ (Kenji Irie)]

I also noticed that the background window (which dropped text to its search box) can't close by close button.

Comment 15

[KENZ (Kenji Irie)]

Attachment: proposal patch

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.

Comment 16

[KENZ (Kenji Irie)]

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.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Thanks for looking into this, KENZ! Can we put the focus() call in the onDrop handler to avoid calling it for non-drop searches?

Comment 18

[KENZ (Kenji Irie)]

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?

Comment 19

[KENZ (Kenji Irie)]

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...

Comment 20

[Ria Klaassen (not reading all bugmail)]

I can't reproduce this anymore (Windows XP).

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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?

Comment 23

[IU]

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

Comment 24

[IU]

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.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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?

Comment 26

[IU]

(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.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

See my comment 22. As said, please answer my question from comment 25.

Comment 28

[IU]

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.

Comment 30

[IU]

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.

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Updating summary so we know what has to be done on this bug.

Comment 32

[KENZ (Kenji Irie)]

Attachment: Revised patch after about one year...

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.

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Kenji, please ask for review from gavin or rflint if you think the patch is ready for.

Comment 34

[KENZ (Kenji Irie)]

Comment on attachment 382720 [details] [diff] [review]
Revised patch after about one year...

Thanks henrik. Requesting review...

Comment 35

[Asaf Romano (gone)]

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.

Comment 36

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 37

[:Gavin Sharp [email: [e-mail]]]

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).

Comment 38

[KENZ (Kenji Irie)]

Attachment: Adding optional parameter to openUILinkIn()

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.

Comment 39

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 40

[Asaf Romano (gone)]

Do we do so when you drop a url on the browser? If so, how doesn't it happen for search?

Comment 41

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 42

[Dão Gottwald [:dao]]

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).

Comment 43

[:Gavin Sharp [email: [e-mail]]]

Sure, I guess we should just do that (despite what I said in comment 37).

Comment 47

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 382774 [details] [diff] [review]
Adding optional parameter to openUILinkIn()

Let's go with Dao's suggested fix in comment 42.

Comment 49

[Dão Gottwald [:dao]]

Bug 626148 fixed this.

Comment 50

[George Carstoiu]

Verified with Mozilla/5.0 (Windows NT 5.1; rv:2.2a1pre) Gecko/20110410 Firefox/4.2a1pre

Comment 51

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As long as we do not have automated tests it would be good to have at least a Litmus test for this case.