#6649: Avoid background messaging to handle AJAX page updates #7030

fregante · 2023-12-02T04:32:19Z

Not yet ready for review, but it's a very small change.

What does this PR do?

Close Watch for navigation via the new Navigation API #6649

Discussion

This reduces the time it takes between navigation and reaction, making it almost sync by comparison. I need to:

Verify whether and how this impacts our expectations, specifically around when code is run (race conditions)
Verify how often this fires compared to the old method

Checklist

Add tests
New files added to src/tsconfig.strictNullChecks.json (if possible)
Designate a primary reviewer

fregante · 2023-12-10T05:49:10Z

Why low priority? This is an mv3 issue. Without this, page updates can take seconds after the worker deactivation

fregante · 2023-12-02T04:35:31Z

src/background/navigation.ts

@@ -51,12 +51,6 @@ async function onNavigation({ tabId, frameId }: Target): Promise<void> {
  if (syncFlagOn("navigation-trace")) {
    await traceNavigation({ tabId, frameId });


I have not removed the listeners due to this logging.

What would you like to do?

Leave as is

Drop logging

Attach/remove the listeners depending on the flag (via addAuthListener), instead of checking the flag at every navigation

Attach/remove the listeners depending on the flag (via addAuthListener), instead of checking the flag at every navigation

Lets do this. Unfortunately navigation tracing code is still helpful to understand wacky SPA navigation behavior of some in-house applications

Done in #7178

fregante · 2023-12-02T04:36:56Z

src/background/navigation.ts

@@ -51,12 +51,6 @@ async function onNavigation({ tabId, frameId }: Target): Promise<void> {
  if (syncFlagOn("navigation-trace")) {
    await traceNavigation({ tabId, frameId });
  }
-
-  if (await canAccessTab({ tabId, frameId })) {


I assume that messaging + this check added at least 100ms to the navigation event.

twschiller · 2023-12-10T12:24:52Z

Why low priority? This is an mv3 issue. Without this, page updates can take seconds after the worker deactivation

Thanks for noting the MV3, I was not aware of that context. Will take a look at the PR today

fregante · 2023-12-10T14:39:09Z

It seems that the API matches our expectations, and that our previous HANDLE_NAVIGATE doesn't take long, assuming that the background is alive and not busy:

Screen.Recording.2.mov

The only adjustment needed, so far, is that the navigate event is also fired when unloading the page, which will most certainly cause some noise.

Here's what I used to compare the API:

Build on main, not on this PR
Enable the messenger logging
Filter "HANDLE_NAVIGATE" in the console

Run the code:

window.navigation.addEventListener("navigate", () => {
	console.log('HANDLE_NAVIGATE local ⚡️')
});

codecov · 2023-12-10T14:40:15Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (486c3d5) 71.15% compared to head (b5f022d) 71.11%.
Report is 1 commits behind head on main.

❗ Current head b5f022d differs from pull request most recent head d44fb9c. Consider uploading reports for the commit d44fb9c to get more accurate results

Files	Patch %	Lines
src/contentScript/lifecycle.ts	0.00%	3 Missing ⚠️
src/contentScript/contentScriptCore.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7030      +/-   ##
==========================================
- Coverage   71.15%   71.11%   -0.05%     
==========================================
  Files        1213     1209       -4     
  Lines       37621    37519     -102     
  Branches     7074     7075       +1     
==========================================
- Hits        26768    26680      -88     
+ Misses      10853    10839      -14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

fregante · 2023-12-15T09:54:41Z

I made a testing page to see how this new event behaves: https://pbx.vercel.app/navigation/

I confirmed that it catches the events we're looking for:

hash changes
pushState/replaceState
history navigation (partial)

There's one situation that it doesn't catch:

history navigation from another website. The page however still contains the previous document (see https://web.dev/bfcache/)
1. Click example.com
2. Go back

And also it triggers on unnecessary events:

navigation away from the page
downloads

I think PB can handle additional requests fine though, so this shouldn't be a blocker.

twschiller · 2023-12-21T01:29:49Z

history navigation from another website.

👍 That's OK. I think the user would want to address this with a focus listener/tigger (or introduce an "tab activated trigger"? It's sort of the same use case as flipping to another tab and then going back to the original tab. I don't think we should treat as navigation

And also it triggers on unnecessary events:

Can we filter those out? Looks like navigation away is handled here?: https://github.com/pixiebrix/pixiebrix-extension/pull/7030/files#diff-83e126c90c800dfc9992a0a6213d9f60b6afb4e8d68797228550cdf5535cb25bR642

twschiller · 2023-12-21T01:32:19Z

src/contentScript/contentScriptCore.ts

@@ -66,6 +66,7 @@ export async function init(): Promise<void> {
  initToaster();

  await handleNavigate();


Can we comment on the ordering here? I.e., why is initNavigation after handleNavigate. I think the answer is just: "handle initial load, then watch for additional navigation events"

Done in #7178

twschiller

See comments on your questions

twschiller · 2023-12-21T01:39:09Z

~~@fregante see limitation here: https://developer.mozilla.org/en-US/docs/Web/API/Navigation_API#limitations~~

twschiller

👍 see comments on your questions. Also, can you double-check this works for frame navigation?

fregante · 2023-12-21T02:54:04Z

Can we filter those out?

I looked at the logs and couldn't find a way. I thought I could use the canIntercept property but that's also false on history navigation (back, forward buttons).

Looks like navigation away is handled here?

Actually I don’t think that's doing any good right now, but it looks like I can use that in combination with beforeunload event and avoid calling handleNavigate

The result of

window.onbeforeunload = () => console.log('unload');
window.navigation.addEventListener('navigate', () => {
    console.log('navigate')
    setTimeout(console.log, 0, 'delayed navigate')
})

is

navigate
unload
delayed navigate

I'll work on this a bit more

fregante · 2023-12-21T04:55:57Z

can you double-check this works for frame navigation?

Each document deals with this independently, so it should keep working. The navigate event is fired for the current frame only, so each iframed content script will listen and reach to their own navigate event

twschiller · 2023-12-21T14:00:00Z

I'll work on this a bit more

If the page is going to unload anyway, it might be fine to leave it? The page would navigate before the bricks run/our bricks running wouldn't block the navigation

We can probably get this merged in and do any tweaks in follow up if necessary

fregante · 2023-12-21T15:59:21Z

We can probably get this merged in and do any tweaks in follow up if necessary

Sure. I was working on it today but I suppose I'll open a separate PR. I think I found a solution that correctly avoids the event on unload.

it might be fine to leave it?

True but maybe we will receive errors, for example if a message is initiated but the page is unloaded immediately after. Or even visible breaking and "noisy" actions on "page load" triggers could be shown to the user for a moment.

Use navigation API

384fb0f

twschiller added the low priority label Dec 5, 2023

fregante commented Dec 10, 2023

View reviewed changes

fregante changed the title ~~Use new Navigation API in content script to catch AJAX navigation~~ #6649: Avoid background messaging to handle AJAX page updates Dec 10, 2023

Merge remote-tracking branch 'origin/main' into F/perf/navigation-api

41884b4

vercel bot deployed to Preview December 10, 2023 14:35 View deployment

twschiller added mv3 performance runtime and removed low priority labels Dec 12, 2023

twschiller assigned fregante Dec 12, 2023

Merge remote-tracking branch 'origin/main' into F/perf/navigation-api

531675f

fregante mentioned this pull request Dec 15, 2023

Add navigation pixiebrix/playground#39

Merged

Reduce events

b5f022d

vercel bot deployed to Preview December 15, 2023 09:49 View deployment

fregante marked this pull request as ready for review December 15, 2023 09:54

fregante assigned twschiller Dec 15, 2023

twschiller requested a review from grahamlangford December 21, 2023 01:30

twschiller reviewed Dec 21, 2023

View reviewed changes

twschiller self-requested a review December 21, 2023 01:36

twschiller added this to the 1.8.6 milestone Dec 21, 2023

twschiller approved these changes Dec 21, 2023

View reviewed changes

twschiller self-requested a review December 21, 2023 01:38

twschiller approved these changes Dec 21, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into F/perf/navigation-api

d44fb9c

grahamlangford approved these changes Dec 21, 2023

View reviewed changes

vercel bot deployed to Preview December 21, 2023 15:53 View deployment

fregante enabled auto-merge (squash) December 21, 2023 15:56

fregante merged commit 4aff813 into main Dec 21, 2023
10 checks passed

fregante deleted the F/perf/navigation-api branch December 21, 2023 15:57

This was referenced Dec 22, 2023

#6649: Exclude navigation events that should not trigger page updates #7178

Merged

Reduce reliance on background script #7182

Closed

fregante mentioned this pull request Jan 9, 2024

Cleanup widgets and side effects on context invalidated (reload, update) #4258

Closed

2 tasks

fregante mentioned this pull request Feb 11, 2024

Review usage of webNavigation events with regard to pre-rendered documents #6665

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#6649: Avoid background messaging to handle AJAX page updates #7030

#6649: Avoid background messaging to handle AJAX page updates #7030

fregante commented Dec 2, 2023 •

edited

Loading

fregante commented Dec 10, 2023

fregante Dec 2, 2023

twschiller Dec 21, 2023 •

edited

Loading

fregante Dec 22, 2023

fregante Dec 2, 2023

twschiller commented Dec 10, 2023 •

edited

Loading

fregante commented Dec 10, 2023

codecov bot commented Dec 10, 2023 •

edited

Loading

fregante commented Dec 15, 2023

twschiller commented Dec 21, 2023 •

edited

Loading

twschiller Dec 21, 2023 •

edited

Loading

fregante Dec 22, 2023 •

edited

Loading

twschiller left a comment

twschiller commented Dec 21, 2023 •

edited

Loading

twschiller left a comment

fregante commented Dec 21, 2023

fregante commented Dec 21, 2023

twschiller commented Dec 21, 2023 •

edited

Loading

fregante commented Dec 21, 2023

		@@ -51,12 +51,6 @@ async function onNavigation({ tabId, frameId }: Target): Promise<void> {
		if (syncFlagOn("navigation-trace")) {
		await traceNavigation({ tabId, frameId });

		@@ -66,6 +66,7 @@ export async function init(): Promise<void> {
		initToaster();

		await handleNavigate();

#6649: Avoid background messaging to handle AJAX page updates #7030

#6649: Avoid background messaging to handle AJAX page updates #7030

Conversation

fregante commented Dec 2, 2023 • edited Loading

What does this PR do?

Discussion

Checklist

fregante commented Dec 10, 2023

fregante Dec 2, 2023

Choose a reason for hiding this comment

twschiller Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

fregante Dec 22, 2023

Choose a reason for hiding this comment

fregante Dec 2, 2023

Choose a reason for hiding this comment

twschiller commented Dec 10, 2023 • edited Loading

fregante commented Dec 10, 2023

codecov bot commented Dec 10, 2023 • edited Loading

Codecov Report

fregante commented Dec 15, 2023

twschiller commented Dec 21, 2023 • edited Loading

twschiller Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

fregante Dec 22, 2023 • edited Loading

Choose a reason for hiding this comment

twschiller left a comment

Choose a reason for hiding this comment

twschiller commented Dec 21, 2023 • edited Loading

twschiller left a comment

Choose a reason for hiding this comment

fregante commented Dec 21, 2023

fregante commented Dec 21, 2023

twschiller commented Dec 21, 2023 • edited Loading

fregante commented Dec 21, 2023

fregante commented Dec 2, 2023 •

edited

Loading

twschiller Dec 21, 2023 •

edited

Loading

twschiller commented Dec 10, 2023 •

edited

Loading

codecov bot commented Dec 10, 2023 •

edited

Loading

twschiller commented Dec 21, 2023 •

edited

Loading

twschiller Dec 21, 2023 •

edited

Loading

fregante Dec 22, 2023 •

edited

Loading

twschiller commented Dec 21, 2023 •

edited

Loading

twschiller commented Dec 21, 2023 •

edited

Loading