Bug 18677 - Synchronous event dispatch confuses the popup blocker into incorrectly blocking chat popups @ gmail.com
Summary: Synchronous event dispatch confuses the popup blocker into incorrectly blocki...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Mark Rowe (bdash)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2008-04-22 11:31 PDT by Geoffrey Garen
Modified: 2008-04-24 17:50 PDT (History)
2 users (show)

See Also:


Attachments
A patch of a layout test (1.92 KB, patch)
2008-04-22 11:59 PDT, Feng Qian
ggaren: review+
Details | Formatted Diff | Diff
Patch of kjs_events.cpp (1.43 KB, patch)
2008-04-22 12:00 PDT, Feng Qian
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2008-04-22 11:31:34 PDT
Feng Qian on the GMail team provided this diagnosis.

WORKS (popout escapes blocker):

Click on 'New window' in conversation view
Shift-C keyboard shortcut
Shift-R keyboard shortcut


DOES NOT WORK (grrrrr):

Shift-click on 'compose mail'
Shift-click on thread in threadlist view
Pop out chat

Looks like the issue is nested event handling.

If a event handler triggers another event handler synchronizely, window.event is gone after exiting the nested event handler. The code causing trouble is in handleEvent function of kjs_events.cpp.

windowWrapper->window()->setCurrentEvent(event);
....
windowWrapper->window()->setCurrentEvent(0);

If we save the old event and resotre it after calling the function, it should work. e.g.,

Event* saved_evt = windowWrapper->window()->currentEvent();
windowWrapper->window()->setCurrentEvent(event);
....
windowWrapper->window()->setCurrentEvent(saved_evt);
Comment 1 Geoffrey Garen 2008-04-22 11:32:17 PDT
<rdar://problem/5880560>
Comment 2 Feng Qian 2008-04-22 11:59:48 PDT
Created attachment 20751 [details]
A patch of a layout test
Comment 3 Feng Qian 2008-04-22 12:00:49 PDT
Created attachment 20752 [details]
Patch of kjs_events.cpp
Comment 4 Geoffrey Garen 2008-04-23 07:51:50 PDT
Comment on attachment 20751 [details]
A patch of a layout test

+if (window.layoutTestController) {
+  layoutTestController.dumpAsText();
+}

According to our style guidelines, this one-line if body should not include braces.
Comment 5 Geoffrey Garen 2008-04-23 07:52:38 PDT
Comment on attachment 20752 [details]
Patch of kjs_events.cpp

+        Event* saved_event = windowWrapper->window()->currentEvent();

According to our style guidelines, variable names should use camelCase, so "saved_event" should be named "savedEvent".
Comment 6 Geoffrey Garen 2008-04-23 07:52:58 PDT
Thanks for fixing this!
Comment 7 Feng Qian 2008-04-23 09:05:46 PDT
ggaren, thanks for you review. You can take the patch and make changes as you suggested and land the patch. I don't have SVN checkin right.
Comment 8 Mark Rowe (bdash) 2008-04-24 17:13:38 PDT
I'm making the requested changes and will land this shortly.
Comment 9 Mark Rowe (bdash) 2008-04-24 17:50:13 PDT
Landed in r32525.