1.05 KB, application/vnd.mozilla.xul+xml
3.17 KB, patch
Benjamin Smedberg: approval1.9.2-
|Details | Diff | Splinter Review|
Created attachment 393277 [details] testFormManager/testClearFormHistory.js using the attached test case, the modal dialog API hangs and does not respond to the instancing of the clear recent history dialog via the tools menu.
That CRH dialog is weird! I tried to get a fix for that but I have no idea where to start. Here some observations: When I let go output from within the getDialog function go to the console I never see an entry for this window. I have three windows open main browser window, Mozmill window, and the Error Console. That means I get the following BrowserChrome flags: 4094, 2147487422, and 2147485198. There is no flag listed for the opened Clear Recent History window. Comparing the opener to other instances of modal dialogs this one uses null for its parent window. But the modal attribute is set. So could it be that when null is specified the window is not listed by getXULWindowEnumerator("")? http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.js#445 As result we hang in our getDialog function: http://hg.mozilla.org/qa/mozmill-tests/file/ac1da292c3b9/shared-modules/testModalDialogAPI.js#l141 Boris, do you know why this window is not listed by the enumerator?
I have no idea. Neil might know... In general, is the issue just that it's app-modal?
Ok, I have flipped null against aParentWindow in sanitize.js and everything works fine. The CRH dialog is now listed when retrieving an enumerator with getXULWindowEnumerator. I know nothing about how the window list is build but by using no parent window seems to create an independent window outside of the window list which will not be detected when the construction of the list happens.
See also bug 309406 (which added the ifdef for Mac) and bug 464930 (CRH shouldn't be a modal dialog).
I don't know about the Mac (which is the only OS that uses a null parent for the CPH dialog). I can't reproduce the problem on Windows.
Comment on attachment 393277 [details] testFormManager/testClearFormHistory.js Forgive my ignorance ... but how do you run this test?
Steven, just clone the mozmill-test repository (http://hg.mozilla.org/qa/mozmill-tests/) and save the test e.g. to firefox/helperClasses. Open Mozmill and run the test. You have to put it into the correct folder due to shared modules dependencies. The same situation can be observed on Windows when changing the first parameter of the window opener to null. It would be helpful to have a better testcase here which doesn't rely on Mozmill. I'll try to create one.
Created attachment 393397 [details] simple window checker This XUL file checks all the open windows and displays their chromeFlags. While writing this code I noticed that I was wrong. The window enumerator returns the correct window instances but opening the CRH with null as parent window seems to ignore the modal flag given to the openWindow function. The flag is not added to the windows chromeFlags but let the window be opened modal. That means we cannot evaluate if this window is modal or not.
Can you call nsIWebBrowserChrome::IsWindowModal perhaps? (QI to nsIInterfaceRequestor and getInterface an nsIWebBrowserChrome)
Thanks Neil! In my testings on Friday I forgot the nsiInterfaceRequestor part so that function wasn't working for me. Now we get the correct modal state of the CRH window. I will incorporate the patch for Mozmill in bug 492136. Does it mean the behavior of the underlying component is ok and will not be changed?
As Neil mentioned on IRC chrome flags should only be used for content windows (pop-ups). So is there anything we can do here or should this bug be closed?
When opening a window from another window (either modal or modeless) then the flags calculated by the window watcher are explicitly set on the newly created chrome window. However if the window is opened without a parent then no chrome flags are set, the XUL window's chromeFlags property has a default value and the window always opens with all of its chrome. Usually nobody cares since the only parentless windows are either the startup browser (which always has all of its chrome) or a random XUL window which doesn't care about chrome flags.
Created attachment 393487 [details] [diff] [review] Proposed patch The simpler patch would have been to add a call to SetChromeFlags to the one caller of CreateTopLevelWindow that doesn't do it itself, but since we're passing the chrome flags around anyway I thought I'd centralise it.
Pushed changeset 5798118100e1 to mozilla-central.
Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20090821 Minefield/3.7a1pre ID:20090821030931 Neil, do you think it's worth to have this patch in 1.9.2?
Mozmill is the only consumer that I know of, so it's up to you, basically.
Comment on attachment 393487 [details] [diff] [review] Proposed patch At the moment our Mozmill tests don't use the flags anymore but it could happen that we have to base on them. So it would be fantastic to get it into 1.9.2.
Comment on attachment 393487 [details] [diff] [review] Proposed patch Caused a regression, can't approve without a complete rollup.