From 4ec997cb455c429be5b26681b0515428e086cac1 Mon Sep 17 00:00:00 2001 From: Nate Chapin Date: Fri, 24 Feb 2023 10:09:20 -0800 Subject: [PATCH] Refactor NavigateEvent state checks * Add a PerformSharedChecks() helper for entry point checks that apply to all functions on NavigateEvent (e.g., checking for detached window, whether the event is trusted, etc.) * Add a state machine for tracking interception state in NavigateEvent. Current states are None, Intercepted, Committed, Scrolled, Finished, and transitions must increase. This is hopefully easy to follow than the several independent booleans we are currently using. * Adding the state machine shows that it's possible to scroll before commit by calling scroll() during navigate event dispatch (and in fact, several tests do so). Fix that. Change-Id: I48095b97a3463d6bc145114639dbfc5711822b27 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4262131 Commit-Queue: Nate Chapin Reviewed-by: Domenic Denicola Cr-Commit-Position: refs/heads/main@{#1109657} --- .../focus-reset/focus-reset-timing.html | 61 +++++++++++++++++++ .../after-transition-explicit-scroll.html | 12 ++-- .../manual-immediate-scroll.html | 8 +-- .../manual-scroll-repeated.html | 16 +++-- .../scroll-after-preventDefault.html | 21 +++++++ .../scroll-on-synthetic-event.html | 19 ++++++ 6 files changed, 123 insertions(+), 14 deletions(-) create mode 100644 navigation-api/focus-reset/focus-reset-timing.html create mode 100644 navigation-api/scroll-behavior/scroll-after-preventDefault.html create mode 100644 navigation-api/scroll-behavior/scroll-on-synthetic-event.html diff --git a/navigation-api/focus-reset/focus-reset-timing.html b/navigation-api/focus-reset/focus-reset-timing.html new file mode 100644 index 00000000000000..df9f03afe9f369 --- /dev/null +++ b/navigation-api/focus-reset/focus-reset-timing.html @@ -0,0 +1,61 @@ + + + + + + diff --git a/navigation-api/scroll-behavior/after-transition-explicit-scroll.html b/navigation-api/scroll-behavior/after-transition-explicit-scroll.html index 83d5cffd2d0968..4b7d0754747821 100644 --- a/navigation-api/scroll-behavior/after-transition-explicit-scroll.html +++ b/navigation-api/scroll-behavior/after-transition-explicit-scroll.html @@ -13,10 +13,14 @@ await navigation.navigate("#frag").finished; assert_not_equals(window.scrollY, 0); navigation.onnavigate = t.step_func(e => { - e.intercept({ scroll: "after-transition" }); - assert_not_equals(window.scrollY, 0); - e.scroll(); - assert_equals(window.scrollY, 0); + e.intercept({ + scroll: "after-transition", + handler: t.step_func(() => { + assert_not_equals(window.scrollY, 0); + e.scroll(); + assert_equals(window.scrollY, 0); + }) + }); }); await navigation.back().finished; assert_equals(window.scrollY, 0); diff --git a/navigation-api/scroll-behavior/manual-immediate-scroll.html b/navigation-api/scroll-behavior/manual-immediate-scroll.html index 112910d46f751d..bafcf6b25628cf 100644 --- a/navigation-api/scroll-behavior/manual-immediate-scroll.html +++ b/navigation-api/scroll-behavior/manual-immediate-scroll.html @@ -14,11 +14,11 @@ assert_not_equals(window.scrollY, 0); navigation.onnavigate = e => { e.intercept({ scroll: "manual" }); - e.scroll(); - assert_equals(window.scrollY, 0); + assert_throws_dom("InvalidStateError", () => e.scroll()); + assert_not_equals(window.scrollY, 0); } await navigation.back().finished; - assert_equals(window.scrollY, 0); -}, "scroll: scroll() should work inside a navigate event handler"); + assert_not_equals(window.scrollY, 0); +}, "scroll: scroll() should not work inside a navigate event handler"); diff --git a/navigation-api/scroll-behavior/manual-scroll-repeated.html b/navigation-api/scroll-behavior/manual-scroll-repeated.html index f9bdb749690f6e..12391460880e03 100644 --- a/navigation-api/scroll-behavior/manual-scroll-repeated.html +++ b/navigation-api/scroll-behavior/manual-scroll-repeated.html @@ -12,12 +12,16 @@ assert_equals(window.scrollY, 0); await navigation.navigate("#frag").finished; assert_not_equals(window.scrollY, 0); - navigation.onnavigate = t.step_func(e => { - e.intercept({ scroll: "manual" }); - e.scroll(); - assert_equals(window.scrollY, 0); - assert_throws_dom("InvalidStateError", () => e.scroll()); - }); + navigation.onnavigate = e => { + e.intercept({ + scroll: "manual", + handler: t.step_func(() => { + e.scroll(); + assert_equals(window.scrollY, 0); + assert_throws_dom("InvalidStateError", () => e.scroll()); + }) + }); + }; await navigation.back().finished; assert_equals(window.scrollY, 0); }, "scroll: scroll() should throw if called a second time"); diff --git a/navigation-api/scroll-behavior/scroll-after-preventDefault.html b/navigation-api/scroll-behavior/scroll-after-preventDefault.html new file mode 100644 index 00000000000000..d83d341feb1d71 --- /dev/null +++ b/navigation-api/scroll-behavior/scroll-after-preventDefault.html @@ -0,0 +1,21 @@ + + + + +
+
+ + diff --git a/navigation-api/scroll-behavior/scroll-on-synthetic-event.html b/navigation-api/scroll-behavior/scroll-on-synthetic-event.html new file mode 100644 index 00000000000000..7efc2d1d980f28 --- /dev/null +++ b/navigation-api/scroll-behavior/scroll-on-synthetic-event.html @@ -0,0 +1,19 @@ + + + + + +