Skip to content

GitLab

T
tangled
Commit 47219e2f authored by Ionel Popescu's avatar Ionel Popescu Committed by Commit Bot
Browse files
Options

Revert "Fixed popup flicker effect for select dropdown." 

This reverts commit 21feee22.

Reason for revert: unblock <select> menu doesn't close on scrolling the page using touch on ChromeOS

Original change's description:
> Fixed popup flicker effect for select dropdown.
>
> The current behavior for a popup is to destroy it, create it and destroy it
> again if the element which was clicked/tapped was already owning the popup.
> This behavior is causing a flicker effect when animation/drop shadow is added
> to the popup.
>
> This behavior can be changed to just store the popup, send the event
> to the element and later destroy the popup if needed.
>
> The popup will be destroyed in the following cases:
> 1. when another element which could open a popup was clicked/tapped
> - WebViewImpl::OpenPagePopup is going to call CancelPagePopup
> 2. when the user has clicked/tapped outside of the popup or on the element which
> had already a popup opened - by checking if the saved popup is
> the same as the current one
>
> Because of the way EventSender::HandleInputEventOnViewOrPopup is implemented
> adding a web_test will not be able to test the impacted scenarios: a popup
> should be dismissed when clicking outside of its area and another one should be
> opened when clicked/tapped on an element which supports having a popup
> (if first popup owner != second popup owner).
> Currently EventSender::HandleInputEventOnViewOrPopup will always send the input
> event to the popup if there is one. The real world implementation is that the popup
> will not receive the event if the click/tap event is outside of its area and the
> WebView is responsible for dismissing it.
>
> Bug: 992238
> Change-Id: I13b7848e6e734e981d4fb9782e68a3d8123bc5ff
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1745769
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Reviewed-by: Kent Tamura <tkent@chromium.org>
> Reviewed-by: Mason Freed <masonfreed@chromium.org>
> Commit-Queue: Ionel Popescu <iopopesc@microsoft.com>
> Cr-Commit-Position: refs/heads/master@{#693370}

TBR=avi@chromium.org,chrishtr@chromium.org,tkent@chromium.org,masonfreed@chromium.org,iopopesc@microsoft.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 992238, 1002020
Change-Id: Idbcc637e2f004ed09bb6cca2324a49fe8709110f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1797059
Commit-Queue: Avi Drissman <avi@chromium.org>
Reviewed-by: default avatarAvi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#695788}
parent 88f4468b
Hide whitespace changes
Inline Side-by-side
Showing with 22 additions and 214 deletions
chrome/browser/popup_operations_interactive_uitest.cc deleted 100644 → 0
View file @ 88f4468b
// Copyright 2019 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "base/files/file_path.h"
#include "base/macros.h"
#include "base/run_loop.h"
#include "base/strings/utf_string_conversions.h"
#include "build/build_config.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_window.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/test/base/in_process_browser_test.h"
#include "chrome/test/base/interactive_test_utils.h"
#include "chrome/test/base/ui_test_utils.h"
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/web_contents.h"
#include "content/public/test/browser_test_utils.h"
#include "ui/base/test/ui_controls.h"
namespace {
const int kSelectHeight = 16;
const int kSelectWidth = 44;
const int kSelectOffsetX = 100;
class PopupOperationsTest : public InProcessBrowserTest {
public:
PopupOperationsTest() {}
// InProcessBrowserTest:
void SetUpOnMainThread() override {
ASSERT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
}
content::WebContents* GetActiveWebContents() {
return browser()->tab_strip_model()->GetActiveWebContents();
}
// Wait for the active web contents title to match |title|.
void WaitForTitle(const std::string& title) {
const base::string16 expected_title(base::ASCIIToUTF16(title));
content::TitleWatcher title_watcher(GetActiveWebContents(), expected_title);
ASSERT_EQ(expected_title, title_watcher.WaitAndGetTitle());
}
void NavigateAndWaitForLoad() {
ASSERT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
// Navigate to the test page and wait for onload to be called.
const GURL url = ui_test_utils::GetTestUrl(
base::FilePath(),
base::FilePath(FILE_PATH_LITERAL("select_popup.html")));
ui_test_utils::NavigateToURL(browser(), url);
WaitForTitle("onload");
}
DISALLOW_COPY_AND_ASSIGN(PopupOperationsTest);
};
IN_PROC_BROWSER_TEST_F(PopupOperationsTest, Load) {
NavigateAndWaitForLoad();
}
#if defined(OS_MACOSX) || defined(OS_CHROMEOS)
// OS_MACOSX: Missing automation provider support: http://crbug.com/1000752.
#define MAYBE_OpenPopup DISABLED_OpenPopup
#else
#define MAYBE_OpenPopup OpenPopup
#endif
// Clicking on a select element should open its popup.
IN_PROC_BROWSER_TEST_F(PopupOperationsTest, MAYBE_OpenPopup) {
NavigateAndWaitForLoad();
// Click on the third select to open its popup.
gfx::Rect bounds = GetActiveWebContents()->GetContainerBounds();
ui_controls::SendMouseMove(bounds.x() + kSelectWidth / 2 + kSelectOffsetX * 2,
bounds.y() + kSelectHeight / 2);
ui_controls::SendMouseClick(ui_controls::LEFT);
WaitForTitle("onclick3");
}
#if defined(OS_MACOSX) || defined(OS_CHROMEOS)
// OS_MACOSX: Missing automation provider support: http://crbug.com/1000752.
#define MAYBE_ChangeValue DISABLED_ChangeValue
#else
#define MAYBE_ChangeValue ChangeValue
#endif
// Clicking on a select element should open its popup and move focus to the
// new popup.
IN_PROC_BROWSER_TEST_F(PopupOperationsTest, MAYBE_ChangeValue) {
NavigateAndWaitForLoad();
// Open the first popup and change the value by pressing UP and Enter.
gfx::Rect bounds = GetActiveWebContents()->GetContainerBounds();
ui_controls::SendMouseMove(bounds.x() + kSelectWidth / 2,
bounds.y() + kSelectHeight / 2);
ui_controls::SendMouseClick(ui_controls::LEFT);
ASSERT_TRUE(ui_test_utils::SendKeyPressSync(browser(), ui::VKEY_UP, false,
false, false, false));
ASSERT_TRUE(ui_test_utils::SendKeyPressSync(browser(), ui::VKEY_RETURN, false,
false, false, false));
WaitForTitle("onchange1");
}
#if defined(OS_MACOSX) || defined(OS_CHROMEOS)
// OS_MACOSX: Missing automation provider support: http://crbug.com/1000752.
#define MAYBE_OpenClosePopup DISABLED_OpenClosePopup
#else
// Temporary disabled due to flakiness: https://crbug.com/1002795.
#define MAYBE_OpenClosePopup DISABLED_OpenClosePopup
#endif
// Clicking on a select element while another select element has its
// popup already open, should open the popup of the clicked select element and
// move the focus to the new popup.
IN_PROC_BROWSER_TEST_F(PopupOperationsTest, MAYBE_OpenClosePopup) {
NavigateAndWaitForLoad();
// Open the first popup, click on the second select to open its popup and
// change its value.
gfx::Rect bounds = GetActiveWebContents()->GetContainerBounds();
ui_controls::SendMouseMove(bounds.x() + kSelectWidth / 2,
bounds.y() + kSelectHeight / 2);
ui_controls::SendMouseClick(ui_controls::LEFT);
ui_controls::SendMouseMove(bounds.x() + kSelectWidth / 2 + kSelectOffsetX,
bounds.y() + kSelectHeight / 2);
ui_controls::SendMouseClick(ui_controls::LEFT);
ASSERT_TRUE(ui_test_utils::SendKeyPressSync(browser(), ui::VKEY_UP, false,
false, false, false));
ASSERT_TRUE(ui_test_utils::SendKeyPressSync(browser(), ui::VKEY_RETURN, false,
false, false, false));
WaitForTitle("onchange2");
}
} // namespace
chrome/test/BUILD.gn
View file @ 47219e2f
......@@ -5395,7 +5395,6 @@ if (!is_android) {
"../browser/password_manager/password_manager_interactive_test_base.cc",
"../browser/password_manager/password_manager_interactive_test_base.h",
"../browser/password_manager/password_manager_interactive_uitest.cc",
"../browser/popup_operations_interactive_uitest.cc",
"../browser/renderer_context_menu/render_view_context_menu_browsertest_util.cc",
"../browser/renderer_context_menu/render_view_context_menu_browsertest_util.h",
"../browser/resource_coordinator/tab_metrics_logger_interactive_uitest.cc",
......
chrome/test/data/select_popup.html deleted 100644 → 0
View file @ 88f4468b
<html>
<head>
<style>
body {
margin: 0px;
padding: 0px;
}
#menu1 {
position: absolute;
top: 0px;
left: 0px;
}
#menu2 {
position: absolute;
top: 0px;
left: 100px;
}
#menu3 {
position: absolute;
top: 0px;
left: 200px;
}
div {
position: absolute;
top: 100px;
}
</style>
<script>
function log(text) {
document.getElementById("status").innerHTML += text + ',';
document.title = text;
}
</script>
</head>
<body onload="log('onload')">
<select id="menu1" onchange="log('onchange1')">
<option>foo</option>
<option>bar</option>
<option>qux</option>
<option selected>baz</option>
</select>
<select id="menu2" onchange="log('onchange2')">
<option>foo</option>
<option>bar</option>
<option>qux</option>
<option selected>baz</option>
</select>
<select id="menu3" onclick="log('onclick3')">
<option>foo</option>
<option>bar</option>
<option>qux</option>
<option selected>baz</option>
</select>
<div id="status"></div>
</body>
</html>
\ No newline at end of file
third_party/blink/renderer/core/exported/web_view_impl.cc
View file @ 47219e2f
......@@ -334,8 +334,9 @@ void WebViewImpl::HandleMouseLeave(LocalFrame& main_frame,
void WebViewImpl::HandleMouseDown(LocalFrame& main_frame,
const WebMouseEvent& event) {
// Save the popup so we can close it if needed after sending the event to
// the element.
// If there is a popup open, close it as the user is clicking on the page
// (outside of the popup). We also save it so we can prevent a click on an
// element from immediately reopening the same popup.
//
// The popup would not be destroyed in this stack normally as it is owned by
// closership from the RenderWidget, which is owned by the browser via the
......@@ -346,6 +347,8 @@ void WebViewImpl::HandleMouseDown(LocalFrame& main_frame,
scoped_refptr<WebPagePopupImpl> page_popup;
if (event.button == WebMouseEvent::Button::kLeft) {
page_popup = page_popup_;
CancelPagePopup();
DCHECK(!page_popup_);
}
// Take capture on a mouse down on a plugin so we can send it mouse events.
......@@ -375,13 +378,11 @@ void WebViewImpl::HandleMouseDown(LocalFrame& main_frame,
main_frame.GetEventHandler().TakeLastMouseDownGestureToken();
}
// If there is a popup open and it is the same as the one we had before
// sending the event to the element it means that the user is clicking outside
// of the popup or on the element which was owning the current opened popup.
if (page_popup_ && page_popup &&
page_popup_->HasSamePopupClient(page_popup.get())) {
// That click triggered a page popup that is the same as the one we just
// closed. It needs to be closed.
CancelPagePopup();
DCHECK(!page_popup_);
}
// Dispatch the contextmenu event regardless of if the click was swallowed.
......@@ -547,14 +548,11 @@ WebInputEventResult WebViewImpl::HandleGestureEvent(
targeted_event);
}
// If there is a popup open and it is the same as the one we had before
// sending the event to the element it means that the user is tapping
// outside of the popup or on the element which was owning the current
// opened popup.
if (page_popup_ && last_hidden_page_popup_ &&
page_popup_->HasSamePopupClient(last_hidden_page_popup_.get())) {
// The tap triggered a page popup that is the same as the one we just
// closed. It needs to be closed.
CancelPagePopup();
DCHECK(!page_popup_);
}
// Don't have this value persist outside of a single tap gesture, plus
// we're done with it now.
......@@ -581,11 +579,14 @@ WebInputEventResult WebViewImpl::HandleGestureEvent(
// Touch pinch zoom and scroll on the page (outside of a popup) must hide
// the popup. In case of a touch scroll or pinch zoom, this function is
// called with GestureTapDown rather than a GSB/GSU/GSE or GPB/GPU/GPE.
// Save the popup so we can prevent the following GestureTap from
// immediately reopening the same popup and close it if needed then.
// When we close a popup because of a GestureTapDown, we also save it so
// we can prevent the following GestureTap from immediately reopening the
// same popup.
// This value should not persist outside of a gesture, so is cleared by
// GestureTap (where it is used) and by GestureCancel.
last_hidden_page_popup_ = page_popup_;
CancelPagePopup();
DCHECK(!page_popup_);
event_result =
MainFrameImpl()->GetFrame()->GetEventHandler().HandleGestureEvent(
targeted_event);
......
third_party/blink/renderer/core/frame/web_frame_widget_impl.cc
View file @ 47219e2f
......@@ -662,11 +662,13 @@ void WebFrameWidgetImpl::HandleMouseLeave(LocalFrame& main_frame,
void WebFrameWidgetImpl::HandleMouseDown(LocalFrame& main_frame,
const WebMouseEvent& event) {
WebViewImpl* view_impl = View();
// Save the popup so we can close it if needed after sending the event to
// the element.
// If there is a popup open, close it as the user is clicking on the page
// (outside of the popup). We also save it so we can prevent a click on an
// element from immediately reopening the same popup.
scoped_refptr<WebPagePopupImpl> page_popup;
if (event.button == WebMouseEvent::Button::kLeft) {
page_popup = view_impl->GetPagePopup();
view_impl->CancelPagePopup();
}
// Take capture on a mouse down on a plugin so we can send it mouse events.
......@@ -698,11 +700,10 @@ void WebFrameWidgetImpl::HandleMouseDown(LocalFrame& main_frame,
main_frame.GetEventHandler().TakeLastMouseDownGestureToken();
}
// If there is a popup open and it is the same as the one we had before
// sending the event to the element it means that the user is clicking outside
// of the popup or on the element which was owning the current opened popup.
if (view_impl->GetPagePopup() && page_popup &&
view_impl->GetPagePopup()->HasSamePopupClient(page_popup.get())) {
// That click triggered a page popup that is the same as the one we just
// closed. It needs to be closed.
view_impl->CancelPagePopup();
}
......
third_party/blink/renderer/core/html/forms/html_select_element.cc
View file @ 47219e2f
......@@ -1980,6 +1980,8 @@ void HTMLSelectElement::ProvisionalSelectionChanged(unsigned list_index) {
void HTMLSelectElement::ShowPopup() {
if (PopupIsVisible())
return;
if (GetDocument().GetPage()->GetChromeClient().HasOpenedPopup())
return;
if (!GetLayoutObject() || !GetLayoutObject()->IsMenuList())
return;
if (VisibleBoundsInVisualViewport().IsEmpty())
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment