Closed Bug 591890 Opened 9 years ago Closed 9 years ago

Focus element from mouseDown event doesn't work when the focus is set to an chrome element

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: julian.viereck, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

Attached file Regression test page
Imagine you've got a div element with a mouseDown event listener. Once the event is fired, the focus is set to an input element, somewhat like this:

      div.addEventListener("mousedown", function(aEvt) {
        div.innerHTML += "(focus)";
        textarea.focus();
        aEvt.preventDefault();
      }, false);

Within the current nightlies, this doesn't work if the focus is set to an element/input node of the chrome space and the user clicks on the div. The "(focus)" is added to the div, but the textarea is not focused. The focus stays at the chrome element. If you replace the "mousedown" event by a "click" event it works.

The regression window is: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9af2a428dcb1&tochange=d379a17cbf8f (from 2009121103 to 2009121203).

Commit http://hg.mozilla.org/mozilla-central/rev/a8f46161c891 (bug 125282) might cause this regression.


How to reproduce:

1. Open the attached test.html in Minefield
2. Focus the URL bar
3. Click on the div

Expected behavior:
- The textarea is focused

Actually behavior:
- The textarea is not focused
Adding Masayuki to CC as he's the author of patch bug 125282.
Um, if the focus isn't moved at mousedown event handling, bug 125282 can cause this bug.
Component: General → Event Handling
OS: Mac OS X → All
Product: Firefox → Core
QA Contact: general → events
I'll note that Bespin is affected by this, which is how we noticed it in the first place. Bespin might not be the only app that moves focus in mousedown.
Blocks: 588381
After bug 125282, we don't allow content to grab the focus away from chrome.

We don't shift the focus to the content area until after the mousedown event (but before the click and mouseup events). There might be some way to handle focus change on mousedown if we expected to shift the focus to the content area, but since we fixed bug 125282 to prevent possible abuse, I don't think we want to undo that behaviour, or open up other possible abuse points.
I agree that bug 125282 is a good change, and we can likely change Bespin to use click rather than mousedown. I just wonder if there are other apps out there that would change the focus on mousedown and run into confusion if someone is using the Web Console to work on their app.

It doesn't seem like we'd be opening up abuse points by allowing a call to .focus() from a mousedown event to proceed, given that the user does still need to actually click the mouse, right? I have no idea what's involved technically in making that happen, however.
How about this? I created a draft patch in my local tree.

1. Notifies to FocusManager when ESM starts mousedown event handling. And FocusManager should get/hold the document which is handling the mousedown event.
2. When the focus move isn't allowed by but bug 125282 during mousedown event handling (i.e., FocusManager knows the document), FocusManager should allow that the document can move focus if it's can access to the new focus node.
3. Notifies to FocusManager when ESM ends mousedown event handling. Then, FocusManager release the nsIDocument reference.
Attached patch Patch v1.0 (obsolete) — Splinter Review
I think that if you agree this patch's approach, we should fix this bug for compatibility.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #472222 - Flags: review?(enndeakin)
Comment on attachment 472222 [details] [diff] [review]
Patch v1.0

>+    if (!sendFocusEvent && mMouseDownEventHandlingDocument) {
>+      // However, while mouse down event is handling, the handling document's
>+      // content should be able to steal focus.
>+      domNode = do_QueryInterface(mMouseDownEventHandlingDocument);
>+      sendFocusEvent = nsContentUtils::CanCallerAccess(domNode);
>+    }
>   }

I think this only be allowed when the element being focused is in the same document.


>+    for (var button = 0; button < 3; button++) {
>+      // Set focus to a chrome element before syhthesizing a mouse down event.

spelling: synthesizing

>+      BrowserSearch.searchBar.focus();
>+
>+      is(fm.focusedElement, BrowserSearch.searchBar.textbox.inputField,
>+         "Failed to move focus to search bar: button=" +
>+         button);
>+
>+      utils.sendMouseEvent("mousedown", x, y, button, 1, 0);
>+      utils.sendMouseEvent("mouseup", x, y, button, 1, 0);
>+

You should just use EventUtils.synthesizeMouse rather than have to do all this manually.
(In reply to comment #8)
> Comment on attachment 472222 [details] [diff] [review]
> Patch v1.0
> 
> >+    if (!sendFocusEvent && mMouseDownEventHandlingDocument) {
> >+      // However, while mouse down event is handling, the handling document's
> >+      // content should be able to steal focus.
> >+      domNode = do_QueryInterface(mMouseDownEventHandlingDocument);
> >+      sendFocusEvent = nsContentUtils::CanCallerAccess(domNode);
> >+    }
> >   }
> 
> I think this only be allowed when the element being focused is in the same
> document.

if (!sendFocusEvent && mMouseDownEventHandlingDocument) {
  domNode = do_QueryInterface(mMouseDownEventHandlingDocument);
  if (nsContentUtils::CanCallerAccess(domNode)) {
    domNode = do_QueryInterface(aNewContent);
    sendFocusEvent =
      nsContentUtils::CheckSameOrigin(mMouseDownEventHandlingDocument, domNode);
  }
}

Is this okay?
No, I mean when aNewContent->GetDocument() == mMouseDownEventHandlingDocument

Why would we want to perform extra work to allow focusing a document that wasn't clicked on?
E.g., do some web applications want to set focus to subdocument's element? Like rich text editor.
Is there a need to support cases like this? I'm worried that this might open up some obscure case we don't want to allow. I'd rather just allow this specific case.

One could always just use the click event instead of mousedown anyway.
How about this approach? I think this is simple and safe.

if (!sendFocusEvent && mMouseDownEventHandlingDocument) {
  nsPIDOMWindow callerWin = nsContentUtils::GetWindowFromCaller();
  if (callerWin) {
    nsCOMPtr<nsIDocument> callerDoc =
      do_QueryInterface(callerWin->GetExtantDocument());
    sendFocusEvent = (callerDoc == mMouseDownEventHandlingDocument);
  }
}

I think that only the document's script is allowed by this code. The new focus content must be an accessible node by the document.
Attached patch Patch v2.0 (obsolete) — Splinter Review
comment 13's approach.
Attachment #472222 - Attachment is obsolete: true
Attachment #474973 - Flags: review?(enndeakin)
Attachment #472222 - Flags: review?(enndeakin)
Comment on attachment 474973 [details] [diff] [review]
Patch v2.0

>+    if (!sendFocusEvent && mMouseDownEventHandlingDocument) {
>+      // However, while mouse down event is handling, the handling document's
>+      // script should be able to steal focus.
>+      nsPIDOMWindow* callerWin = nsContentUtils::GetWindowFromCaller();
>+      if (callerWin) {
>+        nsCOMPtr<nsIDocument> callerDoc =
>+          do_QueryInterface(callerWin->GetExtantDocument());
>+        sendFocusEvent = (callerDoc == mMouseDownEventHandlingDocument);
>+      }
>+    }

I'm still not really convinced that the caller is relevant and that we should just be checking the document of the element being focused, but can't think of any specific issue it would cause. Maybe ask Olli what he thinks about it?

>+    PRBool resetMouseButtonDownStateOfFM = PR_FALSE;
>+    if (aEvent->message == NS_MOUSE_BUTTON_DOWN &&
>+        NS_IS_TRUSTED_EVENT(aEvent)) {
>+      nsFocusManager* fm = nsFocusManager::GetFocusManager();
>+      if (fm) {
>+        fm->SetMouseButtonDownHandlingDocument(mDocument);
>+        resetMouseButtonDownStateOfFM = PR_TRUE;
>+      }
>+    }
>+

I think it would be safer to add this part and the corresponding cleanup code to nsAutoHandlingUserInputStatePusher so it can be handled more automatically.
Olli, any thoughts of the above?
I think
>+      domNode = do_QueryInterface(mMouseDownEventHandlingDocument);
>+      sendFocusEvent = nsContentUtils::CanCallerAccess(domNode);
should work just fine; content can't access chrome nodes.
Attached patch Patch v3.0Splinter Review
Thank you, smaug.
Attachment #474973 - Attachment is obsolete: true
Attachment #481154 - Flags: review?(enndeakin)
Attachment #474973 - Flags: review?(enndeakin)
Enn, would you rereview it?
Hmm, the patch overlaps a bit with the functionality of 
https://bugzilla.mozilla.org/attachment.cgi?id=481639&action=diff
Um, it's similar to my patch, but you handle mousedown event by PostHandleEvent. But I need to handle it by PreHandleEvent because the DOM event handlers for the event is run before PostHandleEvent.
Attachment #481154 - Flags: review?(enndeakin) → review+
Smaug:

Both my patch and your patch are granted by Enn, should both be landed after a+? Or should we create another patch?
My patch is a blocker which I'll land later today.
(In reply to comment #24)
> My patch is a blocker which I'll land later today.

I meant that do we need to optimize/combine the overlapped function before landing? I think that we should just land both patches without any changes for now, and if we need to optimize it, we should do it later. How about you?
Comment on attachment 481154 [details] [diff] [review]
Patch v3.0

Okay, I should fix bug 604289 but this patch blocks it, so, I should land this patch ASAP.

This fixes a regression of bug 125282. This patch allows mouse downed window to steal focus from any elements at mouse down event handler.
Attachment #481154 - Flags: approval2.0?
Masayuki tells me this blocks betaN blocker bug 604289, so marking this one a blocker as well.
Blocks: 604289
blocking2.0: --- → betaN+
Attachment #481154 - Flags: approval2.0?
http://hg.mozilla.org/mozilla-central/rev/261ca48443c0
http://hg.mozilla.org/mozilla-central/rev/4b69c06d878c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.