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

RESOLVED FIXED

Status

()

Firefox
Address Bar
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: dietrich, Assigned: Neil Deakin)

Tracking

(Depends on: 1 bug)

Trunk
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [hardblocker][fx4-fixed-bugday])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

7 years ago
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!
(Reporter)

Updated

7 years ago
blocking2.0: --- → betaN+
(Assignee)

Comment 1

7 years ago
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.
Changing to all as this happens on Windows and Mac.
OS: Linux → All
Hardware: x86 → All
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.
Is there a workaround which lets you get focus back to the document?
(Reporter)

Comment 5

7 years ago
(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?
(Assignee)

Comment 6

7 years ago
(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.
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...
Depends on: 604289
Whiteboard: [hardblocker]
Beltzner, if this is a blocker, bug 604289 should be so too...
(Reporter)

Comment 9

7 years ago
(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.
(Reporter)

Comment 10

7 years ago
(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?
(Assignee)

Comment 11

7 years ago
Note that Safari has this bug as well.
(Assignee)

Comment 12

7 years ago
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.
(Assignee)

Comment 13

7 years ago
Comment on attachment 503277 [details] [diff] [review]
patch

Seems to pass all of the tests.
Attachment #503277 - Flags: review?(Olli.Pettay)
(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.
(Assignee)

Comment 15

7 years ago
(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.
Created attachment 503400 [details] [diff] [review]
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.
(Reporter)

Updated

7 years ago
Assignee: nobody → enndeakin
Created attachment 503458 [details] [diff] [review]
Another approach v2.0
Attachment #503400 - Attachment is obsolete: true
Attachment #503458 - Flags: review?(enndeakin)
(Assignee)

Comment 18

7 years ago
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.
(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 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.
(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.
Attachment #503458 - Attachment is obsolete: true
Attachment #503458 - Flags: review?(enndeakin)
I'd still like the check for chrome, since calling
some_other_element.focus(); event.preventDefault();
should work even in mousedown.
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.
Whiteboard: [hardblocker] → [hardblocker][has patch][needs review smaug]
(Assignee)

Updated

7 years ago
Attachment #503277 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

7 years ago
Whiteboard: [hardblocker][has patch][needs review smaug] → [hardblocker]
(Assignee)

Comment 24

7 years ago
Created attachment 505496 [details] [diff] [review]
patch, version 2
Attachment #503277 - Attachment is obsolete: true
(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 on attachment 505496 [details] [diff] [review]
patch, version 2

Were you going to ask review?
(Assignee)

Comment 27

7 years ago
Created attachment 505777 [details] [diff] [review]
patch, version 3 which passes tests
Attachment #505496 - Attachment is obsolete: true
Attachment #505777 - Flags: review?(Olli.Pettay)
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
Attachment #505777 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 29

7 years ago
(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.
(Assignee)

Comment 30

7 years ago
I checked it in with the null check:

http://hg.mozilla.org/mozilla-central/rev/00caaac7044f
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Still reproducible with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre
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.
Whiteboard: [hardblocker] → [hardblocker][fx4-fixed-bugday]
You need to log in before you can comment on or make changes to this bug.