Bug 36062

Summary:

Chromium should not lose activation to select popups

Product:

WebKit

Reporter:

Jay Campan <jcampan>

Component:

WebKit Misc.

Assignee:

Jay Campan <jcampan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, fishd, inferno, japhet, progame+wk, victorw, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

Windows XP

Attachments:

Description	Flags
Make the select popup non-activated	none
Fixed style issues	none
Keeping the old WebViewClient method so not break the Chromium build.	none
Applied changes suggested by Eric	fishd: review-, fishd: commit-queue-
Fixing more style issues	none
One more style issue	none
Fix a crasher on LayoutTests	none

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	Details Formatted Diff Diff
Fixed style issues (16.98 KB, patch) 2010-03-12 10:15 PST, Jay Campan	no flags	Details Formatted Diff Diff
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	Details Formatted Diff Diff
Applied changes suggested by Eric (17.95 KB, patch) 2010-03-19 09:15 PDT, Jay Campan	fishd: review- fishd: commit-queue-	Details Formatted Diff Diff
Fixing more style issues (27.68 KB, patch) 2010-03-22 09:54 PDT, Jay Campan	no flags	Details Formatted Diff Diff
One more style issue (27.68 KB, patch) 2010-03-22 10:00 PDT, Jay Campan	no flags	Details Formatted Diff Diff
Fix a crasher on LayoutTests (31.78 KB, patch) 2010-03-23 18:10 PDT, Jay Campan	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

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.