Reset state to inline-open when speaker note popup is closed

[elliotbrandwein]

Partially addresses #182.

[mgeisler]

Hi @elliotbrandwein, thanks for the PR!

I tried a few things to get this working back when I implemented it: and I ran into a lot of weird restrictions on when one can use the onbeforeunload event...

Your solution works okay until you navigate away from the page that opened the popup window. The two windows are synchronized, but after navigating forward or backward in either window, the unload event no longer triggers and so the state isn't reset.

My understanding is that browsers basically refuse to implement this according to spec because of spammy popup windows.

[elliotbrandwein]

Hi @elliotbrandwein, thanks for the PR!

I tried a few things to get this working back when I implemented it: and I ran into a lot of weird restrictions on when one can use the onbeforeunload event...

Your solution works okay until you navigate away from the page that opened the popup window. The two windows are synchronized, but after navigating forward or backward in either window, the unload event no longer triggers and so the state isn't reset.

My understanding is that browsers basically refuse to implement this according to spec because of spammy popup windows.

I see. at this point i might be dipping into #1232 but i can see the fix for this issue being that i build a speaker notes state system that keeps track of the page of the speaker notes & if the popup is active or not. If you dont mind feature creep I can implement that into this pr

[mgeisler]

If you dont mind feature creep I can implement that into this pr

That would be wonderful actually! I've been waiting for someone with enough JavaScript skills to come and help out 😅

[moaminsharifi]

Do you need help on this?
@elliotbrandwein @mgeisler

[mgeisler]

Hey @moaminsharifi, I would be happy to get help here 😄

[moaminsharifi]

After checking JS window documentation, My idea due to current system which is mostly based on pure js and not any webframe work is make it something like SPA-ish:

1- Disable navigation links' default behavior (using the css selector of a.mobile-nav-chapters)

2- when user clicks on navigation links , we use fetch module of browser to get all html content and render it

• We need to render the sidebar
• We need to render the content part
• We need to check if popup open then render that part ( and if user click in popup then update main window)

https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API
https://www.w3schools.com/jsref/met_doc_write.asp

3- check URL in browser with JS.

https://stackoverflow.com/questions/824349/how-do-i-modify-the-url-without-reloading-the-page

I want to know I much I can modify in js part? like adding new js lib is ok? @mgeisler

[elliotbrandwein]

After checking JS window documentation, My idea due to current system which is mostly based on pure js and not any webframe work is make it something like SPA-ish:

1- Disable navigation links' default behavior (using the css selector of a.mobile-nav-chapters)

2- when user clicks on navigation links , we use fetch module of browser to get all html content and render it

• We need to render the sidebar
• We need to render the content part
• We need to check if popup open then render that part ( and if user click in popup then update main window)

https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API
https://www.w3schools.com/jsref/met_doc_write.asp

3- check URL in browser with JS.

https://stackoverflow.com/questions/824349/how-do-i-modify-the-url-without-reloading-the-page

I want to know I much I can modify in js part? like adding new js lib is ok? @mgeisler

That definitely works to solve the ticket here but for the issue at hand of the main tab not getting the signal that the notes tab closed we still need to solve for.

My thinking is that since onbeforeunload isn't reliable we use localStorage and a storage listener to trigger the logic for when the popup tab is open/closed.

That being this fix would have to wait until after you create the new tab lib, or it would need to be redone as part of your cl.

[mgeisler]

I want to know I much I can modify in js part? like adding new js lib is ok? @mgeisler

Hi @moaminsharifi, I would be fine with that — we don't have any particular limitations for for this, apart that the result should be maintainable and load fast, etc 🙂

My thinking is that since onbeforeunload isn't reliable we use localStorage and a storage listener to trigger the logic for when the popup tab is open/closed.

I might have tried this, but I'm not sure! It's nearly a year since I played around with this, so I'm really appreciating someone new taking a look at this with fresh eyes!

[moaminsharifi]

I want to know I much I can modify in js part? like adding new js lib is ok? @mgeisler

Hi @moaminsharifi, I would be fine with that — we don't have any particular limitations for for this, apart that the result should be maintainable and load fast, etc 🙂

My thinking is that since onbeforeunload isn't reliable we use localStorage and a storage listener to trigger the logic for when the popup tab is open/closed.

I might have tried this, but I'm not sure! It's nearly a year since I played around with this, so I'm really appreciating someone new taking a look at this with fresh eyes!

It will be my pleasure to contribute in whatever way I can to the improvement of this book. I have other ideas to make this part better for both teachers and students besides my comments here.

@mgeisler I'll work on it in my local branch https://github.com/moaminsharifi/comprehensive-rust/tree/fixture-pop-up-close-issue

[mgeisler]

@elliotbrandwein, since I think the change here does improve the situation in some cases, we can merge the PR if you like? Then you and @moaminsharifi could discuss a better long-term solution (if one exists 😄). That way we can stop discussing on the PR and collect our thoughts in concrete issues instead.

[elliotbrandwein]

updated the format of the pr so it should (hopefully) pass the pre submits and get merged

[mgeisler]

updated the format of the pr so it should (hopefully) pass the pre submits and get merged

Thanks @elliotbrandwein! I hope you and @moaminsharifi can come up with some good solution for the general problem of syncing the state.