618907 - location bar steals focus (or shares it?!) with a Google document

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[Dietrich Ayala (:dietrich)]

1. open a new document at docs.google.com

2. start typing in the document

3. click in the location bar, so that results show up, but don't select anything

4. click in the document and start typing again

Expected result: typing shows up the document, and the location bar doesn't do anything at all.

Actual result: the typing shows up in the location bar!

Comment 1

[Neil Deakin]

Looks like a "regression" from one of the changes to prevent focus stealing.

At this point http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#1137 , sendFocusEvent is false and mMouseDownEventHandlingDocument is null.

I didn't investigate too far, but some simple debugging would suggest that they are canceling the default mousedown behaviour and calling focus() within a setTimeout. Allowing focus in this case would mean removing the focus stealing preventions, so I'm not sure if there is a suitable fix here.

Comment 2

[Marcia Knous [:marcia]]

Changing to all as this happens on Windows and Mac.

Comment 3

[Johnathan Nightingale [:johnath]]

Notes from the Grand Retriage: neil says masayuki is the best person to continue investigating, beltzner suggests cc'ng blizzard to ask google why they gotta be up in our business.

Comment 4

[Justin Lebar (not reading bugmail)]

Is there a workaround which lets you get focus back to the document?

Comment 5

[Dietrich Ayala (:dietrich)]

(In reply to comment #1)
> I didn't investigate too far, but some simple debugging would suggest that they
> are canceling the default mousedown behaviour and calling focus() within a
> setTimeout. Allowing focus in this case would mean removing the focus stealing
> preventions, so I'm not sure if there is a suitable fix here.

I don't understand. I click in web content. And start typing. Why should anything I type show up in the last-focused chrome UI? I've explicitly given focus to the content.

Can you cc the people who worked on the anti-focus-stealing changes?

Comment 6

[Neil Deakin]

(In reply to comment #5)
> I don't understand. I click in web content.

The page is canceling the default behaviour of the mousedown event. The default behaviour of the mousedown event is to focus the page content. By canceling the default behaviour, the content does not get focused.

Note that the setting of the :active state is also prevented.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, it seems that we should store the mousedown document until next user input event such as another mousedown event, key event...

I'm still working on bug 604289. The patch is almost ready locally, but I need to add some more complex testcases...

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Beltzner, if this is a blocker, bug 604289 should be so too...

Comment 9

[Dietrich Ayala (:dietrich)]

(In reply to comment #6)
> (In reply to comment #5)
> > I don't understand. I click in web content.
> 
> The page is canceling the default behaviour of the mousedown event. The default
> behaviour of the mousedown event is to focus the page content. By canceling the
> default behaviour, the content does not get focused.
> 
> Note that the setting of the :active state is also prevented.

Could we have our own handler that detects if the coordinates of the click were in content, and unfocus whatever had focus in chrome?

that would at least not cause the focus to appear to be in two places, and would clarify that the web page is broken, not Firefox.

Comment 10

[Dietrich Ayala (:dietrich)]

(In reply to comment #7)
> I'm still working on bug 604289. The patch is almost ready locally, but I need
> to add some more complex testcases...

Does that fix this bug?

Comment 11

[Neil Deakin]

Note that Safari has this bug as well.

Comment 12

[Neil Deakin]

Attachment: patch

I suppose a simple fix would be to focus the frame that was clicked even when the event was canceled.

Comment 13

[Neil Deakin]

Comment on attachment 503277 [details] [diff] [review]
patch

Seems to pass all of the tests.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #12)
> Created attachment 503277 [details] [diff] [review]
> patch
> 
> I suppose a simple fix would be to focus the frame that was clicked even when
> the event was canceled.

Hmm, isn't this a big specification change?

Seems that the changed behavior isn't same as DOM3 spec. See bottom of the click event description.
http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#event-type-click

(In reply to comment #10)
> (In reply to comment #7)
> > I'm still working on bug 604289. The patch is almost ready locally, but I need
> > to add some more complex testcases...
> 
> Does that fix this bug?

Unfortunately, no. This needs additional work.

Comment 15

[Neil Deakin]

(In reply to comment #14)
> Hmm, isn't this a big specification change?
> 
> Seems that the changed behavior isn't same as DOM3 spec. See bottom of the
> click event description.
> http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#event-type-click

I'm not changing the behaviour of the click event, I'm changing the behaviour of the mousedown event. The specification doesn't and shouldn't mention focusing behaviour of click since that is platform specific.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Another approach

(In reply to comment #15)
> (In reply to comment #14)
> > Hmm, isn't this a big specification change?
> > 
> > Seems that the changed behavior isn't same as DOM3 spec. See bottom of the
> > click event description.
> > http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#event-type-click
> 
> I'm not changing the behaviour of the click event, I'm changing the behaviour
> of the mousedown event. The specification doesn't and shouldn't mention
> focusing behaviour of click since that is platform specific.

The mousedown event behavior change equals changing the click event behavior too.
I think that this patch is safer. But this patch breaks the patch for bug 604289, but it's not matter because I need more days for the bug.
# I'll post this patch to tryserver.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Another approach v2.0

Comment 18

[Neil Deakin]

Comment on attachment 503458 [details] [diff] [review]
Another approach v2.0

This doesn't seem to reset the mousedown document. Wouldn't this be a problem if some unrelated code happens to change the focus or active window in between the mouse click and when a focus attempt is made?

I think that using setTimeout(focusSomething, 0) is something that should be blocked.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #18)
> This doesn't seem to reset the mousedown document. Wouldn't this be a problem
> if some unrelated code happens to change the focus or active window in between
> the mouse click and when a focus attempt is made?

The mousedown document is cleared when:

* A trusted event which is handled on focused content is dispatched (nsAutoHandlingUserInputStatePusher::nsAutoHandlingUserInputStatePusher()).
* Focus is moved (nsFocusManager::SetFocusInternal()).

I think that this is enough. If focus is moved on mouse event handler, the mousedown document will be cleared.

> I think that using setTimeout(focusSomething, 0) is something that should be
> blocked.

I'm not sure what you meant.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 503277 [details] [diff] [review]
patch

># HG changeset patch
># Parent 6d621e3fc42e74c055ef2ac2b4dff7979a732016
>
>diff --git a/content/events/src/nsEventStateManager.cpp b/content/events/src/nsEventStateManager.cpp
>--- a/content/events/src/nsEventStateManager.cpp
>+++ b/content/events/src/nsEventStateManager.cpp
>@@ -2956,16 +2956,24 @@
>               activeContent = par;
>           }
>         }
>       }
>       else {
>         // if we're here, the event handler returned false, so stop
>         // any of our own processing of a drag. Workaround for bug 43258.
>         StopTrackingDragGesture();
>+
>+        // Even when the event was cancelled, ensure that the window that was
>+        // clicked is focused.
>+        EnsureDocument(mPresContext);
>+        nsIFocusManager* fm = nsFocusManager::GetFocusManager();
>+        if (mDocument && fm) {
>+          fm->SetFocusedWindow(mDocument->GetWindow());
>+        }
>       }
>       SetActiveManager(this, activeContent);
>     }
I think it would less risky to change focused window only if the current focused window is chrome and
new one is content. That way we shouldn't regress focus() calls in content mousedown listeners.

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #20)
> I think it would less risky to change focused window only if the current
> focused window is chrome and
> new one is content. That way we shouldn't regress focus() calls in content
> mousedown listeners.

I checked other browser behavior on Windows.

IE9: moves focus to the mousedown content even if the event is prevented (before mouseup, focus is moved)
Chrome: doesn't move focus to the mousedown content, but the window becomes active and active content gets focus
Opera: doesn't move focus like us.

So, the Enn's approach is same as google chrome. Therefore, it has less risk.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

I'd still like the check for chrome, since calling
some_other_element.focus(); event.preventDefault();
should work even in mousedown.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Hmm, what would happen if we focused the window in ESM::PreHandleEvent?
That might be the right thing to do, but riskier than Enn's approach + check for chrome. So better to try PreHandleEvent only after FF4.

Comment 24

[Neil Deakin]

Attachment: patch, version 2

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #23)
> Hmm, what would happen if we focused the window in ESM::PreHandleEvent?

I have no idea of negative effect. However, I think that it can remove the current mousedown hack.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 505496 [details] [diff] [review]
patch, version 2

Were you going to ask review?

Comment 27

[Neil Deakin]

Attachment: patch, version 3 which passes tests

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 505777 [details] [diff] [review]
patch, version 3 which passes tests

># HG changeset patch
># Parent 01662232f7eb268003876eec04a11edfc620121c
>
>diff --git a/content/events/src/nsEventStateManager.cpp b/content/events/src/nsEventStateManager.cpp
>--- a/content/events/src/nsEventStateManager.cpp
>+++ b/content/events/src/nsEventStateManager.cpp
>@@ -2956,16 +2956,34 @@
>               activeContent = par;
>           }
>         }
>       }
>       else {
>         // if we're here, the event handler returned false, so stop
>         // any of our own processing of a drag. Workaround for bug 43258.
>         StopTrackingDragGesture();
>+
>+        // When the event was cancelled, there is currently a chrome document
>+        // focused and a mousedown just occurred on a content document, ensure
>+        // that the window that was clicked is focused.
>+        EnsureDocument(mPresContext);
>+        nsIFocusManager* fm = nsFocusManager::GetFocusManager();
>+        if (mDocument && fm) {
>+          nsCOMPtr<nsIDOMWindow> currentWindow;
>+          fm->GetFocusedWindow(getter_AddRefs(currentWindow));
>+          if (currentWindow != mDocument->GetWindow() &&
>+              !nsContentUtils::IsChromeDoc(mDocument)) {
>+            nsCOMPtr<nsPIDOMWindow> win = do_QueryInterface(currentWindow);
>+            nsCOMPtr<nsIDocument> currentDoc = do_QueryInterface(win->GetExtantDocument());
If currentWindow is null, you crash here. I think we should focus the window also if currentWindow is null.

Something like 
            nsCOMPtr<nsPIDOMWindow> win = do_QueryInterface(currentWindow);
            nsCOMPtr<nsIDocument> currentDoc;
            if (win) {
              currentDoc = do_QueryInterface(win->GetExtantDocument());
            }
            if (!currentDoc || nsContentUtils::IsChromeDoc(currentDoc)) {
              fm->SetFocusedWindow(mDocument->GetWindow());
            }
...

With the null check,r=me

Comment 29

[Neil Deakin]

(In reply to comment #28)
> I think we should focus the window also if currentWindow is null.

That would mean that the window had been lowered, and I think we should not focus it.

Comment 30

[Neil Deakin]

I checked it in with the null check:

http://hg.mozilla.org/mozilla-central/rev/00caaac7044f

Comment 31

[Aleksei Perepelkin [:chaos]]

Still reproducible with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre

Comment 32

[u279076]

I'm not able to reproduce this on any platform. But perhaps I'm doing the steps wrong.  Aleksej, can you confirm the steps you are taking to reproduce this bug?  Thanks.