Closed GoogleCodeExporter closed 9 years ago
Thanks for working on this. A few questions:
1. Clicking the 'X' on the popup dialog causes the parent window UI to freeze.
This is probably because the "Console Message" dialog shown by cefclient needs
to be parented to the modal dialog window instead of the parent window. We'll
need some way for the client application to know when a window is being
displayed with modality. Can we make this information available in
OnBeforePopup()?
2. When a modal dialog is displayed animation in the parent window (like your
clock example) stops running. Is this expected/desirable behavior?
Original comment by magreenb...@gmail.com
on 1 Jun 2011 at 2:21
1. Ouch, good catch! I think I see the error in my ways, the parent window is
not being re-enabled in this scenario and I was expecting all code paths go
through UIT_CloseBrowser(). I will investigate an alternative.
2. Yes, same thing happens in Firefox. Chrome::runModal instantiates a class
called PageGroupLoadDeferrer which defers any callback in the opener's page
group while the object is alive. I believe this is being done to make sure the
opener does not attempt to navigate away from the current URL while the modal
dialog is up, timers included.
Original comment by gusver...@gmail.com
on 3 Jun 2011 at 3:07
Attached is a patch that fixes the problem in comment 1 and extends
CefLifeSpanHandler to allow clients to override the modal dialog support.
I changed when CefLifeSpanHandler::OnBeforeClose is being called. The previous
code called this method after the window has been closed, in
UIT_DestroyBrowser(). I need to be able to re-enable the owner window before
the window is destroyed, so I added a WM_CLOSE handler in the
CefBrowserImpl::WndProc and moved the OnBeforeClose call there.
This means that, from now on, it will be up to clients to forward WM_CLOSE
messages to all contained CefBrowserWindow instances when the frame window is
about to close. (See cefclient_win.cpp) if they want to get an OnBeforeClose
callback.
New methods in CefLifespanHandler:
///
// Called just before a modal loop is about to be entered. This gives you
// a chance to override the default implementation which disables the opener
// window and makes this browser window owned by the opener. Return true to
// skip the default. If you handle this call, you will also need to handle
// the OnBeforeClose() call to restore the opener window's state.
///
/*--cef()--*/
virtual bool OnBeforeModal(CefRefPtr<CefBrowser> browser) { return false; }
///
// Called to enter the modal loop. Provide your own modal loop here. Return
// true if you ran your own modal loop and false to use the default.
///
/*--cef()--*/
virtual bool RunModal(CefRefPtr<CefBrowser> browser) { return false; }
///
// Called when a modal browser window has been destroyed. You must implement
// this if you are handling RunModal().
///
/*--cef()--*/
virtual void QuitModal() { }
Original comment by gusver...@gmail.com
on 4 Jun 2011 at 7:46
Attachments:
1:
> I need to be able to re-enable the owner window before the window is
destroyed, so
> I added a WM_CLOSE handler in the CefBrowserImpl::WndProc and moved the
> OnBeforeClose call there.
> This means that, from now on, it will be up to clients to forward WM_CLOSE
> messages to all contained CefBrowserWindow instances when the frame window is
> about to close. (See cefclient_win.cpp) if they want to get an OnBeforeClose
> callback.
What specifically couldn't you do from WM_DESTROY that works from WM_CLOSE? For
example, were the calls to GetAncestor() or EnableWindow() failing?
2:
+CefBrowserImpl::~CefBrowserImpl()
+{
+#if defined(OS_WIN)
+ if (is_modal_) {
+ if( client_.get()) {
+ CefRefPtr<CefLifeSpanHandler> handler = client_->GetLifeSpanHandler();
+ if (handler.get()) {
+ // Notify the handler that it can exit its modal loop if it was in one.
+ handler->QuitModal();
+ }
+ }
+
+ // exit our own internal modal message loop now...
+ if (internal_modal_message_loop_is_active_) {
+ MessageLoop* message_loop = MessageLoop::current();
+ message_loop->QuitNow();
+ }
+ }
+#endif
+}
We can't put this kind of logic in the CefBrowserImpl destructor because there
is no guarantee that the CefBrowserImpl object will be destroyed on the UI
thread. This logic should probably be in UIT_DestroyBrowser() instead.
3: Under what circumstances would a client return true from either
OnBeforeModal or RunModal but not both? It might be better to have only a
RunModal method which would be called before any of the modal-related logic.
Returning true would cancel both the modal setup and execution, and returning
false would execute both.
Original comment by magreenb...@gmail.com
on 6 Jun 2011 at 2:49
Issue 56 has been merged into this issue.
Original comment by magreenb...@gmail.com
on 6 Jun 2011 at 4:33
Thanks for taking the time to review my latest patch.
1. The sequence of events is: WM_CLOSE -> DestroyWindow -> focus change ->
WM_DESTROY. Enabling the window in WM_DESTROY is too late with regards to
restoring focus to the operner window.
2. My bad. I was not that happy with this either. I'll move it to
UIT_DestroyBrowser().
3. I don't have a specific use-case. However, my thinking was that
OnBeforeModal would let you override things that only require Win32 APIs, while
RunModal would let you override, or make use of, the MessageLoop implementation
that is not exposed in libcef. I am OK with simplifying this to just the
RunModal callback.
I'll work on a new patch and add a test case in cefclient.
Original comment by gusver...@gmail.com
on 7 Jun 2011 at 4:43
Attached is a patch that addresses the previous issues and adds a "Modal
Dialog" option to cefclient's tests menu.
Original comment by gusver...@gmail.com
on 10 Jun 2011 at 2:27
Attachments:
Thanks, committed as revision 255 with minor changes:
1. Use |opener_| in place of |is_popup_| and make the implementation
cross-platform.
2. Style-related changes.
Original comment by magreenb...@gmail.com
on 14 Jun 2011 at 3:11
Original issue reported on code.google.com by
gusver...@gmail.com
on 27 May 2011 at 2:50Attachments: