RESOLVED FIXED 36062
Chromium should not lose activation to select popups
https://bugs.webkit.org/show_bug.cgi?id=36062
Summary Chromium should not lose activation to select popups
Jay Campan
Reported 2010-03-12 09:48:41 PST
When a select popup is opened in Chromium, it steals activation from the browser, causing spurious blur/focus events to be sent on the page. The WebKit Chromium API should support making the select popup not activable.
Attachments
Make the select popup non-activated (16.98 KB, patch)
2010-03-12 10:08 PST, Jay Campan
no flags
Fixed style issues (16.98 KB, patch)
2010-03-12 10:15 PST, Jay Campan
no flags
Keeping the old WebViewClient method so not break the Chromium build. (17.26 KB, patch)
2010-03-15 14:23 PDT, Jay Campan
no flags
Applied changes suggested by Eric (17.95 KB, patch)
2010-03-19 09:15 PDT, Jay Campan
fishd: review-
fishd: commit-queue-
Fixing more style issues (27.68 KB, patch)
2010-03-22 09:54 PDT, Jay Campan
no flags
One more style issue (27.68 KB, patch)
2010-03-22 10:00 PDT, Jay Campan
no flags
Fix a crasher on LayoutTests (31.78 KB, patch)
2010-03-23 18:10 PDT, Jay Campan
no flags
Jay Campan
Comment 1 2010-03-12 10:08:01 PST
Created attachment 50605 [details] Make the select popup non-activated
WebKit Review Bot
Comment 2 2010-03-12 10:12:06 PST
Attachment 50605 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/platform/chromium/PopupMenuChromium.h:119: One space before end of line comments [whitespace/comments] [5] WebKit/chromium/src/WebViewImpl.cpp:844: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 2 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Jay Campan
Comment 3 2010-03-12 10:15:49 PST
Created attachment 50606 [details] Fixed style issues
Jay Campan
Comment 4 2010-03-15 14:23:17 PDT
Created attachment 50739 [details] Keeping the old WebViewClient method so not break the Chromium build.
Eric Seidel (no email)
Comment 5 2010-03-15 15:19:20 PDT
Comment on attachment 50739 [details] Keeping the old WebViewClient method so not break the Chromium build. Extra space: 315 PopupContainer::~PopupContainer() Would appear we desparately need a convenience method for at least part of this: static_cast<ChromeClientChromium*>( 390 m_frameView->frame()->page()->chrome()->client())
Jay Campan
Comment 6 2010-03-19 09:15:33 PDT
Created attachment 51159 [details] Applied changes suggested by Eric
WebKit Review Bot
Comment 7 2010-03-19 09:21:03 PDT
Attachment 51159 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/platform/chromium/PopupMenuChromium.h:42: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Darin Fisher (:fishd, Google)
Comment 8 2010-03-21 23:17:15 PDT
Comment on attachment 51159 [details] Applied changes suggested by Eric > Index: WebCore/page/chromium/ChromeClientChromium.h ... > + // Notifies the client a popup was closed. > + virtual void popupClosed(WebCore::PopupContainer* popupContainer) = 0; nit: ^^^ no need for WebCore:: prefix when already within the WebCore namespace. > Index: WebCore/platform/chromium/PopupMenuChromium.h ... > + // Convenience method that returns the ChromeClient of the frame view this > + // popup is associated with. > + ChromeClientChromium* chromeClientChromium(); nit: // Returns the ChromeClientChromium of the page this popup is associated with. > Index: WebKit/chromium/src/WebViewImpl.cpp ... > +bool WebViewImpl::selectPopupHandleKeyEvent(const WebKeyboardEvent& event) > +{ > + if (!m_selectPopup) > + return false; nit: ^^^ indentation It turns out that the rule for indentation within a namespace changed, and the guidance is to revise the indentation of files as we modify them. So, in this case, PopupContainer.h should have its style fixed. Otherwise, R=me
Jay Campan
Comment 9 2010-03-22 09:54:56 PDT
Created attachment 51297 [details] Fixing more style issues
WebKit Review Bot
Comment 10 2010-03-22 09:56:53 PDT
Attachment 51297 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/platform/chromium/PopupMenuChromium.h:63: One space before end of line comments [whitespace/comments] [5] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Jay Campan
Comment 11 2010-03-22 10:00:53 PDT
Created attachment 51298 [details] One more style issue
WebKit Commit Bot
Comment 12 2010-03-22 20:02:04 PDT
Comment on attachment 51298 [details] One more style issue Clearing flags on attachment: 51298 Committed r56376: <http://trac.webkit.org/changeset/56376>
WebKit Commit Bot
Comment 13 2010-03-22 20:02:10 PDT
All reviewed patches have been landed. Closing bug.
Nate Chapin
Comment 14 2010-03-23 14:32:34 PDT
This patch was reverted in http://trac.webkit.org/changeset/56416 due to consistent crashes in two tests on chromium linux ( fast/forms/select-empty-list.html and fast/forms/slider-delete-while-dragging-thumb.html ).
Jay Campan
Comment 15 2010-03-23 18:10:27 PDT
Created attachment 51469 [details] Fix a crasher on LayoutTests There was a crasher that could happen when a select popup is opened and a page navigates. The cause is that the PopupMenuContainer still exists but the PopupMenuClient has been deleted, and then when hidden the popup would try to access the PopupMenuClient, causing the crasher. We now clear the reference to the PopupMenuClient when the popup is deleted and we don't access it.
WebKit Commit Bot
Comment 16 2010-03-24 11:41:46 PDT
Comment on attachment 51469 [details] Fix a crasher on LayoutTests Clearing flags on attachment: 51469 Committed r56449: <http://trac.webkit.org/changeset/56449>
WebKit Commit Bot
Comment 17 2010-03-24 11:41:51 PDT
All reviewed patches have been landed. Closing bug.
Abhishek Arya
Comment 18 2010-04-12 16:23:56 PDT
*** Bug 37423 has been marked as a duplicate of this bug. ***
Yair Yogev
Comment 19 2010-10-16 07:50:21 PDT
This fix may have caused Bug 47769
Note You need to log in before you can comment on or make changes to this bug.