Skip to content

Commit

Permalink
[M102] Fenced frame: Make window.focus() rules match element.focus()
Browse files Browse the repository at this point in the history 
rules.

Original CLs that are in this change:
https://chromium-review.googlesource.com/c/chromium/src/+/3594164
https://chromium-review.googlesource.com/c/chromium/src/+/3594041

---

The current rules introduced in the programmatic focus CL are overly
strict. This change will tie window.focus() to transient activation
the same way element.focus() is tied to transient activation. This
will also have window.focus() consume transient user activation for
fenced frames.

This CL also adds assert messages in the script-focus WPT to make the
tests easier to debug.

See CL where rule was originally introduced:
https://chromium-review.googlesource.com/c/chromium/src/+/3569943

Mark script-focus.https.html as flaky

Note that this issue is blocked on https://crbug.com/1066891, so in the
meantime the test will be marked as an expected flake.

Bug: 1319425
Change-Id: Iac9908d1f06e64cf6191eb31a1e7d079df7757be
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3621213
Commit-Queue: Liam Brady <lbrady@google.com>
Reviewed-by: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/branch-heads/5005@{#393}
Cr-Branched-From: 5b4d945-refs/heads/main@{#992738}
  • Loading branch information
Liam Brady authored and Chromium LUCI CQ committed May 3, 2022
1 parent 82c0da5 commit d330bed
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 30 deletions.
13 changes: 11 additions & 2 deletions third_party/blink/renderer/core/frame/dom_window.cc
View file
Expand Up @@ -440,8 +440,17 @@ void DOMWindow::focus(v8::Isolate* isolate) {
if (!page)
return;

if (!frame->ShouldAllowScriptFocus())
return;
if (!frame->ShouldAllowScriptFocus()) {
// Disallow script focus that crosses a fenced frame boundary on a
// frame that doesn't have transient user activation. Note: all calls to
// DOMWindow::focus come from JavaScript calls in the web platform
if (!frame->HasTransientUserActivation())
return;
// Fenced frames should consume user activation when attempting to pull
// focus across a fenced boundary into itself.
if (frame->IsInFencedFrameTree())
LocalFrame::ConsumeTransientUserActivation(DynamicTo<LocalFrame>(frame));
}

RecordWindowProxyAccessMetrics(
WebFeature::kWindowProxyCrossOriginAccessFocus,
Expand Down
28 changes: 13 additions & 15 deletions third_party/blink/renderer/core/page/focus_controller.cc
View file
Expand Up @@ -1307,12 +1307,21 @@ bool FocusController::SetFocusedElement(Element* element,
params.source_capabilities, focus_options);
}

// Disallow programmatic focus that crosses a fenced frame boundary on a
// frame that doesn't have transient user activation.
if (new_focused_frame && !new_focused_frame->ShouldAllowScriptFocus() &&
!new_focused_frame->HasTransientUserActivation() &&
params_to_use.type == mojom::blink::FocusType::kScript) {
return false;
// Disallow script focus that crosses a fenced frame boundary on a
// frame that doesn't have transient user activation.
if (!new_focused_frame->HasTransientUserActivation())
return false;
// Fenced frames should consume user activation when attempting to pull
// focus across a fenced boundary into itself.
// TODO(crbug.com/1123606) Right now the browser can't verify that the
// renderer properly consumed user activation. When user activation code is
// migrated to the browser, move this logic to the browser as well.
if (new_focused_frame->IsInFencedFrameTree()) {
LocalFrame::ConsumeTransientUserActivation(
DynamicTo<LocalFrame>(new_focused_frame));
}
}

if (old_document && old_document != new_document)
Expand All @@ -1323,17 +1332,6 @@ bool FocusController::SetFocusedElement(Element* element,
return false;
}

// TODO(crbug.com/1123606) Right now the browser can't verify that the
// renderer properly consumed user activation. When user activation code is
// migrated to the browser, move this logic to the browser as well.
if (new_focused_frame &&
params_to_use.type == mojom::blink::FocusType::kScript &&
new_focused_frame->IsInFencedFrameTree() &&
!new_focused_frame->ShouldAllowScriptFocus()) {
LocalFrame::ConsumeTransientUserActivation(
DynamicTo<LocalFrame>(new_focused_frame));
}

SetFocusedFrame(new_focused_frame);

if (new_document) {
Expand Down
4 changes: 2 additions & 2 deletions third_party/blink/web_tests/TestExpectations
View file
Expand Up @@ -7741,8 +7741,8 @@ crbug.com/1315753 virtual/selectmenu-new-popup/the-selectmenu-element/selectmenu
# HighlightOverlayPainting: decoration painting hasn’t landed yet.
crbug.com/1147859 virtual/css-highlight-overlay-painting/wpt_internal/css/css-pseudo/target-text-001.html [ Failure ]

crbug.com/1295980 [ Mac ] virtual/fenced-frame-mparch/wpt_internal/fenced_frame/script-focus.https.html [ Pass Timeout ]
crbug.com/1295980 [ Mac ] virtual/fenced-frame-shadow-dom/wpt_internal/fenced_frame/script-focus.https.html [ Pass Timeout ]
crbug.com/1316341 virtual/fenced-frame-mparch/wpt_internal/fenced_frame/script-focus.https.html [ Failure Pass Timeout ]
crbug.com/1316341 virtual/fenced-frame-shadow-dom/wpt_internal/fenced_frame/script-focus.https.html [ Failure Pass Timeout ]

# Branch Sheriff 2022-04-21
crbug.com/1316804 virtual/fenced-frame-shadow-dom/wpt_internal/fenced_frame/urn.https.html [ Skip ]
Expand Down
55 changes: 44 additions & 11 deletions third_party/blink/web_tests/wpt_internal/fenced_frame/script-focus.https.html
View file
Expand Up @@ -18,7 +18,8 @@
const button = document.createElement("button");
document.body.append(button);
button.focus();
assert_equals(document.activeElement == button, expecting_focus);
assert_equals(document.activeElement == button, expecting_focus,
"Button's focus should match expected focus");
}, [expecting_focus]);
}

Expand Down Expand Up @@ -59,8 +60,10 @@
const button = document.createElement("button");
document.body.append(button);
button.focus();
assert_false(document.activeElement == button);
assert_false(navigator.userActivation.isActive);
assert_false(document.activeElement == button,
"The button should not have focus");
assert_false(navigator.userActivation.isActive,
"Window should not have user activation");
}, "An embedder cannot pull focus out of a fenced frame");

promise_test(async () => {
Expand Down Expand Up @@ -122,12 +125,35 @@
const button = document.createElement("button");
document.body.append(button);
button.focus();
assert_false(document.activeElement == button);
assert_false(navigator.userActivation.isActive);
assert_false(document.activeElement == button,
"The button should not have focus");
assert_false(navigator.userActivation.isActive,
"The fenced frame should not have user activation");
});
});
}, "A fenced frame nested in another fenced frame cannot pull focus");

promise_test(async () => {
const [actions, ff1, ff1_element] = await SetupTest();

await ClickOn(document.body, actions);

const button = document.createElement("button");
document.body.append(button);
button.focus();
assert_equals(document.activeElement, button,
"The button in the main page should have focus.");

await ff1.execute(async () => {
assert_false(navigator.userActivation.isActive,
"The fenced frame should not have user activation.");
window.focus();
});

assert_equals(document.activeElement, button,
"The button in the main page should still have focus.");
}, "A fenced frame cannot pull window.focus() without user activation");

promise_test(async () => {
const [actions, ff1, ff1_element] = await SetupTest();

Expand All @@ -137,15 +163,20 @@
const button = document.createElement("button");
document.body.append(button);
button.focus();
assert_equals(document.activeElement, button);
assert_equals(document.activeElement, button,
"The button should have focus.");

await ff1.execute(async () => {
assert_true(navigator.userActivation.isActive);
assert_true(navigator.userActivation.isActive,
"The fenced frame should have user activation.");
window.focus();
assert_false(navigator.userActivation.isActive,
"The fenced frame's user activation should be consumed by the focus");
});

assert_equals(document.activeElement, button);
}, "A fenced frame cannot pull window.focus(), even with user activation");
assert_equals(document.activeElement, document.body,
"The main page's focus should be pulled away from the button.");
}, "A fenced frame can pull window.focus() after user activation");

promise_test(async () => {
var actions = new test_driver.Actions();
Expand All @@ -159,13 +190,15 @@
const button = document.createElement("button");
document.body.append(button);
button.focus();
assert_equals(document.activeElement, button);
assert_equals(document.activeElement, button,
"The button in the iframe should have focus.");
}, [true]);

const button = document.createElement("button");
document.body.append(button);
button.focus();
assert_equals(document.activeElement, button);
assert_equals(document.activeElement, button,
"The button in the main page should have focus.");
}, "An cross-origin iframe can pull focus back and forth without activation");

</script>
Expand Down

0 comments on commit d330bed

Please sign in to comment.