Bug 36062 – Chromium should not lose activation to select popups

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jay Campan]

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.

[Jay Campan]

Created attachment 50605 [details]
Make the select popup non-activated

[WebKit Review Bot]

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]

Created attachment 50606 [details]
Fixed style issues

[Jay Campan]

Created attachment 50739 [details]
Keeping the old WebViewClient method so not break the Chromium build.

[Eric Seidel (no email)]

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]

Created attachment 51159 [details]
Applied changes suggested by Eric

[WebKit Review Bot]

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 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]

Created attachment 51297 [details]
Fixing more style issues

[WebKit Review Bot]

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]

Created attachment 51298 [details]
One more style issue

[WebKit Commit Bot]

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]

All reviewed patches have been landed.  Closing bug.

[Nate Chapin]

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]

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 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]

All reviewed patches have been landed.  Closing bug.

[Abhishek Arya]

*** Bug 37423 has been marked as a duplicate of this bug. ***

[Yair Yogev]

This fix may have caused Bug 47769