Closed Bug 522533 Opened 15 years ago Closed 15 years ago

Sort out focus handling in Electrolysis

Categories

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

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(2 files, 3 obsolete files)

      No description provided.
Ok, I think I should at least basic (tabbing etc) working pretty soon.
Assignee: nobody → Olli.Pettay
Attached patch wip for gtk2 (obsolete) — Splinter Review
Tabbing through chrome and content should work.
Need to still think about the APIs.
Attached patch Add some #ifdef MOZ_IPC (obsolete) — Splinter Review
onfocus="this.QueryInterface(Components.interfaces.nsIFrameLoaderOwner).frameLoader.activateRemoteFrame();" is a bit hackish but works for now.
Might be better to modify focusmanager a bit to handle remote iframes.
Attachment #406882 - Attachment is obsolete: true
Blocks: 516727
Blocks: 512521
Attached patch patchSplinter Review
Attachment #407017 - Attachment is obsolete: true
Blocks: 520822
I'm not sure who should review this all, especially the widget/ part.
Note, this fixes only gtk2, not windows.
Comment on attachment 409388 [details] [diff] [review]
patch

Karl, could you look at the widget/ part?
Attachment #409388 - Flags: review?(mozbugz)
Comment on attachment 409388 [details] [diff] [review]
patch

Benjamin, perhaps you could check the rest.

ContentChrome is not too good name for the class.
Perhaps I should change that to ContentDriver and move it to a separate file?
Attachment #409388 - Flags: review?(benjamin)
Comment on attachment 409388 [details] [diff] [review]
patch

It doesn't seem necessary for ContentChrome and TabChild to be different classes: I think you could make TabChild implement nsIWebBrowserChrome2 etc directly, make it a refcounted class, and then in the Alloc functions, instead of doing:

PIFrameEmbeddingChild*
ContentProcessChild::AllocPIFrameEmbedding()
{
    PIFrameEmbeddingChild* iframe = new TabChild();

do

PIFrameEmbeddingChild*
ContentProcessChild::AllocPIFrameEmbedding()
{
    nsRefPtr<PIFrameEmbeddingChild> iframe = new TabChild();
    NS_ADDREF(iframe);

and then instead of deleting it in ContentProcessChild::DeallocPIFrameEmbedding, just NS_RELEASE it.

>diff --git a/content/base/public/nsIFrameLoader.idl b/content/base/public/nsIFrameLoader.idl

>+  /**
>+   * Activate remote frame.
>+   * Does nothing with non-remote frames.
>+   */
>+  void activateRemoteFrame();

Would it make more sense for this to throw NS_ERROR_UNEXPECTED for non-remote frames, even assert that it is not called?

>diff --git a/dom/ipc/PIFrameEmbedding.ipdl b/dom/ipc/PIFrameEmbedding.ipdl

>+    moveFocus(bool forward);
>+
> child:
>     createWidget(MagicWindowHandle parentWidget);
>     destroyWidget();
>@@ -63,6 +66,8 @@
>          PRUint32 width,
>          PRUint32 height);
> 
>+    activate();

I think I know what these methods do, but could you document them with basic doc-comments? In particular, "activate" isn't obviously related to focus.

>diff --git a/dom/ipc/TabParent.h b/dom/ipc/TabParent.h

>+    void SetOwnerElement(nsIDOMElement* aElement) { mFrameElement = aElement; }

I don't have a strong opinion here, but do you really want to store the owner element, instead of say the owner frameloader? It also seems a little risky that this isn't a strong reference but isn't ever cleared when we start tearing down the frameloader.

>diff --git a/dom/ipc/test.xul b/dom/ipc/test.xul
>--- a/dom/ipc/test.xul
>+++ b/dom/ipc/test.xul
>@@ -22,5 +22,6 @@
>     <toolbarbutton onclick="restart()" label="Recover"/>
>   </toolbar>
> 
>-  <browser type="content" src="http://www.google.com/" flex="1" id="page" remote="true"/>
>+  <browser type="content" src="http://www.google.com/" flex="1" id="page" remote="true"
>+           onfocus="this.QueryInterface(Components.interfaces.nsIFrameLoaderOwner).frameLoader.activateRemoteFrame();"/>
> </window>

Is this something that browser elements will always have to do, or should the frameloader/content code do this automatically? File a followup?

>diff --git a/embedding/browser/webBrowser/nsWebBrowser.cpp b/embedding/browser/webBrowser/nsWebBrowser.cpp

>+#ifdef MOZ_IPC
>+      widgetInit.mTopLevelContentWidget = (mContentType == typeContentWrapper);
>+#endif

Why is this #ifdef MOZ_IPC? What happens when embedders try to use this e10s-enabled xulrunner?

>+#ifdef MOZ_IPC
>+    case NS_ACTIVATE: {

Also these? Is this just an e10s hack that we need to sort out later?
Attachment #409388 - Flags: review?(benjamin) → review+
> >+#ifdef MOZ_IPC
> >+      widgetInit.mTopLevelContentWidget = (mContentType == typeContentWrapper);
> >+#endif
> 
> Why is this #ifdef MOZ_IPC? What happens when embedders try to use this
> e10s-enabled xulrunner?
> 
> >+#ifdef MOZ_IPC
> >+    case NS_ACTIVATE: {
> 
> Also these? Is this just an e10s hack that we need to sort out later?

These are MOZ_IPC for now. I think we could remove the ifdef, but needs some testing.
> >+  <browser type="content" src="http://www.google.com/" flex="1" id="page" remote="true"
> >+           onfocus="this.QueryInterface(Components.interfaces.nsIFrameLoaderOwner).frameLoader.activateRemoteFrame();"/>
> > </window>
> 
> Is this something that browser elements will always have to do, or should the
> frameloader/content code do this automatically? File a followup?
Yeah, followup to change focusmanager. Though, using JS isn't too bad either.
The other option (better) option is to change focusmanager.
Attachment #409388 - Flags: review?(mozbugz) → review+
Comment on attachment 409388 [details] [diff] [review]
patch

Can you include function names and more context in future patches, please?
This can be added automatically as described here:
https://developer.mozilla.org/en/Mercurial_FAQ#Configuration

I've only really looked at the widget/ code.

>diff --git a/widget/public/nsWidgetInitData.h b/widget/public/nsWidgetInitData.h
>--- a/widget/public/nsWidgetInitData.h
>+++ b/widget/public/nsWidgetInitData.h
>@@ -138,6 +139,7 @@
>   PRPackedBool  clipChildren, clipSiblings, mDropShadow;
>   PRPackedBool  mListenForResizes;
>   PRPackedBool  mUnicode;
>+  PRPackedBool  mTopLevelContentWidget;
> };
> 
> #endif // nsWidgetInitData_h__
>diff --git a/widget/src/gtk2/nsWindow.cpp b/widget/src/gtk2/nsWindow.cpp
>--- a/widget/src/gtk2/nsWindow.cpp
>+++ b/widget/src/gtk2/nsWindow.cpp
>@@ -539,7 +540,7 @@
>     NS_ASSERTION(mContainer || mIsDestroyed,
>                  "DispatchActivateEvent only intended for container windows");
> 
>-    if (!mIsTopLevel)
>+    if (!mIsTopLevel && !mIsTopLevelContent)
>         return;
> 
> #ifdef ACCESSIBILITY
>@@ -553,7 +554,7 @@
> void
> nsWindow::DispatchDeactivateEvent(void)
> {
>-    if (!mIsTopLevel)
>+    if (!mIsTopLevel && !mIsTopLevelContent)
>         return;
> 
>     nsGUIEvent event(PR_TRUE, NS_DEACTIVATE, this);
>@@ -3700,6 +3701,10 @@
>     BaseCreate(baseParent, aRect, aHandleEventFunction, aContext,
>                aAppShell, aToolkit, aInitData);
> 
>+    if (aInitData) {
>+      mIsTopLevelContent = aInitData->mTopLevelContentWidget;
>+    }
>+
>     // Do we need to listen for resizes?
>     PRBool listenForResizes = PR_FALSE;;
>     if (aNativeParent || (aInitData && aInitData->mListenForResizes))
>diff --git a/widget/src/gtk2/nsWindow.h b/widget/src/gtk2/nsWindow.h
>--- a/widget/src/gtk2/nsWindow.h
>+++ b/widget/src/gtk2/nsWindow.h
>@@ -408,6 +408,8 @@
>     nsCOMPtr<nsIWidget> mParent;
>     // Is this a toplevel window?
>     PRPackedBool        mIsTopLevel;
>+    // Is this a toplevel content window?
>+    PRPackedBool        mIsTopLevelContent;
>     // Has this widget been destroyed yet?
>     PRPackedBool        mIsDestroyed;
> 

I'm guessing it would be fine to just skip the mIsTopLevel checks and always send the NS_(DE)ACTIVATE events.  There were no other listeners last I checked.  Then mTopLevelContentWidget and mIsTopLevelContent won't be needed.

>diff --git a/widget/src/windows/nsWindow.cpp b/widget/src/windows/nsWindow.cpp
>--- a/widget/src/windows/nsWindow.cpp
>+++ b/widget/src/windows/nsWindow.cpp
>diff --git a/widget/src/windows/nsWindow.h b/widget/src/windows/nsWindow.h
>--- a/widget/src/windows/nsWindow.h
>+++ b/widget/src/windows/nsWindow.h
>@@ -406,6 +406,7 @@
>   WNDPROC               mPrevWndProc;
>   HBRUSH                mBrush;
>   PRPackedBool          mIsTopWidgetWindow;
>+  PRPackedBool          mIsTopLevelContent;
>   PRPackedBool          mHas3DBorder;
>   PRPackedBool          mInDtor;
>   PRPackedBool          mIsVisible;

No point in adding mIsTopLevelContent to windows/* while it's not being used.
(In reply to comment #13) 
> I'm guessing it would be fine to just skip the mIsTopLevel checks and always
> send the NS_(DE)ACTIVATE events.  There were no other listeners last I checked.
>  Then mTopLevelContentWidget and mIsTopLevelContent won't be needed.
We don't want to send (DE)ACTIVATE to subframes which have widget.
 
> No point in adding mIsTopLevelContent to windows/* while it's not being used.
Oops, where did that come.
(In reply to comment #10)
> >+  void activateRemoteFrame();
> 
> Would it make more sense for this to throw NS_ERROR_UNEXPECTED for non-remote
> frames, even assert that it is not called?

Certainly not assert, but NS_ERROR_UNEXPECTED is ok.
(In reply to comment #14)
> (In reply to comment #13) 
> > I'm guessing it would be fine to just skip the mIsTopLevel checks and always
> > send the NS_(DE)ACTIVATE events.  There were no other listeners last I checked.

Ah, ok, now I think I know what you mean. I'll test this a bit.
..but since NS_(DE)ACTIVATE is used by nsObjectFrame, I don't want to change it.
Depends on: 526247
(In reply to comment #17)
Thanks for looking into that.
Let's discuss this in bug 526247.
Summary: Sort out focus handling is Electrolysis → Sort out focus handling in Electrolysis
Attached patch betterSplinter Review
Attachment #410540 - Attachment is obsolete: true
http://hg.mozilla.org/projects/electrolysis/rev/43851a1c6bc6

I'll file a followup for Windows.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Depends on: 526814
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.