Skip to content

Commit

Permalink
Fix regression in computing <select> visibility.
Browse files Browse the repository at this point in the history
This was regressed by https://crrev.com/c/3792529.

Prior to the above CL, in an out-of-process iframe, the IsEmpty check in
MenuListSelectType::ShowPopup would skip the visual viewport check in
Element::VisibleBoundsInVisualViewport because the frame is not the
page's main frame.

The above CL tried to replace this with an equivalent check at the call
site:

  if (!document.GetFrame()->LocalFrameRoot().IsOutermostMainFrame()) {
    if (local_root_rect.IsEmpty())
      return;
  }

followed by the visual viewport intersection if we're in a main frame.
However, this isn't equivalent because if the element is showing in the
local root we don't return and we execute the visual viewport
intersection code erroneously. If the element's y position in the local
root is greater than the visual viewport's height this will cause it to
be inadvertantly discarded.

The fix here is to only perform the visual viewport check in the
outermost main frame as before.

Bug: 1395079
Change-Id: Ib79f9ab59f80819ca52e41d86e441338fb74a374
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4083099
Reviewed-by: David Bokan <bokan@chromium.org>
Commit-Queue: Srinivas Sista <srinivassista@chromium.org>
Owners-Override: Srinivas Sista <srinivassista@chromium.org>
Cr-Commit-Position: refs/branch-heads/5359@{#1112}
Cr-Branched-From: 27d3765-refs/heads/main@{#1058933}
  • Loading branch information
bokand authored and Chromium LUCI CQ committed Dec 7, 2022
1 parent 252560c commit 932cd03
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 9 deletions.
69 changes: 69 additions & 0 deletions content/browser/site_per_process_hit_test_browsertest.cc
Expand Up @@ -6197,6 +6197,75 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessHitTestBrowserTest,
}
#endif // !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_MAC) && !BUILDFLAG(IS_CASTOS)

// On Mac and Android, the reported menu coordinates are relative to the OOPIF,
// and its screen position is computed later, so this test isn't relevant on
// those platforms. This has been disabled on CastOS due to flakiness per
// crbug.com/1074249. (This test is based on the one above which is disabled
// on CastOS for this reason).
//
// Tests that a <select>'s visibility is correctly computed and thus shows the
// popup when clicked.
#if !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_MAC) && !BUILDFLAG(IS_CASTOS)
IN_PROC_BROWSER_TEST_F(SitePerProcessHitTestBrowserTest,
ScrolledMainFrameSelectInLongIframe) {
GURL main_url(embedded_test_server()->GetURL(
"a.com", "/frame_tree/page_with_tall_positioned_frame.html"));
EXPECT_TRUE(NavigateToURL(shell(), main_url));

FrameTreeNode* root = web_contents()->GetPrimaryFrameTree().root();
FrameTreeNode* child_node = root->child_at(0);

RenderProcessHost* rph = child_node->current_frame_host()->GetProcess();
RenderProcessHostWatcher watcher(
rph, RenderProcessHostWatcher::WATCH_FOR_HOST_DESTRUCTION);

GURL child_url(embedded_test_server()->GetURL(
"b.com", "/site_isolation/page-with-select.html"));
EXPECT_TRUE(NavigateToURLFromRenderer(child_node, child_url));

// This is to make sure that the navigation is completed and the previous
// RenderProcessHost is destroyed.
watcher.Wait();

EXPECT_TRUE(ExecJs(child_node,
"document.querySelector('select').style.top = '700px';"));

WaitForHitTestData(child_node->current_frame_host());

EXPECT_EQ(
" Site A ------------ proxies for B\n"
" +--Site B ------- proxies for A\n"
"Where A = http://a.com/\n"
" B = http://b.com/",
DepictFrameTree(root));

RenderWidgetHostViewBase* rwhv_child = static_cast<RenderWidgetHostViewBase*>(
child_node->current_frame_host()->GetRenderWidgetHost()->GetView());

HitTestRegionObserver hit_test_data_change_observer(
rwhv_child->GetRootFrameSinkId());
hit_test_data_change_observer.WaitForHitTestData();

// Scroll the main frame so that the <select> is visible on screen. The
// element is at (9,700) of the iframe document and the iframe is at (50,50)
// of the main document.
EXPECT_TRUE(ExecJs(root, "window.scrollTo(0, 740);"));

hit_test_data_change_observer.WaitForHitTestDataChange();

auto popup_waiter = std::make_unique<ShowPopupWidgetWaiter>(
web_contents(), child_node->current_frame_host());

// Left click the <select> element inside the iframe.
DispatchMouseDownEventAndWaitUntilDispatch(web_contents(), rwhv_child,
gfx::PointF(15, 710), rwhv_child,
gfx::PointF(15, 710));

// Ensure the popup is requested. This test fails if this timesouts.
popup_waiter->Wait();
}
#endif // !BUILDFLAG(IS_ANDROID) && !BUILDFLAG(IS_MAC) && !BUILDFLAG(IS_CASTOS)

#if defined(USE_AURA)
class SitePerProcessGestureHitTestBrowserTest
: public SitePerProcessHitTestBrowserTest {
Expand Down
19 changes: 10 additions & 9 deletions third_party/blink/renderer/core/html/forms/select_type.cc
Expand Up @@ -325,22 +325,23 @@ void MenuListSelectType::ShowPopup(PopupMenu::ShowEventType type) {

gfx::Rect local_root_rect = select_->VisibleBoundsInLocalRoot();

if (!document.GetFrame()->LocalFrameRoot().IsOutermostMainFrame()) {
if (document.GetFrame()->LocalFrameRoot().IsOutermostMainFrame()) {
gfx::Rect visual_viewport_rect =
document.GetPage()->GetVisualViewport().RootFrameToViewport(
local_root_rect);
visual_viewport_rect.Intersect(
gfx::Rect(document.GetPage()->GetVisualViewport().Size()));
if (visual_viewport_rect.IsEmpty())
return;
} else {
// TODO(bokan): If we're in a remote frame, we cannot access the active
// visual viewport. VisibleBoundsInLocalRoot will clip to the outermost
// main frame but if the user is pinch-zoomed this won't be accurate.
// https://crbug.com/840944.
if (local_root_rect.IsEmpty())
return;
}

gfx::Rect visual_viewport_rect =
document.GetPage()->GetVisualViewport().RootFrameToViewport(
local_root_rect);
visual_viewport_rect.Intersect(
gfx::Rect(document.GetPage()->GetVisualViewport().Size()));
if (visual_viewport_rect.IsEmpty())
return;

if (!popup_) {
popup_ = document.GetPage()->GetChromeClient().OpenPopupMenu(
*document.GetFrame(), *select_);
Expand Down

0 comments on commit 932cd03

Please sign in to comment.