Commit 4773122e authored by Adithya Srinivasan's avatar Adithya Srinivasan Committed by Chromium LUCI CQ

Portals: Fix a couple of focus edge cases

Fixes a couple of instances where portals could get page focus:

1) Committing a pending navigation in an iframe inside a portal could
   cause the portal's frame tree to receive page focus
2) A predecessor page after activation would still have page focus when
   orphaned (not inserted into DOM)

Bug: 1030838
Change-Id: I671d35fbb9b0041adb2d91082e1e6b1ac7a47a0e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2563870Reviewed-by: default avatarAlex Moshchuk <alexmos@chromium.org>
Commit-Queue: Adithya Srinivasan <adithyas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#846114}
parent 2233c765
......@@ -616,6 +616,9 @@ void Portal::ActivateImpl(blink::TransferableMessage data,
outer_contents_main_frame_view->Destroy();
}
// Remove page focus from the now orphaned predecessor.
outer_contents->GetMainFrame()->GetRenderWidgetHost()->Blur();
// These pointers are cleared so that they don't dangle in the event this
// object isn't immediately deleted. It isn't done sooner because
// ActivatePortalWebContents misbehaves if the WebContents doesn't appear to
......
......@@ -1808,6 +1808,70 @@ IN_PROC_BROWSER_TEST_F(PortalBrowserTest, DidFocusIPCFromFrameInsidePortal) {
EXPECT_EQ(web_contents_impl->GetFocusedFrame(), main_frame);
}
IN_PROC_BROWSER_TEST_F(PortalBrowserTest, DidFocusIPCFromOrphanedPortal) {
EXPECT_TRUE(NavigateToURL(
shell(), embedded_test_server()->GetURL("portal.test", "/title1.html")));
WebContentsImpl* web_contents_impl =
static_cast<WebContentsImpl*>(shell()->web_contents());
RenderFrameHostImpl* main_frame = web_contents_impl->GetMainFrame();
GURL a_url = embedded_test_server()->GetURL("a.com", "/title1.html");
Portal* portal = CreatePortalToUrl(web_contents_impl, a_url);
WebContentsImpl* portal_contents = portal->GetPortalContents();
RenderFrameHostImpl* portal_main_frame = portal_contents->GetMainFrame();
EXPECT_EQ(web_contents_impl->GetFocusedWebContents(), web_contents_impl);
EXPECT_EQ(web_contents_impl->GetFocusedFrame(), main_frame);
EXPECT_EQ(portal_contents->GetFocusedFrame(), nullptr);
EXPECT_TRUE(main_frame->GetRenderWidgetHost()->is_focused());
// Activate portal, keep in orphaned state for a while, and then adopt and
// insert predecessor.
// TODO(mcnee): Ideally, we would have a test interceptor to precisely
// control when to proceed with adoption.
const int time_in_orphaned_state =
TestTimeouts::tiny_timeout().InMilliseconds();
EXPECT_TRUE(
ExecJs(portal_main_frame,
JsReplace("window.addEventListener('portalactivate', e => { "
" var stop = performance.now() + $1;"
" while (performance.now() < stop) {}"
" var portal = e.adoptPredecessor(); "
" document.body.appendChild(portal);"
"});",
time_in_orphaned_state)));
{
PortalActivatedObserver activated_observer(portal);
PortalCreatedObserver adoption_observer(portal_main_frame);
EXPECT_TRUE(ExecJs(main_frame,
"let portal = document.querySelector('portal');"
"portal.activate();"));
activated_observer.WaitForActivate();
// |web_contents_impl| is orphaned and therefore still points to itself
// as the focused WebContents node in its tree. It shouldn't have a view
// and it shouldn't have page focus.
EXPECT_EQ(web_contents_impl->GetFocusedWebContents(), web_contents_impl);
EXPECT_EQ(web_contents_impl->GetRenderWidgetHostView(), nullptr);
EXPECT_FALSE(main_frame->GetRenderWidgetHost()->is_focused());
// Simulate DidFocusFrame IPC being sent from the renderer while orphaned.
main_frame->DidFocusFrame();
EXPECT_EQ(web_contents_impl->GetFocusedFrame(), main_frame);
EXPECT_FALSE(main_frame->GetRenderWidgetHost()->is_focused());
EXPECT_EQ(blink::mojom::PortalActivateResult::kPredecessorWasAdopted,
activated_observer.WaitForActivateResult());
adoption_observer.WaitUntilPortalCreated();
}
// Adoption is complete, so |web_contents_impl_| is no longer orphaned and is
// an inner WebContents.
EXPECT_EQ(web_contents_impl->GetFocusedWebContents(), portal_contents);
EXPECT_EQ(web_contents_impl->GetFocusedFrame(), main_frame);
EXPECT_FALSE(main_frame->GetRenderWidgetHost()->is_focused());
}
// Test that a renderer process is killed if it sends an AdvanceFocus IPC to
// advance focus into a portal.
IN_PROC_BROWSER_TEST_F(PortalBrowserTest, AdvanceFocusIntoPortal) {
......
......@@ -501,6 +501,9 @@ void FrameTree::ReplicatePageFocus(bool is_focused) {
void FrameTree::SetPageFocus(SiteInstance* instance, bool is_focused) {
RenderFrameHostManager* root_manager = root_->render_manager();
// Portal frame tree should not get page focus.
DCHECK(!GetMainFrame()->InsidePortal() || !is_focused);
// This is only used to set page-level focus in cross-process subframes, and
// requests to set focus in main frame's SiteInstance are ignored.
if (instance != root_manager->current_frame_host()->GetSiteInstance()) {
......
......@@ -2973,7 +2973,8 @@ void RenderFrameHostManager::CommitPending(
// Remember if the page was focused so we can focus the new renderer in
// that case.
bool focus_render_view = old_view && old_view->HasFocus();
bool focus_render_view =
render_frame_host_->GetMainFrame()->GetRenderWidgetHost()->is_focused();
// Remove the current frame and its descendants from the set of fullscreen
// frames immediately. They can stay in pending deletion for some time.
......@@ -3072,7 +3073,7 @@ void RenderFrameHostManager::CommitPending(
if (is_main_frame) {
new_view->Focus();
} else {
// The current tab has page-level focus, so we need to propagate
// The current WebContents has page-level focus, so we need to propagate
// page-level focus to the subframe's renderer. Before doing that, also
// tell the new renderer what the focused frame is if that frame is not
// in its process, so that Blink's page-level focus logic won't try to
......
......@@ -2514,8 +2514,8 @@ TEST_P(RenderFrameHostManagerTest,
DidNavigateFrame(child, hostB);
// Ensure that the main page is focused.
main_test_rfh()->GetView()->Focus();
EXPECT_TRUE(main_test_rfh()->GetView()->HasFocus());
main_test_rfh()->GetRenderWidgetHost()->SetPageFocus(true);
EXPECT_TRUE(main_test_rfh()->GetRenderWidgetHost()->is_focused());
// Navigate the subframe to C.
NavigationEntryImpl entryC(
......
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