Bug 9622 – REGRESSION: showModalDialog returnValue ignored, function result is always "undefined"

• Component: DOM
• Status: RESOLVED FIXED

[Mark Malone]

The result of the showModalDialog call is the value of the returnValue variable. Current webkit builds report "undefined" as the result of the showModalDialog call regardless of the returnValue's value.

[Darin Adler]

Works in 10.4.7.

[Darin Adler]

The problem is that Window::clear() is not being called at all. This is presumably having many bad symptoms, not just this one!

[Darin Adler]

The change that broke this was the one for bug 6818.

[Darin Adler]

Created attachment 9092 [details]
patch, including change log and a manual test

Not the most elegant solution to the problem, but does fix it.

[Darin Adler]

Created attachment 9095 [details]
patch, including change log and a manual test

[Anders Carlsson]

Comment on attachment 9095 [details]
patch, including change log and a manual test

r=me

[Darin Adler]

Committed revision 15097.

[Darin Adler]

<rdar://problem/4604701>

[Darin Adler]

Comment on attachment 9095 [details]
patch, including change log and a manual test

Further testing show this not working in some cases. New patch forthcoming.

[Darin Adler]

Created attachment 9269 [details]
better patch with change log, can still use the old test case

[Darin Adler]

Created attachment 9271 [details]
better patch with change log, can still use the old test case

[Geoffrey Garen]

Comment on attachment 9271 [details]
better patch with change log, can still use the old test case

r=me

I think kjs_window.cpp could use a comment explaining why you need to do this:

+    if (!returnValue)
+        returnValue = dialogWindow->getDirect("returnValue");

Also, it looks like you changed the indenting in KJSProxy::clear from 4 spaces to 2.

[Darin Adler]

(In reply to comment #12)
> I think kjs_window.cpp could use a comment explaining why you need to do this:
> 
> +    if (!returnValue)
> +        returnValue = dialogWindow->getDirect("returnValue");

OK. Will do.

> Also, it looks like you changed the indenting in KJSProxy::clear from 4 spaces
> to 2.

Actually, my previous fix changed the indenting in KJSProxy::clear from 2 spaces to 4, and I rolled it out. My new change was small enough that I did not feel comfortable reindenting the function at the same time.

[Darin Adler]

Committed revision 15239.