Moving too quickly between pages on the popped-out "speaker notes" window resets to non-notes view

[fw-immunant]

To reproduce, tap left and right a couple times while the speaker notes window is focused after popping it out into its own window. It will revert to showing a second copy of the course material, as if the speaker notes window had been closed.

[mgeisler]

Thanks for logging this! The speaker notes system is quite rudimentary and it could benefit from someone with better JavaScript skills than me giving it an overhaul.

[randomPoison]

I've found that navigating at all in the speaker notes immediately breaks the connection between the speaker notes and the main window. I'm assuming this is the same issue that @fw-immunant ran into here.

[djmitche]

I've seen that as well.

[michael-kerscher]

I can also reproduce this reliably by clicking into the notes and go to the right. The speaker notes view is now turning into the regular slide view".
Moving left does break the connection to the regular slides but stays in the speaker notes view - it is independent from the regular slides though

Interesting behavior: moving left or right has a different effect

[michael-kerscher]

I found the issue with the "next" navigation and created a pull request.

There is an additional issue causing the break:

comprehensive-rust/theme/speaker-notes.js

Lines 119 to 123 in c05f0b6

	popup.onload = () => {
	popup.onbeforeunload = () => {
	setState("inline-open");
	applyState();
	};

This triggers on any unload (e.g. access next slide) of the (speaker notes) page - which means prev+next navigation events.

There is a workaround that tries to avoid acting on this, but depending on the the sequence and timing of events this still might break

comprehensive-rust/theme/speaker-notes.js

Lines 209 to 226 in c05f0b6

	// When navigating to another page, we see two state changes in rapid
	// succession:
	//
	// - "popup" -> "inline-open"
	// - "inline-open" -> "popup"
	//
	// When the page is closed, we only see:
	//
	// - "popup" -> "inline-open"
	//
	// We can use a timeout to detect the difference. The effect is that
	// showing the speaker notes is delayed by 500 ms when closing the
	// speaker notes window.
	if (timeout) {
	clearTimeout(timeout);
	}
	timeout = setTimeout(applyState, 500);
	break;

[michael-kerscher]

To add some thoughts on this: We need some mechanism for the slides to discover that the speaker notes are closed. Currently this is done by signaling back to the slides when the speaker notes close (with the beforeunload event - that triggers for any unload, be it window close or some full navigation from the current page).

Options that might be viable and need some discussion:

• quick&dirty: use the current mechanism (see above) that waits for some time but clearly match the events. It currently does not take into account the current state when clearing the timeout (quick navigation can break this with a sequence of "inline-popup" (due to beforeunload event, then speaker notes setup sends "popup" (canceling the timeout) and regular page sends either the "inline-popup" or "inline-closed" because of this toggling (this seems to trigger a toggle on page load if I see this correctly)

  comprehensive-rust/theme/speaker-notes.js

  Lines 90 to 93 in c05f0b6

  	// Create speaker notes.
  	notes.addEventListener("toggle", (event) => {
  	setState(notes.open ? "inline-open" : "inline-closed");
  	});

  (this is likely the least effort, a bit brittle depending on timing)
• A ping-pong style polling mechanism based on BroadcastChannels where the slides ask regularly (e.g. 500ms) "is there an active speaker notes window" (ping) and the speaker notes answer on that channel with a "yes" (pong). Once that pong is not sent, the state is set to inline-open (this seems like the most reliable way and might even recover if a speaker-notes window reappears (think Ctrl+Shift+T -> reopen accidentally closed tab).
• Add some mechanism to remove the beforeunload handler when a navigation event happens (more difficult but possible, create a function that is called when click is registered + use that function also when the current_page key of the localstorage changes. This could be brittle due to the many ways a navigation event could happen (not only click, but javascript or the back/prev functionality)

[djmitche]

I think that popout is a WindowProxy object, so it should be possible for the main page to poll that object.

[michael-kerscher]

Yes it is, but it is destroyed on navigation attempts like switching to a different slide as far as I understand. I also thought about using that to figure out if popout.closed is true

[michael-kerscher]

This should now be fixed with the latest PR. I wanted to avoid all the described race conditions and used the ping-pong style mechanism described above. Getting hold of the popout window in any way was not feasible because a navigation event (next slide, etc.) is destroying these window proxy objects.

With the new approach the notes can even recover itself when the connection was lost. Once a new window/tab with speaker notes is reopened, the broadcast channel is discovering the new window and notes are synced again.