Bug 9622 - REGRESSION: showModalDialog returnValue ignored, function result is always "undefined"
Summary: REGRESSION: showModalDialog returnValue ignored, function result is always "u...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Darin Adler
URL: http://groupaware.com/aspect/dialog/L...
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2006-06-27 11:18 PDT by Mark Malone
Modified: 2006-07-08 12:58 PDT (History)
5 users (show)

See Also:


Attachments
patch, including change log and a manual test (6.96 KB, patch)
2006-06-29 09:14 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
patch, including change log and a manual test (7.66 KB, patch)
2006-06-29 09:36 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
better patch with change log, can still use the old test case (8.22 KB, patch)
2006-07-08 09:05 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
better patch with change log, can still use the old test case (6.72 KB, patch)
2006-07-08 09:43 PDT, Darin Adler
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Malone 2006-06-27 11:18:59 PDT
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.
Comment 1 Darin Adler 2006-06-28 16:00:44 PDT
Works in 10.4.7.
Comment 2 Darin Adler 2006-06-29 08:51:03 PDT
The problem is that Window::clear() is not being called at all. This is presumably having many bad symptoms, not just this one!
Comment 3 Darin Adler 2006-06-29 08:54:26 PDT
The change that broke this was the one for bug 6818.
Comment 4 Darin Adler 2006-06-29 09:14:21 PDT
Created attachment 9092 [details]
patch, including change log and a manual test

Not the most elegant solution to the problem, but does fix it.
Comment 5 Darin Adler 2006-06-29 09:36:54 PDT
Created attachment 9095 [details]
patch, including change log and a manual test
Comment 6 Anders Carlsson 2006-06-29 09:43:31 PDT
Comment on attachment 9095 [details]
patch, including change log and a manual test

r=me
Comment 7 Darin Adler 2006-06-29 19:50:48 PDT
Committed revision 15097.
Comment 8 Darin Adler 2006-06-29 19:52:13 PDT
<rdar://problem/4604701>
Comment 9 Darin Adler 2006-07-08 08:56:51 PDT
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.
Comment 10 Darin Adler 2006-07-08 09:05:46 PDT
Created attachment 9269 [details]
better patch with change log, can still use the old test case
Comment 11 Darin Adler 2006-07-08 09:43:15 PDT
Created attachment 9271 [details]
better patch with change log, can still use the old test case
Comment 12 Geoffrey Garen 2006-07-08 10:57:15 PDT
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.
Comment 13 Darin Adler 2006-07-08 12:41:12 PDT
(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.
Comment 14 Darin Adler 2006-07-08 12:58:29 PDT
Committed revision 15239.