midzer / tobii

An accessible, open-source lightbox with no dependencies
https://midzer.github.io/tobii/demo/
MIT License
196 stars 20 forks source link

Fix exception when clicking the back button #117

Closed mspae closed 9 months ago

mspae commented 9 months ago

Hello, first of all thanks for this awesome lib!

This MR fixes a specific issue I've run into:

Reproduction

Cause

Finally...

IMHO I think this is a good fix since we don't need the call to window.history.back at this point anyway. The check for a falsy value handles both cases (the back button case and the "I just opened the popup and now want to close it" case)

Let me know if you agree or if you want something changed. Cheers.

midzer commented 9 months ago

Hi, welcome and thanks for this detailed PR!

Cannot reproduce in Chrom* and Firefox though. Which browser(s) is causing this Exception?

mspae commented 9 months ago

Ok I researched some more and I think the reason why I'm getting this is because the page uses window.location.hash/hashchange event which means it conflicts with the history API in this way. (duh)

Fortunately I can easily change the site to use the modern API and this should resolve the issue :crossed_fingers:

The proposed change might still be a good idea to make tobii more resililent for projects where switching APIs is not an option. Also maybe we could add a small info somewhere in the README about using the legacy browser API. (Let me know if I should add it to the MR)

PS: I was using Chrome Version 117.0.5938.88 (Official Build) (64-bit)

EDIT1: I think the above explanation is not what's going on. I think I have conflicting history API calls in my code. I'll update this post when I've found the issue.

EDIT2: Okay. So the cause of the issue was I was actually using pushState before but I called it with undefined as the first parameter. My IDE didn't complain (lib.dom.ts says it can be any) but as per spec this is wrong. Sorry for wasting your time :see_no_evil: