Last Comment Bug 761927 - Focus is broken with <iframe mozbrowser remote>
: Focus is broken with <iframe mozbrowser remote>
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
Mentors:
: 757763 (view as bug list)
Depends on:
Blocks: b2g-e10s-work 766878
  Show dependency treegraph
 
Reported: 2012-06-05 23:45 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-06-23 05:43 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
gaia test (1.83 KB, patch)
2012-06-07 23:18 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Review
starter patch (3.49 KB, patch)
2012-06-08 00:06 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Review
wip-0.2 (4.43 KB, patch)
2012-06-12 03:01 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
no flags Details | Diff | Review
WIP (4.46 KB, patch)
2012-06-19 17:44 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Review
part 0: Add casting helpers and clean house a little (3.59 KB, patch)
2012-06-21 00:35 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
justin.lebar+bug: review+
Details | Diff | Review
Fix focus for <iframe mozbrowser remote> (3.21 KB, patch)
2012-06-21 00:38 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
justin.lebar+bug: review-
Details | Diff | Review
Fix focus for <iframe mozbrowser remote>, v2 (3.12 KB, patch)
2012-06-21 15:34 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
enndeakin: review+
justin.lebar+bug: review+
Details | Diff | Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-05 23:45:19 PDT
I know discussions of a "proper" IME API are underway, but in the meantime we should tweak the current code to work with content processes so as to unblock a lot of other work.  I think we should be able to implement this entirely within b2g "chrome".  Perhaps involving frame scripts.
Comment 1 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-06-06 03:36:59 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #0)
> I know discussions of a "proper" IME API are underway, but in the meantime
> we should tweak the current code to work with content processes so as to
> unblock a lot of other work.  I think we should be able to implement this
> entirely within b2g "chrome".  Perhaps involving frame scripts.

I will land https://bugzilla.mozilla.org/show_bug.cgi?id=757496, https://bugzilla.mozilla.org/show_bug.cgi?id=757496 and https://bugzilla.mozilla.org/show_bug.cgi?id=754083 which use a frame script :)
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-06 11:41:11 PDT
OK.  If you've already tested with out of process content, feel free to DUP this bug to one of those.

If not let's leave this open to test.
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-07 23:03:21 PDT
IME doesn't work with those patches.
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-07 23:18:52 PDT
Created attachment 631281 [details] [diff] [review]
gaia test
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-08 00:06:06 PDT
Created attachment 631290 [details] [diff] [review]
starter patch

forms.js wasn't loading.  This patch gets forms.js to load

I/Gecko   ( 4839): ###################################### forms.js loaded
I/Gecko   ( 4839): ======================= webapi.js ======================= 

... but still nothing happens.
Comment 6 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-06-08 04:02:14 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #5)
> Created attachment 631290 [details] [diff] [review]
> starter patch
> 
> forms.js wasn't loading.  This patch gets forms.js to load
> 
> I/Gecko   ( 4839): ###################################### forms.js loaded
> I/Gecko   ( 4839): ======================= webapi.js ======================= 
> 
> ... but still nothing happens.

I will have a look at it.
Comment 7 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-06-12 03:01:01 PDT
(In reply to Vivien Nicolas (:vingtetun) from comment #6)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #5)
> > Created attachment 631290 [details] [diff] [review]
> > starter patch
> > 
> > forms.js wasn't loading.  This patch gets forms.js to load
> > 
> > I/Gecko   ( 4839): ###################################### forms.js loaded
> > I/Gecko   ( 4839): ======================= webapi.js ======================= 
> > 
> > ... but still nothing happens.
> 
> I will have a look at it.

The attached version show the keyboard in Gaia.

But it seems like there is a focus issue when I click inside an <iframe mozbrowser remote> (that happens even if forms.js is not loaded). So for example if I click inside an input field the focus does not stay there.
Comment 8 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-06-12 03:01:48 PDT
Created attachment 632186 [details] [diff] [review]
wip-0.2
Comment 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-19 17:10:27 PDT
With that patch applied, I don't see anything happen on desktop b2g.  But I'm running with bug 757137 applied.  Will try on phone and without those patches.
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-19 17:12:25 PDT
Thanks bugzilla, for unsetting that blocking flag.

Please to be restoring (I don't have the powers).
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-19 17:27:04 PDT
Same results without those patches and on device.
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-19 17:44:29 PDT
Created attachment 634684 [details] [diff] [review]
WIP

Fixes typo in last patch.

With this, I now see the IME pop up after clicking an <input> box twice, but no text is entered.
Comment 13 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-19 17:57:27 PDT
There's no focusedWindow or activeWindow.
Comment 14 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-19 20:21:23 PDT
Felipe helpfully implemented cross-process focus in bug 583976, but in this code from those patches

        TabParent* remote = GetRemoteForContent(aContent);
        if (remote) {
          remote->Activate();

remote->Activate() is never being run in b2g, for some reason.  Sniffing around.
Comment 15 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-20 00:17:21 PDT
There's a combination of problems that are leading to badness here.
 - the <iframe mozbrowser remote> embedded within the top-level <html:iframe mozbrowser> seems not to be getting focus, per comment 14.  This may or may not be messing with the focus manager in the content process, not sure yet, but
 - MozKeyboard is using nsIDOMWindowUtils.sendKeyEvent() to deliver keys to content, which relies on focus working properly.  Nothing wrong with that, but
 - if I change it to send Forms:Input:Value, just for testing purposes to work around the focus bugs, the messages end being received by the wrong frame :(.  They're ending up in the system app, which doesn't have focus.
 - for some reason the messages are being delivered to the system app twice.  Not sure what's up there.

The worst of the problems here is definitely focus not working for <iframe mozbrowser remote>.  I think I almost know what's up there, tracking it down.  If that's fixed, it should make key events work but not resolve the other problems.
Comment 16 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-20 02:08:57 PDT
Something is not making sense to me at all here.  I clicked on the <iframe mozbrowser remote> to focus it.  Here's my gdb session from nsFocusManager.cpp, in GetFocusedDescendant() which is trying to walk down the window hierarchy to find the deepest focused element

Breakpoint 2, nsFocusManager::GetFocusedDescendant (aWindow=0xe95b00, aDeep=true, aFocusedWindow=0x7fffffffab10) at /home/cjones/mozilla/mozilla-central/dom/base/nsFocusManager.cpp:269

// Loop 1
(gdb) p window
$1 = (nsGlobalChromeWindow *) 0xe95b00
    currentContent = window->GetFocusedNode();
(gdb) p currentContent
$2 = (nsHTMLIFrameElement *) 0xc4da20   // <html:iframe mozbrowser>?  ("system app")
(gdb) p currentContent->mFrameLoader.mRawPtr->mRemoteBrowser
$10 = (nsFrameLoader::TabParent *) 0x0

    window = GetContentWindow(currentContent);
// Loop 2
(gdb) p window
$11 = (nsGlobalWindow *) 0xc57580
    currentContent = window->GetFocusedNode();
(gdb) p currentContent
$12 = (nsHTMLIFrameElement *) 0x16cfe50  // <iframe mozbrowser remote>?
(gdb) p currentContent->mFrameLoader.mRawPtr->mRemoteBrowser
$13 = (nsFrameLoader::TabParent *) 0x0   // if so, WTF

    window = GetContentWindow(currentContent);
// Loop 3
(gdb) p window
$14 = (nsGlobalWindow *) 0x153f3f0
(gdb) p currentContent
$15 = (nsIContent *) 0x0
(gdb) p window->mDoc.mRawPtr->mFirstChild->mFirstChild 
$25 = (nsIContent *) 0x0                  // WTF, empty document?


I'm still trying to figure what is what here, but
 - maybe we're creating a Window for the <iframe mozbrowser remote> in the same process? o_O  But if so, why does what appears to be the nsFrameLoader for its frame have a null TabParent?

 - and/or we're walking one level too deep in the focus hierarchy, somehow.
Comment 17 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-20 03:00:18 PDT
Thanks to a magical incantation from bz (window->mDoc.mRawPtr->mDocumentURI.mRawPtr->mSpec) I was able to get some more info

(gdb) p window->mDoc.mRawPtr->mDocumentURI.mRawPtr->mSpec
$3 = {
  <nsACString_internal> = {
    mData = 0x981a68 "chrome://browser/content/shell.xul", 

(gdb) p window->mDoc.mRawPtr->mDocumentURI.mRawPtr->mSpec
$6 = {
  <nsACString_internal> = {
    mData = 0x11709f8 "http://system.gaiamobile.org/", 

(gdb) p window->mDoc.mRawPtr->mDocumentURI.mRawPtr->mSpec
$7 = {
  <nsACString_internal> = {
    mData = 0x164a608 "http://homescreen.gaiamobile.org/", 

What happens is, we end up only with one of three states

  shell.xul
    system.gaiamobile.org
      homescreen.gaiamobile.org  <-- focused

  shell.xul
    system.gaiamobile.org

  shell.xul
    system.gaiamobile.org
      keyboard.gaiamobile.org  <-- focused

(plus transitions in and out of those states).  So it looks like the <iframe mozbrowser remote> is invisible to whatever is trying to set the focused window.
Comment 18 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-20 03:26:41 PDT
diff --git a/layout/ipc/RenderFrameParent.cpp b/layout/ipc/RenderFrameParent.cpp
--- a/layout/ipc/RenderFrameParent.cpp
+++ b/layout/ipc/RenderFrameParent.cpp
@@ -692,17 +692,17 @@ RenderFrameParent::BuildDisplayList(nsDi
                                     nsSubDocumentFrame* aFrame,
                                     const nsRect& aDirtyRect,
                                     const nsDisplayListSet& aLists)
 {
   // We're the subdoc for <browser remote="true"> and it has
   // painted content.  Display its shadow layer tree.
   nsDisplayList shadowTree;
   ContainerLayer* container = GetRootLayer();
-  if (aBuilder->IsForEventDelivery() && container) {
+  if (0 && aBuilder->IsForEventDelivery() && container) {
     nsRect bounds = aFrame->EnsureInnerView()->GetBounds();
     ViewTransform offset =
       ViewTransform(GetRootFrameOffset(aFrame, aBuilder), 1, 1);
     BuildListForLayer(container, mFrameLoader, offset,
                       aBuilder, shadowTree, aFrame);
   } else {
     shadowTree.AppendToTop(
       new (aBuilder) nsDisplayRemote(aBuilder, aFrame, this));

doesn't make the bug go away, even though I confirmed that we have the right frame and offset/bounds for the display item.
Comment 19 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-20 16:35:28 PDT
D'oh, this would have called the default hit-testing function, which is a no-op.
Comment 20 :Felipe Gomes (needinfo me!) 2012-06-20 16:44:41 PDT
Chris, is GetRemoteForContent being too strict? It was originally only built for <browser remote>, but I think Justin added the <iframe> condition too. However, it still requires the remote attribute to be present, and that it is a xul element.. So an <html:iframe mozbrowser remote> will return null there (because it's html)
Comment 21 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-20 17:00:57 PDT
It's not being called at all.  There's a failure somewhere else.  We never focus the frame element corresponding to the OOP iframe/browser.

Do you happen to remember if the frontend code manually managed focus the <browser remote>, or it all happened in gecko?
Comment 22 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-20 17:15:12 PDT
The modified hit-test is now "working", but doesn't magically make the bug go away.  (We're finding the nsSubDocumentFrame for the <iframe mozbrowser remote>.)  Digging some more.
Comment 23 :Felipe Gomes (needinfo me!) 2012-06-20 17:16:16 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #21)
> It's not being called at all.  There's a failure somewhere else.  We never
> focus the frame element corresponding to the OOP iframe/browser.

Ah ok, I wasn't sure if you meant the call failed or wasn't being reached. So just keep in mind you'll probably need to remove the aContent->IsXUL() check when it actually gets hit.

> 
> Do you happen to remember if the frontend code manually managed focus the
> <browser remote>, or it all happened in gecko?

After bug 583976, all in Gecko. All that is necessary is that the focus reaches the frame/browser in the parent. It was manually managed before, and you can still call those functions if you wanna test if the other pieces of this bug will work once the frame is focused.
This is the rev where I removed the manual calls, see activateRemoteFrame(): http://hg.mozilla.org/mozilla-central/rev/6759afeead2b
Comment 24 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-20 18:29:57 PDT
I think I'm pretty close to stuck here, would really appreciate some help to move forward.  To summarize,

 - I have content with the following setup
<xul:window>
  <html:iframe mozbrowser>
    // ... other stuff here
    <iframe mozbrowser remote="true">

The remote="true" iframe is loaded in a content process.

 - Clicking the <iframe remote> frame does not move focus to it in the parent process AFAICT.  In particular we never run the code added for bug 583976:
     http://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#1778

 - This prevents events targeting that iframe in the parent process from being forwarded to the content process.

 - I found a potential bug where the <iframe remote>'s nsSubDocumentFrame isn't participating in hit testing.  I don't know if this is *actually* a bug though.  After making the nsSubDocumentFrame participate in hit testing, then I see events targeting the <iframe remote>, but focus still never moves to it AFAICT.

Pointers on how to proceed would be much appreciated!
Comment 25 Neil Deakin 2012-06-20 18:40:43 PDT
How far does it get? Enabling DEBUG_FOCUS will dump a bunch of logging about what methods are called. I can examine the log if you like and see if I work out what is happening.

What should be happening is that nsFocusManager::SetFocus is called with the element that was clicked on. Various things could happen that could cause an early return: that element isn't focusable, the window is inactive, in a non-front tab, etc...
Comment 26 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-20 19:19:56 PDT
Oh!  I thought that I wasn't seeing SetFocus() called for my <iframe remote=true>, but running through again I see

Breakpoint 1, nsFocusManager::SetFocus (this=0x835880, aElement=0xfcadd8, aFlags=0) at /home/cjones/mozilla/mozilla-central/dom/base/nsFocusManager.cpp:438
$12 = (nsHTMLIFrameElement *) 0xfcad40
$13 = (nsFrameLoader::TabParent *) 0xf99820

So there's more I can dig through :).

Thanks!
Comment 27 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-20 22:28:17 PDT
OK, here's what's going wrong.  We get to

 nsFocusManager::CheckIfFocusable
   nsIFrame::IsFocusable
     nsGenericHTMLFrameElement::IsHTMLFocusable
       nsGenericHTMLElement::IsHTMLFocusable

which says that the element *is* focusable, but doesn't return |force=true|.  So then we pop and then call into

 nsFocusManager::CheckIfFocusable
   nsIFrame::IsFocusable
     nsGenericHTMLFrameElement::IsHTMLFocusable
       nsContentUtils::IsSubDocumentTabbable

In IsSubDocumentTabbable(), we hit

  nsIDocument* subDoc = doc->GetSubDocumentFor(aContent);
  if (!subDoc) {
    return false;
  }

and return |false|.  This is expected because the <iframe remote> doesn't have a local subdocument.  So we pop and finally have our last chance at

 nsFocusManager::CheckIfFocusable
   nsIFrame::IsFocusable
     nsGenericHTMLFrameElement::IsHTMLFocusable

    if (!isFocusable && !aWithMouse &&
        GetType() == nsGkAtoms::scrollFrame &&
        mContent->IsHTML() &&
        !mContent->IsRootOfNativeAnonymousSubtree() &&
        mContent->GetParent() &&
        !mContent->HasAttr(kNameSpaceID_None, nsGkAtoms::tabindex)) {

but we're an nsSubDocumentFrame, not a scroll frame.

So that's at least the first part of the bug here.  I'm not really sure what the best fix is; I sort of think that patching bool nsContentUtils::IsSubDocumentTabbable might be the right thing.  Thoughts?
Comment 28 Boris Zbarsky [:bz] 2012-06-20 22:34:00 PDT
Seems plausible, but my knowledge of that code is somewhat cursory....  enn is really the right guy to talk to.
Comment 29 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-21 00:34:07 PDT
Going to focus (hah!) this bug more narrowly.  Let's fix the rest of the IME in a followup, since the brokenness is orthogonal.
Comment 30 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-21 00:35:24 PDT
Created attachment 635212 [details] [diff] [review]
part 0: Add casting helpers and clean house a little
Comment 31 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-21 00:38:45 PDT
Created attachment 635213 [details] [diff] [review]
Fix focus for <iframe mozbrowser remote>

This patch does two things
 - fixes the definition of nsEventStateManager::IsRemote to actually check the "remote" attribute for <iframe mozbrowser>.  Justin, that's we want right?
 - makes a "remote frame" tabbable.  Their subdocuments are in child processes.  I have no idea what the implications of this may be.
 - expand the cross-process focus support in nsFocusManager to include <iframe mozbrowser remote>, and clean house a little.

Thanks to Felipe and Neil for the useful hints!
Comment 32 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-21 00:39:55 PDT
I should add that with these patches, input from the keyboard works 100% fine in desktop b2g for <iframe mozbrowser remote>.  But the b2g/gaia IME setup is still hosed.
Comment 33 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-21 00:40:23 PDT
Sorry for noise, one more note --- Justin/Neil, if you guys have suggestions on how to write a test for this bug, I would very much like to.
Comment 34 Justin Lebar (not reading bugmail) 2012-06-21 09:56:14 PDT
Comment on attachment 635213 [details] [diff] [review]
Fix focus for <iframe mozbrowser remote>

> bool
> nsContentUtils::IsSubDocumentTabbable(nsIContent* aContent)
> {
>   nsIDocument* doc = aContent->GetCurrentDoc();
>   if (!doc) {
>     return false;
>   }
> 
>+  // If the subdocument lives in another process, the frame is
>+  // tabbable.
>+  if (nsEventStateManager::IsRemoteTarget(aContent)) {
>+    return true;
>+  }
>+

I don't know this code, but this doesn't seem right.  A remote subdocument can
have no content viewer just like an in-process subdocument can have no content
viewer, no?

>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
>@@ -1674,20 +1674,21 @@ nsEventStateManager::IsRemoteTarget(nsIC
>       target->AttrValueIs(kNameSpaceID_None, nsGkAtoms::Remote,
>                           nsGkAtoms::_true, eIgnoreCase)) {
>     return true;
>   }
> 
>   // <frame/iframe mozbrowser>
>   nsCOMPtr<nsIMozBrowserFrame> browserFrame = do_QueryInterface(target);
>   if (browserFrame) {
>-    bool isRemote = false;
>-    browserFrame->GetReallyIsBrowser(&isRemote);
>-    if (isRemote) {
>-      return true;
>+    bool isBrowser = false;
>+    browserFrame->GetReallyIsBrowser(&isBrowser);
>+    if (isBrowser) {
>+      return target->AttrValueIs(kNameSpaceID_None, nsGkAtoms::Remote,
>+                                 nsGkAtoms::_true, eIgnoreCase);

A browserFrame can be remote even without an explicit "remote=true".  (This
adds complexity, but I did it because the idea is to phase out the "remote="
attribute once we solve all the OOP issues.)

You really want to get the frameloader's mRemoteFrame variable here, or
otherwise do !!frameLoader->GetRemoteBrowser(), like the focus manager used to
do.
Comment 35 Justin Lebar (not reading bugmail) 2012-06-21 12:10:30 PDT
> A remote subdocument can have no content viewer just like an in-process subdocument can have no 
> content viewer, no?

If this is hard to fix, I'm happy to punt on it for now.  It probably doesn't make a big difference for devices without keyboards...
Comment 36 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-21 12:37:21 PDT
(In reply to Justin Lebar [:jlebar] from comment #34)
> Comment on attachment 635213 [details] [diff] [review]
> Fix focus for <iframe mozbrowser remote>
> 
> > bool
> > nsContentUtils::IsSubDocumentTabbable(nsIContent* aContent)
> > {
> >   nsIDocument* doc = aContent->GetCurrentDoc();
> >   if (!doc) {
> >     return false;
> >   }
> > 
> >+  // If the subdocument lives in another process, the frame is
> >+  // tabbable.
> >+  if (nsEventStateManager::IsRemoteTarget(aContent)) {
> >+    return true;
> >+  }
> >+
> 
> I don't know this code, but this doesn't seem right.  A remote subdocument
> can
> have no content viewer just like an in-process subdocument can have no
> content
> viewer, no?
> 

I don't really understand what this code is trying to achieve, TBH.  But we can't synchronously know any of the information about the subdocument that's being checked below, so it feels like something that should be part of the "remote focus protocol".

> >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
> >@@ -1674,20 +1674,21 @@ nsEventStateManager::IsRemoteTarget(nsIC
> >       target->AttrValueIs(kNameSpaceID_None, nsGkAtoms::Remote,
> >                           nsGkAtoms::_true, eIgnoreCase)) {
> >     return true;
> >   }
> > 
> >   // <frame/iframe mozbrowser>
> >   nsCOMPtr<nsIMozBrowserFrame> browserFrame = do_QueryInterface(target);
> >   if (browserFrame) {
> >-    bool isRemote = false;
> >-    browserFrame->GetReallyIsBrowser(&isRemote);
> >-    if (isRemote) {
> >-      return true;
> >+    bool isBrowser = false;
> >+    browserFrame->GetReallyIsBrowser(&isBrowser);
> >+    if (isBrowser) {
> >+      return target->AttrValueIs(kNameSpaceID_None, nsGkAtoms::Remote,
> >+                                 nsGkAtoms::_true, eIgnoreCase);
> 
> A browserFrame can be remote even without an explicit "remote=true".  (This
> adds complexity, but I did it because the idea is to phase out the "remote="
> attribute once we solve all the OOP issues.)
> 
> You really want to get the frameloader's mRemoteFrame variable here, or
> otherwise do !!frameLoader->GetRemoteBrowser(), like the focus manager used
> to
> do.

Sure, makes sense.  I was mostly curious whether the existing code was wrong, which it seems to be.
Comment 37 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-21 15:34:37 PDT
Created attachment 635509 [details] [diff] [review]
Fix focus for <iframe mozbrowser remote>, v2
Comment 38 Justin Lebar (not reading bugmail) 2012-06-22 07:15:07 PDT
> +  return nsEventStateManager::IsRemoteTarget(aContent) ?
> +    TabParent::GetFrom(aContent) : nsnull;

Can't you just return |TabParent::GetFrom(aContent)|?

> But we can't synchronously know any of the information about the subdocument that's being checked 
> below, so it feels like something that should be part of the "remote focus protocol".

Agreed; the frame would need to somehow asynchronously "reject" the tab.
Comment 39 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-06-22 18:02:27 PDT
(In reply to Justin Lebar [:jlebar] from comment #38)
> > +  return nsEventStateManager::IsRemoteTarget(aContent) ?
> > +    TabParent::GetFrom(aContent) : nsnull;
> 
> Can't you just return |TabParent::GetFrom(aContent)|?
> 

Yes.  In fact, nsFocusManager::GetRemoteForContent is unnecessary now, TabParent::GetFrom does exactly the same thing.  I just removed nsFocusManager::GetRemoteForContent entirely.  I'm assuming that change doesn't need re-review.
Comment 41 Justin Lebar (not reading bugmail) 2012-06-22 19:42:59 PDT
*** Bug 757763 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.