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)
Tracking
()
RESOLVED
FIXED
People
(Reporter: smaug, Assigned: smaug)
References
Details
Attachments
(2 files, 3 obsolete files)
16.83 KB,
patch
|
karlt
:
review+
benjamin
:
review+
|
Details | Diff | Splinter Review |
16.77 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 2•15 years ago
|
||
Ok, I think I should at least basic (tabbing etc) working pretty soon.
Assignee: nobody → Olli.Pettay
Assignee | ||
Comment 3•15 years ago
|
||
Tabbing through chrome and content should work. Need to still think about the APIs.
Assignee | ||
Comment 4•15 years ago
|
||
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
Assignee | ||
Comment 5•15 years ago
|
||
Attachment #407017 -
Attachment is obsolete: true
Assignee | ||
Comment 6•15 years ago
|
||
I'm not sure who should review this all, especially the widget/ part. Note, this fixes only gtk2, not windows.
Karl is our GTK2 guy now :-)
Assignee | ||
Comment 8•15 years ago
|
||
Comment on attachment 409388 [details] [diff] [review] patch Karl, could you look at the widget/ part?
Attachment #409388 -
Flags: review?(mozbugz)
Assignee | ||
Comment 9•15 years ago
|
||
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 10•15 years ago
|
||
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+
Assignee | ||
Comment 11•15 years ago
|
||
> >+#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.
Assignee | ||
Comment 12•15 years ago
|
||
> >+ <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.
Updated•15 years ago
|
Attachment #409388 -
Flags: review?(mozbugz) → review+
Comment 13•15 years ago
|
||
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.
Assignee | ||
Comment 14•15 years ago
|
||
(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.
Assignee | ||
Comment 15•15 years ago
|
||
(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.
Assignee | ||
Comment 16•15 years ago
|
||
(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.
Assignee | ||
Comment 17•15 years ago
|
||
..but since NS_(DE)ACTIVATE is used by nsObjectFrame, I don't want to change it.
Comment 18•15 years ago
|
||
(In reply to comment #17) Thanks for looking into that. Let's discuss this in bug 526247.
Updated•15 years ago
|
Summary: Sort out focus handling is Electrolysis → Sort out focus handling in Electrolysis
Assignee | ||
Comment 19•15 years ago
|
||
Assignee | ||
Comment 20•15 years ago
|
||
Attachment #410540 -
Attachment is obsolete: true
Assignee | ||
Comment 21•15 years ago
|
||
http://hg.mozilla.org/projects/electrolysis/rev/43851a1c6bc6 I'll file a followup for Windows.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•