FIREFOX START PAGE: Focus pulled to start page Google box

Do you mean the caret? That's what Ctrl-L does. This seems invalid.

I think you misunderstand. I am complaining that when the caret is in the
location field (CTRL-L, etc) and google.com loads up, its Javascript steals the
caret away from the location field into the page. The Javascript code shouldn't
be allowed to steal the caret if it's in the location field.

->bryner for triage, cc aaronl.

Very similar is bug 124750.
Seeing on Linux 2002030414 m0.9.9branch build

urlbar

This doesn't seem like an URL bar problem so much as a focus problem. cc bryner.
 Dup?

*** Bug 143973 has been marked as a duplicate of this bug. ***

URL From Bug 143973
http://www.episode-x.com/episode2/index.php#685

*** Bug 140524 has been marked as a duplicate of this bug. ***

*** Bug 230648 has been marked as a duplicate of this bug. ***

This bug is almost 2 years old. Is anyone following up on it?

The search bar in Firefox, and using multiple tabs as well as multiple instances
of the browser are also effected by this problem.

*** Bug 271565 has been marked as a duplicate of this bug. ***

Google.com loads pretty quickly for testing this. Slower loading testcase coming up.

Created an attachment (id=166947)
slower loading testcase

Just start slowly typing in the URL bar as the page loads, and watch the focus
get stolen by the Google form.

Perhaps a solution should be when a page sets focus, to make the next tabstop
the form the page sets focus to.

+1

I like Brian's idea of redirecting focus requests to he next tabstop.

As much as I hate focus stealing, this is definitely not a "major loss of function".

*** Bug 229007 has been marked as a duplicate of this bug. ***

*** Bug 274747 has been marked as a duplicate of this bug. ***

*** Bug 302671 has been marked as a duplicate of this bug. ***

I posted bug 302671 marked duplicate for this bug and pointed out that forms
should not be allowed to steal focus while typing. There are some times where
you want the forms to steal focus. If I were to type google.com into the url
box, that would be the behavior I would expect.

I disagree. When I first filed this issue 3 years ago my original complaint was
that if Google.com finishes loading while I am in the middle of entering a new
URL in the location bar, it should not steal the focus away.

Specifically, if my browser homepage was google and I started typing in the
location bar immediately after loading the browser and before Google requested
keyboard focus, then the browser should not allow it to steal the focus.

Simple rule: if the user began doing something after the page began loading but
before it finished loading, then the user is deemed to be "busy" and the website
should not be allowed to steal focus. This is exactly what happens if you open a
new window in Windows and shift focus away before it actually pops up. When it
actually pops up, its focus request will be rejected and instead you'll get a
flashing indicator in your taskbar. This is exactly the kind of behavior I'm
voting for.

I think you misunderstood me. When I'm finished typing google.com in the URL
bar and hit enter, I expect google to steal the focus. otherwise the caret
should stay in the location bar, but a quick test of firefox shows that focus
is transferred to the page regardless. IE has the same behavior. Rats!

*** Bug 317643 has been marked as a duplicate of this bug. ***

A simple solution to this would be as follows:

Detect current typing, and do not change focus until typing is complete.
(For example, prior to changing focus, if there were more than 2 keypresses in
the last second, abort [or delay] the focus change; otherwise, change focus)

NOTE:
This happens in other occurances of data entry, potentially (worst-case) I
could begin typing in a password somewhere, and would finish typing this in, in
a clear-text box, and accidentally submit this somewhere.

I think Epiphany <http://www.gnome.org/projects/epiphany/> has solved this problem. Maybe we can see how they did it.

In reply to comment 23: hitting enter is not the same as continuously typing. When you hit enter you are telling the browser "I am done with what I was typing, so take me there", and you are relinquishing your focus. When you are typing, you are busy.

I think that bug 359549 is a duplicate of this one. It happens to me a lot. my homepage is mail.yahoo.com, which is heavy on javascript and loads slowly. While it loads, I try to type something into the search bar, with the intention of opening the search in another tab (using alt-enter). however, yahoo keeps stealing the focus from the search bar *repeatedly* while it is loading. I think this behavior is completely wrong.

Regarding comment 25: I don't think that detecting typing is the right approach either. If I interactively placed the focus on the search bar, it should just stay there, no matter what. The search bar is used for many references other than google - perhaps I am thinking about what to type in for a couple of seconds before actually typing it in. Same goes for password prompts and other firefox components.

My rule of thumb is: if the user placed the focus on any component which is a browser component, and not a component inside the web page, then the web page shouldn't be able to steal it.

In my mind, anything that is not placed inside a tab (e.g., search bar, url bar, password prompts, and any input that originates from a firefox extension) is a browser component, and focus should not be stolen from it.

+1

I agree with #27

*** Bug 367440 has been marked as a duplicate of this bug. ***

*** Bug 370094 has been marked as a duplicate of this bug. ***

Created an attachment (id=254761)
Testcase

This is a focus problem, not related to Urlbar. Changing component to forms.

Please note that unlike the previous testcase, the new one (by pdp) works reliably, is very simple, and has been published on bugtraq and FD, and may be a vector of other, more severe bugs (like 370092), i.e. may be a security problem. I hope this raises the priority.

*** Bug 377011 has been marked as a duplicate of this bug. ***

UI elements such as location should have a higher focus priority than page elements.

*** Bug 359549 has been marked as a duplicate of this bug. ***

*** Bug 417915 has been marked as a duplicate of this bug. ***

The reporter of bug 417915 points out that this is especially annoying now that Firefox's default homepage is one that has a textbox.focus() call. If you start Firefox and hit Cmd+L right away, http://www.google.com/firefox will often steal focus from the address bar.

*** Bug 164339 has been marked as a duplicate of this bug. ***

ok, thanks Masayuki. I'm seeing the problem in today's nightly. That saved me a build.

Created an attachment (id=394621)
Patch v4.2

Adding some tests to the new test.

However, this test makes an unexpected failure of non-related test only on Linux. I'm looking for the reason.

# If this tests are not run, the unexpected failure doesn't happen.

> Running chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js...
> TEST-INFO | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Console message: [JavaScript Error: "[Exception... "'JavaScript component does not have a method named: "observe"' when calling method: [nsIObserver::observe]" nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)" location: "JS frame :: file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js :: notifyAction :: line 664" data: no]" {file: "file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js" line: 664}]
> TEST-INFO | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Console message: [JavaScript Error: "[Exception... "'JavaScript component does not have a method named: "observe"' when calling method: [nsIObserver::observe]" nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)" location: "JS frame :: file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js :: notifyAction :: line 664" data: no]" {file: "file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js" line: 664}]
> TEST-INFO | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Console message: [JavaScript Error: "[Exception... "'JavaScript component does not have a method named: "observe"' when calling method: [nsIObserver::observe]" nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)" location: "JS frame :: file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js :: notifyAction :: line 664" data: no]" {file: "file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js" line: 664}]
> TEST-INFO | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Console message: [JavaScript Error: "[Exception... "'JavaScript component does not have a method named: "observe"' when calling method: [nsIObserver::observe]" nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)" location: "JS frame :: file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js :: notifyAction :: line 664" data: no]" {file: "file:///builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/components/nsSearchService.js" line: 664}]
> TEST-INFO | chrome://mochikit/content/browser/browser/base/content/test/browser_overflowScroll.js | Console message: [JavaScript Error: "[Exception... "'JavaScript component does not have a method ...

Created an attachment (id=395023)
Patch v4.3

ok, this should be good.

Boris still thinks nsContentUtils::CanCallerAccess isn't the cause of the tp regression. But the experimentation didn't show so. The method can be improved the performance, but I'm not expert of it. So, I have no idea to fix this bug without I reduce the calling it.

This patch is simpler than the previous reviewed patch. But this still add a new flag to nsIFocusManager. See the test case, the web developers can cheat the previous patch by using the createEvent. So, the flag is really needed.

(In reply to comment #100)
> The lines of "NOISE: nsContentUtils::CanCallerAccess Called" are the log of
> nsContentUtils::CanCallerAccess called (I put printf() to the patch).
>
> It was called 32 times per a cycle.

Is that calls to nsContentUtils::CanCallerAccess or calls from the focus manager to it? When I run the normal performance test (not sure about this specific one) I get only one call to ShiftFocusInner.

What happens if you just replace the call to nsContentUtils::CanCallerAccess with code that just always matches/never matches?

I suspect more likely that removing a focus call is causing some other behaviour to occur, or changing the timing of a flush or somesuch. The pageload performance tests don't actually test focus time, they only test up until the page is loaded, and focus can occur after that point. So using the pageload tests for testing focus performance regressions is generally going to be inaccurate anyway.

I don't really like this newer patch as it adds quite a bit of extra complexity and requires callers to think about what to set the new flag to.

That said, I noticed a problem with the earlier patch anyway with the following test:

<textarea id="t">...</textarea><script>document.getElementById('t').focus()</script>

Focus the urlbar, reload the page, lower the window and then raise it again. The urlbar should be focused yet the textarea is now focused, even though the urlbar was focused before the window was lowered. This is caused because the setting of 'canStealFocus' should actually just be clearing 'allowFrameSwitch', so that the later conditions which check this don't match.

Personally I would rather just fix this problem, and check in the earlier patch.

(In reply to comment #114)
> (In reply to comment #100)
> > The lines of "NOISE: nsContentUtils::CanCallerAccess Called" are the log of
> > nsContentUtils::CanCallerAccess called (I put printf() to the patch).
> >
> > It was called 32 times per a cycle.
>
> Is that calls to nsContentUtils::CanCallerAccess or calls from the focus
> manager to it? When I run the normal performance test (not sure about this
> specific one) I get only one call to ShiftFocusInner.

That means latter (from the first landed patch).

> What happens if you just replace the call to nsContentUtils::CanCallerAccess
> with code that just always matches/never matches?

Did you mean the |foo = nsContentUtils::CanCallerAccess| to |foo = PR_TRUE|? If so, at that time, the perf regression didn't happen.

> I suspect more likely that removing a focus call is causing some other
> behaviour to occur, or changing the timing of a flush or somesuch. The pageload
> performance tests don't actually test focus time, they only test up until the
> page is loaded, and focus can occur after that point. So using the pageload
> tests for testing focus performance regressions is generally going to be
> inaccurate anyway.

If so, the new patch without the optimization shouldn't be faster than the old patch, I'll check it.

And I'll check the bug of you pointed out.

I posted the old patch and following patterns:
* nsContentUtils::CanCallerAccess is replaced to PR_TRUE
* nsContentUtils::CanCallerAccess is replaced to PR_FALSE
* the if block was commented out

But the their tp*4* scores are almost same value. So, looks like the tp4 don't include the nsIDOMElement::focus cost in onload event to the score.

Boris, do you know the difference between tp3 and tp4?

It's a different pageset. I think the harness is the same.

*** Bug 513390 has been marked as a duplicate of this bug. ***

Created an attachment (id=397823)
Patch v5.0

I hope Enn likes this patch. In the AdjustWindowFocus, we need to check the permission too. And I removes to check the key/mouse events flag for the simple code.

I'll create some additional tests.

I don't know if this is a new bug or not, but this is a related problematic issue:

If you are using a form and you are editing a field, the page JS can move the focus to another spot in the form. Annoying, right?

But this is also a security problem because of login pages. Here entry 1 is often username name and entry 2 is often password. In some cases I can finish the user name, start in on the password and the JS moves me back to the username.

Now, imagine doing that when using a public computer lab or worse a projector. This issue makes me wonder if the importance of this bug should not be changed.

login page

Paul Bailey, this is a good point, but not the same as this bug. Can you file a new one, please?

The patch will forbid to steal focus from another document.

Ben Bucksch, good idea. I added bug 532097, with some more thought into what should be fixed.

Created an attachment (id=415556)
Patch v5.0.1

o.k., would you review this again?

1. We should test only when the focus moves between different documents.
2. Don't refer the mouse flag and the keyboard flag, the web developers can cheat the check by DOM event creating.
3. AdjustWindowFocus() needs to check too only when it's called by |SetFocusInner|.

(From update of attachment 415556)
>+ // XXX Clear the dragging state here, if not so, it breaks next test result.
>+ EventUtils.synthesizeKey("VK_ESCAPE", {}, window);

This won't do anything as the escape key during a drag is handled by the system.

>+
>+ callback = doTest2;
>+ loadTestPage();
>+
>+ // Set focus to chrome element before onload events of the loading contents.
>+ BrowserSearch.searchBar.focus();

I think it would be clearer to call this last line during loadTestPage before loading the url.

> #include "nsTreeWalker.h"
> #include "nsIDOMNodeFilter.h"
>+#include "nsIScriptSecurityManager.h"

I don't think this is needed.

(In reply to comment #125)
> (From update of attachment 415556 [details])
> >+ // XXX Clear the dragging state here, if not so, it breaks next test result.
> >+ EventUtils.synthesizeKey("VK_ESCAPE", {}, window);
>
> This won't do anything as the escape key during a drag is handled by the
> system.

Oh, really? But this change fixed the orange which is happened by my new test file... I'll check it again tomorrow.

> >+
> >+ callback = doTest2;
> >+ loadTestPage();
> >+
> >+ // Set focus to chrome element before onload events of the loading contents.
> >+ BrowserSearch.searchBar.focus();
>
> I think it would be clearer to call this last line during loadTestPage before
> loading the url.

No. Probably, Fx sets focus to contents when it starts to load a page on active tab. Therefore loadTestPage() sets focus to the active tab manually. This test tests a case which the focus is moved to chrome by user during the page loading. So, your suggestion changes the content of the testing.

(In reply to comment #126)

> > This won't do anything as the escape key during a drag is handled by the
> > system.
>
> Oh, really? But this change fixed the orange which is happened by my new test
> file... I'll check it again tomorrow.

Possibly some side effect then. I don't mind if this is put in, but it would be good to know what is happening here.

> No. Probably, Fx sets focus to contents when it starts to load a page on active
> tab. Therefore loadTestPage() sets focus to the active tab manually. This test
> tests a case which the focus is moved to chrome by user during the page
> loading. So, your suggestion changes the content of the testing.

OK, although I'm worried that the timing of loading and focusing might cause intermittent failures.

(In reply to comment #127)
> OK, although I'm worried that the timing of loading and focusing might cause
> intermittent failures.

I think that is no problem. The onload event which fires doTest2() is going to be called after the doTest1() is finished. Can onload() event intrude into the stack?

Created an attachment (id=416682)
Patch v5.1

I see what happens on browser_drag.js.

When the tests finished, the identity panel is opened by a click event which is generated by the drag emulation. So, we need to close it by the esc key event.

A better fix for that is to prevent the panel from opening by cancelling either the click or popupshowing events.

I think it needs more code than my approach, and it depends on the Fx's XUL structure, so, it could be broken by some UI changes...

Why is this in browser/base/content/test/ anyway, rather than dom/tests/browser/ for instance?

Created an attachment (id=417025)
Patch v5.2

O.K. The new test file is moved to dom/tests/browser.

Created an attachment (id=417026)
Patch v5.2

Oops...

http://hg.mozilla.org/mozilla-central/rev/a8f46161c891

Hey, having just fixed this, it might make sense to fix Bug 532097 while you know this portion of the code well.

(From update of attachment 417026)
>+ // If the previous (or more) test finished with cleaning up the tabs,
>+ // there may be some pending animations. That can cause a failure of
>+ // this tests, so, we should test this in another stack.
>+ setTimeout(doTest, 0);

Tabs are closed synchronously, so I don't think I understand this. What kind of animations are you referring to?

(In reply to comment #137)
> (From update of attachment 417026 [details])
> >+ // If the previous (or more) test finished with cleaning up the tabs,
> >+ // there may be some pending animations. That can cause a failure of
> >+ // this tests, so, we should test this in another stack.
> >+ setTimeout(doTest, 0);
>
> Tabs are closed synchronously, so I don't think I understand this. What kind of
> animations are you referring to?

I'm not sure, on Linux, the test failed without the code in the older patch. I have no idea except that the animation is the problem.

Which animation?

So, I guessed the tab closing isn't synchronously.

But it is.

*** Bug 561131 has been marked as a duplicate of this bug. ***

*** Bug 569254 has been marked as a duplicate of this bug. ***

*** Bug 565544 has been marked as a duplicate of this bug. ***

Web pages still can steal focus, see bug 604289. I'll fix it ASAP.

*** Bug 623066 has been marked as a duplicate of this bug. ***

*** Bug 624680 has been marked as a duplicate of this bug. ***

*** Bug 626156 has been marked as a duplicate of this bug. ***

*** Bug 638047 has been marked as a duplicate of this bug. ***