Closed Bug 761927 Opened 12 years ago Closed 12 years ago

Focus is broken with <iframe mozbrowser remote>

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla16
Project Flags:
blocking-basecamp +

People

(Reporter: cjones, Assigned: cjones)

References

Details

Attachments

(2 files, 5 obsolete files)

part 0: Add casting helpers and clean house a little
3.59 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
Fix focus for <iframe mozbrowser remote>, v2
3.12 KB, patch
enndeakin
: review+
justin.lebar+bug
: review+
Details | Diff | Splinter Review 
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.
(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 :)
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.
IME doesn't work with those patches.
Attached patch starter patch (obsolete) — — Splinter Review 
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.
(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.
(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.
blocking-basecamp: --- → +
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.
blocking-basecamp: + → ---
Thanks bugzilla, for unsetting that blocking flag.

Please to be restoring (I don't have the powers).
blocking-basecamp: --- → ?
Same results without those patches and on device.
Attached patch WIP (obsolete) — — Splinter Review 
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.
There's no focusedWindow or activeWindow.
blocking-basecamp: ? → +
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.
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.
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.
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.
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.
D'oh, this would have called the default hit-testing function, which is a no-op.
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)
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?
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.
(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
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!
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...
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!
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?
Seems plausible, but my knowledge of that code is somewhat cursory....  enn is really the right guy to talk to.
Going to focus (hah!) this bug more narrowly.  Let's fix the rest of the IME in a followup, since the brokenness is orthogonal.
Summary: Make current IME prototype work with out-of-process content → Focus is broken with <iframe mozbrowser remote>
Assignee: nobody → jones.chris.g

        Attachment #631281 -
        Attachment is obsolete: true

        Attachment #632186 -
        Attachment is obsolete: true

        Attachment #634684 -
        Attachment is obsolete: true

        Attachment #635212 -
        Flags: review?(justin.lebar+bug)

    
    

    
  


  
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!

        Attachment #635213 -
        Flags: review?(justin.lebar+bug)

        Attachment #635213 -
        Flags: review?(enndeakin)

    
    

    
  
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.

    
    

    
  
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.
Component: General → DOM
Product: Boot2Gecko → Core
QA Contact: general → general

        Attachment #635212 -
        Flags: review?(justin.lebar+bug) → review+

    
    

    
  
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.

        Attachment #635213 -
        Flags: review?(justin.lebar+bug) → review-

    
    

    
  
> 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...

    
    

    
  
(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.

    
    

    
  


  

        Attachment #635213 -
        Attachment is obsolete: true

        Attachment #635213 -
        Flags: review?(enndeakin)

        Attachment #635509 -
        Flags: review?(justin.lebar+bug)

        Attachment #635509 -
        Flags: review?(enndeakin)

    
    

    
  
> +  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.

        Attachment #635509 -
        Flags: review?(justin.lebar+bug) → review+

    
  

        Attachment #635509 -
        Flags: review?(enndeakin) → review+

    
    

    
  
(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.

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/671fff6e55ef
https://hg.mozilla.org/mozilla-central/rev/c35d2d3071ac
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Component: DOM → DOM: Core & HTML

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: