Skip to content

Commit

Permalink
NavigationAPI: don't reset focus if focus changed before the transiti…
Browse files Browse the repository at this point in the history
…on completes

https://github.com/WICG/navigation-api#focus-management says:
"However, this focus reset will not take place if the user or developer
has manually changed focus while the promise was settling, and that
element is still visible and focusable."

We weren't currently handling the case where the focus was changed
while the promise was settling.

Bug: 1183545
Change-Id: I96636a2cd06cecf1f680b476190e2fa14731b05b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3566413
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Nate Chapin <japhet@chromium.org>
Cr-Commit-Position: refs/heads/main@{#990087}
  • Loading branch information
natechapin authored and Chromium LUCI CQ committed Apr 7, 2022
1 parent 53ca258 commit b537dd6
Show file tree
Hide file tree
Showing 11 changed files with 227 additions and 16 deletions.
1 change: 1 addition & 0 deletions third_party/blink/renderer/core/dom/build.gni
Expand Up @@ -162,6 +162,7 @@ blink_core_sources_dom = [
"flat_tree_traversal.h",
"flat_tree_traversal_forbidden_scope.h",
"focus_params.h",
"focused_element_change_observer.h",
"focusgroup_flags.cc",
"focusgroup_flags.h",
"frame_request_callback_collection.cc",
Expand Down
18 changes: 18 additions & 0 deletions third_party/blink/renderer/core/dom/document.cc
Expand Up @@ -138,6 +138,7 @@
#include "third_party/blink/renderer/core/dom/events/scoped_event_queue.h"
#include "third_party/blink/renderer/core/dom/flat_tree_traversal.h"
#include "third_party/blink/renderer/core/dom/focus_params.h"
#include "third_party/blink/renderer/core/dom/focused_element_change_observer.h"
#include "third_party/blink/renderer/core/dom/layout_tree_builder_traversal.h"
#include "third_party/blink/renderer/core/dom/live_node_list.h"
#include "third_party/blink/renderer/core/dom/mutation_observer.h"
Expand Down Expand Up @@ -2859,6 +2860,7 @@ void Document::Shutdown() {
mojom::blink::FocusType::kNone);
}
sequential_focus_navigation_starting_point_ = nullptr;
focused_element_change_observers_.clear();

if (this == &AXObjectCacheOwner()) {
ax_contexts_.clear();
Expand Down Expand Up @@ -4877,6 +4879,9 @@ bool Document::SetFocusedElement(Element* new_focused_element,
params.type != mojom::blink::FocusType::kScript)
last_focus_type_ = params.type;

for (auto& observer : focused_element_change_observers_)
observer->DidChangeFocus();

focused_element_->SetFocused(true, params.type);
// Setting focus can cause the element to become detached (e.g. if an
// ancestor element's onblur removes it), so return early here if that's
Expand Down Expand Up @@ -8024,6 +8029,7 @@ void Document::Trace(Visitor* visitor) const {
visitor->Trace(unassociated_listed_elements_);
visitor->Trace(intrinsic_size_observer_);
visitor->Trace(anchor_element_interaction_tracker_);
visitor->Trace(focused_element_change_observers_);
visitor->Trace(pending_link_header_preloads_);
Supplementable<Document>::Trace(visitor);
TreeScope::Trace(visitor);
Expand Down Expand Up @@ -8363,6 +8369,18 @@ void Document::RemovePendingLinkHeaderPreloadIfNeeded(
pending_link_header_preloads_.erase(&preload);
}

void Document::AddFocusedElementChangeObserver(
FocusedElementChangeObserver* observer) {
DCHECK(observer);
focused_element_change_observers_.insert(observer);
}

void Document::RemoveFocusedElementChangeObserver(
FocusedElementChangeObserver* observer) {
DCHECK(focused_element_change_observers_.Contains(observer));
focused_element_change_observers_.erase(observer);
}

void Document::WriteIntoTrace(perfetto::TracedValue ctx) const {
perfetto::TracedDictionary dict = std::move(ctx).WriteDictionary();
dict.Add("url", Url());
Expand Down
7 changes: 7 additions & 0 deletions third_party/blink/renderer/core/dom/document.h
Expand Up @@ -155,6 +155,7 @@ template <typename EventType>
class EventWithHitTestResults;
class ExceptionState;
class FontMatchingMetrics;
class FocusedElementChangeObserver;
class FormController;
class FrameCallback;
class FrameScheduler;
Expand Down Expand Up @@ -980,6 +981,9 @@ class CORE_EXPORT Document : public ContainerNode,
void SetActiveElement(Element*);
Element* GetActiveElement() const { return active_element_.Get(); }

void AddFocusedElementChangeObserver(FocusedElementChangeObserver*);
void RemoveFocusedElementChangeObserver(FocusedElementChangeObserver*);

Element* HoverElement() const { return hover_element_.Get(); }

void RemoveFocusedElementOfSubtree(Node&, bool among_children_only = false);
Expand Down Expand Up @@ -2141,6 +2145,9 @@ class CORE_EXPORT Document : public ContainerNode,
Member<RootScrollerController> root_scroller_controller_;
Member<AnchorElementInteractionTracker> anchor_element_interaction_tracker_;

HeapHashSet<Member<FocusedElementChangeObserver>>
focused_element_change_observers_;

double overscroll_accumulated_delta_x_ = 0;
double overscroll_accumulated_delta_y_ = 0;

Expand Down
@@ -0,0 +1,18 @@
// Copyright 2022 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.

#ifndef THIRD_PARTY_BLINK_RENDERER_CORE_DOM_FOCUSED_ELEMENT_CHANGE_OBSERVER_H_
#define THIRD_PARTY_BLINK_RENDERER_CORE_DOM_FOCUSED_ELEMENT_CHANGE_OBSERVER_H_

#include "third_party/blink/renderer/platform/heap/garbage_collected.h"

namespace blink {

class FocusedElementChangeObserver : public GarbageCollectedMixin {
public:
virtual void DidChangeFocus() = 0;
};

} // namespace blink
#endif // THIRD_PARTY_BLINK_RENDERER_CORE_DOM_FOCUSED_ELEMENT_CHANGE_OBSERVER_H_
34 changes: 29 additions & 5 deletions third_party/blink/renderer/core/navigation_api/navigate_event.cc
Expand Up @@ -9,6 +9,7 @@
#include "third_party/blink/renderer/bindings/core/v8/v8_navigation_transition_while_options.h"
#include "third_party/blink/renderer/core/dom/abort_signal.h"
#include "third_party/blink/renderer/core/dom/dom_exception.h"
#include "third_party/blink/renderer/core/dom/element.h"
#include "third_party/blink/renderer/core/event_interface_names.h"
#include "third_party/blink/renderer/core/event_type_names.h"
#include "third_party/blink/renderer/core/frame/local_dom_window.h"
Expand Down Expand Up @@ -82,6 +83,8 @@ void NavigateEvent::transitionWhile(ScriptState* script_state,
return;
}

if (navigation_action_promises_list_.IsEmpty())
DomWindow()->document()->AddFocusedElementChangeObserver(this);
navigation_action_promises_list_.push_back(newNavigationAction);

if (options->hasFocusReset()) {
Expand Down Expand Up @@ -116,18 +119,39 @@ void NavigateEvent::transitionWhile(ScriptState* script_state,
}
}

bool NavigateEvent::ShouldResetFocus() const {
void NavigateEvent::ResetFocusIfNeeded() {
// We only do focus reset if transitionWhile() was called, opting us into the
// new default behavior which the navigation API provides.
if (navigation_action_promises_list_.IsEmpty())
return false;
return;
auto* document = DomWindow()->document();
document->RemoveFocusedElementChangeObserver(this);

// If focus has changed since transitionWhile() was invoked, don't reset
// focus.
if (did_change_focus_during_transition_while_)
return;

// If we're in "navigation API mode" per the above, then either leaving focus
// reset behavior as the default, or setting it to "after-transition"
// explicitly, should reset the focus.
return !focus_reset_behavior_ ||
focus_reset_behavior_->AsEnum() ==
V8NavigationFocusReset::Enum::kAfterTransition;
if (focus_reset_behavior_ &&
focus_reset_behavior_->AsEnum() !=
V8NavigationFocusReset::Enum::kAfterTransition) {
return;
}

if (Element* focus_delegate = document->GetAutofocusDelegate()) {
focus_delegate->focus();
} else {
document->ClearFocusedElement();
document->SetSequentialFocusNavigationStartingPoint(nullptr);
}
}

void NavigateEvent::DidChangeFocus() {
DCHECK(!navigation_action_promises_list_.IsEmpty());
did_change_focus_during_transition_while_ = true;
}

bool NavigateEvent::ShouldSendAxEvents() const {
Expand Down
11 changes: 9 additions & 2 deletions third_party/blink/renderer/core/navigation_api/navigate_event.h
Expand Up @@ -12,6 +12,7 @@
#include "third_party/blink/renderer/bindings/core/v8/v8_navigation_focus_reset.h"
#include "third_party/blink/renderer/bindings/core/v8/v8_navigation_scroll_restoration.h"
#include "third_party/blink/renderer/core/dom/events/event.h"
#include "third_party/blink/renderer/core/dom/focused_element_change_observer.h"
#include "third_party/blink/renderer/core/execution_context/execution_context_lifecycle_observer.h"
#include "third_party/blink/renderer/core/navigation_api/navigation_api.h"
#include "third_party/blink/renderer/platform/heap/collection_support/heap_vector.h"
Expand All @@ -29,7 +30,9 @@ class ExceptionState;
class FormData;
class ScriptPromise;

class NavigateEvent final : public Event, public ExecutionContextClient {
class NavigateEvent final : public Event,
public ExecutionContextClient,
public FocusedElementChangeObserver {
DEFINE_WRAPPERTYPEINFO();

public:
Expand Down Expand Up @@ -66,11 +69,14 @@ class NavigateEvent final : public Event, public ExecutionContextClient {
const HeapVector<ScriptPromise>& GetNavigationActionPromisesList() {
return navigation_action_promises_list_;
}
bool ShouldResetFocus() const;
void ResetFocusIfNeeded();
bool ShouldSendAxEvents() const;

void SaveStateFromDestinationItem(HistoryItem*);

// FocusedElementChangeObserver implementation:
void DidChangeFocus() final;

const AtomicString& InterfaceName() const final;
void Trace(Visitor*) const final;

Expand All @@ -97,6 +103,7 @@ class NavigateEvent final : public Event, public ExecutionContextClient {

enum class ManualRestoreState { kNotRestored, kRestored, kDone };
ManualRestoreState restore_state_ = ManualRestoreState::kNotRestored;
bool did_change_focus_during_transition_while_ = false;
};

} // namespace blink
Expand Down
Expand Up @@ -104,15 +104,7 @@ class NavigateReaction final : public ScriptFunction::Callable {
value);
}

if (navigate_event_->ShouldResetFocus()) {
auto* document = navigation_api->GetSupplementable()->document();
if (Element* focus_delegate = document->GetAutofocusDelegate()) {
focus_delegate->focus();
} else {
document->ClearFocusedElement();
document->SetSequentialFocusNavigationStartingPoint(nullptr);
}
}
navigate_event_->ResetFocusIfNeeded();

if (react_type_ == ReactType::kTransitionWhile && window->GetFrame()) {
window->GetFrame()->Loader().DidFinishNavigation(
Expand Down
@@ -0,0 +1,35 @@
<!doctype html>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<body>
<script>
promise_test(async t => {
let transitionWhile_resolve;
navigation.addEventListener("navigate", e => {
e.transitionWhile(new Promise(resolve => transitionWhile_resolve = resolve),
{ focusReset: "after-transition" });
}, { once: true });

const button = document.body.appendChild(document.createElement("button"));
const button2 = document.body.appendChild(document.createElement("button"));
button2.tabIndex = 0;
t.add_cleanup(() => {
button.remove();
button2.remove();
});

assert_equals(document.activeElement, document.body, "Start on body");
button.focus();
assert_equals(document.activeElement, button, "focus() worked");

const finished = navigation.navigate("#1").finished;
button.onblur = () => button2.focus();
button.blur();
assert_equals(document.activeElement, button2, "focus() in blur worked");

transitionWhile_resolve();
await finished;
assert_equals(document.activeElement, button2, "Focus was not reset after the transition");
}, "");
</script>
</body>
@@ -0,0 +1,36 @@
<!doctype html>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<body>
<script>
promise_test(async t => {
let transitionWhile_resolve;
navigation.addEventListener("navigate", e => {
e.transitionWhile(new Promise(resolve => transitionWhile_resolve = resolve),
{ focusReset: "after-transition" });
}, { once: true });

const button = document.body.appendChild(document.createElement("button"));
const button2 = document.body.appendChild(document.createElement("button"));
button2.tabIndex = 0;
t.add_cleanup(() => {
button.remove();
button2.remove();
});

assert_equals(document.activeElement, document.body, "Start on body");
button.focus();
assert_equals(document.activeElement, button, "focus() worked");

const finished = navigation.navigate("#1").finished;
button2.focus();
assert_equals(document.activeElement, button2, "focus() worked");
button.focus();
assert_equals(document.activeElement, button, "focus() worked");

transitionWhile_resolve();
await finished;
assert_equals(document.activeElement, button, "Focus was not reset after the transition");
}, "");
</script>
</body>
@@ -0,0 +1,34 @@
<!doctype html>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<body>
<script>
promise_test(async t => {
let transitionWhile_resolve;
navigation.addEventListener("navigate", e => {
e.transitionWhile(new Promise(resolve => transitionWhile_resolve = resolve),
{ focusReset: "after-transition" });
}, { once: true });

const button = document.body.appendChild(document.createElement("button"));
const button2 = document.body.appendChild(document.createElement("button"));
button2.tabIndex = 0;
t.add_cleanup(() => {
button.remove();
button2.remove();
});

assert_equals(document.activeElement, document.body, "Start on body");
button.focus();
assert_equals(document.activeElement, button, "focus() worked");

const finished = navigation.navigate("#1").finished;
button2.focus();
assert_equals(document.activeElement, button2, "focus() worked");

transitionWhile_resolve();
await finished;
assert_equals(document.activeElement, button2, "Focus was not reset after the transition");
}, "");
</script>
</body>
@@ -0,0 +1,39 @@
<!doctype html>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<body>
<script>
promise_test(async t => {
let transitionWhile_resolve;
navigation.addEventListener("navigate", e => {
e.transitionWhile(new Promise(resolve => transitionWhile_resolve = resolve), { focusReset: "after-transition" });
}, { once: true });

const button = document.body.appendChild(document.createElement("button"));
const button2 = document.body.appendChild(document.createElement("button"));
button2.tabIndex = 0;
t.add_cleanup(() => {
button.remove();
button2.remove();
});

assert_equals(document.activeElement, document.body, "Start on body");
button.focus();
assert_equals(document.activeElement, button, "focus() worked");

const finished = navigation.navigate("#1").finished;

let onfocus_called = false;
document.body.onfocus = onfocus_called = true;
button.remove();
assert_equals(document.activeElement, document.body, "Removing the element reset focus");
assert_true(onfocus_called);

document.body.onfocus = t.unreached_func("onfocus shouldn't fire a second time due to focus reset");
transitionWhile_resolve();
await finished;
assert_equals(document.activeElement, document.body, "Focus remains on document.body after promise fulfills");
await new Promise(resolve => t.step_timeout(resolve, 10));
}, "");
</script>
</body>

0 comments on commit b537dd6

Please sign in to comment.