Skip to content

Implement backward and forward navigation options to iframed window #181

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jan 23, 2023

Conversation

brandonmcconnell
Copy link
Contributor

@brandonmcconnell brandonmcconnell commented Jan 11, 2023

Overview of changes

This PR adds backward and forward navigation options to iframed window. Because this sort of behavior isn't normally possible within the natural constraints of the History API since the history of the iframed window and that of its containing parent window operate together, I opted to manually track the history of the iframed window in two arrays— history_bwd and history_fwd.

Implementation/test cases & detailed steps for each

When any path is manually entered into the iframe's simulated address field… (expand to see details)
  • the current path is appended to history_bwd
  • history_fwd is reset
  • the new path is naturally set to the entered path (current functionality)
When the "back" button is clicked… (expand to see details)
  • the current path is prepended to history_fwd
  • the last item in history_bwd is sliced off
  • the sliced item (the previous path) replaces the current path
When the "forward" button is clicked… (expand to see details)
  • the current path is appended to history_bwd
  • the first item in history_fwd is sliced off
  • the sliced item (the next path) replaces the current path
When any link within the iframe is clicked, it is picked up by handle_message which then falls back to using the nav_to function for handling and tracking the history accordingly, effectively… (expand to see details)
  • the current path is appended to history_bwd
  • history_fwd is reset
  • the new path is naturally set to the entered path (current functionality)

Notes

  • The icons I used are custom ones I threw together in Illustrator, so feel free to use them without any risk of copyright violations, though I understand there are probably plenty of reasons to switch them, even just to stay on brand.
  • I noticed after my initial successful tests that while the back/forward functionality worked as expected, the history persisted across tutorials, so I added an additional reset_history function that resets the history, which I trigger in the try clause within afterNavigate.

Testing

This can be tested/demoed effectively using any of the routing examples:

Screen recording (UPDATED)

✅ Updated to include the flash-of-white fix implemented in commit 644f5e3 (expand for details on fix)


As of commit 644f5e3, the flash of white is now resolved by hiding the iframe (display: none, in addition to the existing parentNode?.removeChild(iframe) and parentNode?.appendChild(iframe)) until the iframe content has loaded, using an event listener set attached to iframe within the onMount and destroyed accordingly.

Screen.Recording.2023-01-11.at.1.09.05.PM.mov

@vercel
Copy link

vercel bot commented Jan 11, 2023

@brandonmcconnell is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Jan 11, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
learn-svelte-dev ✅ Ready (Inspect) Visit Preview Jan 11, 2023 at 9:10PM (UTC)

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thank you! I swapped out the two svgs for one and adjusted the colors so it works for light mode, too.

@Rich-Harris
Copy link
Member

Sorry, I dropped the ball here, meant to review this PR sooner. I think we should revert it, so I've opened #187. The back/forward buttons in this PR cause the iframe to be reloaded, which gives a very misleading impression of how history navigation works in SvelteKit. It's better to not have history UI than to have UI that causes full page reloads

@brandonmcconnell
Copy link
Contributor Author

brandonmcconnell commented Jan 23, 2023

@Rich-Harris I might be wrong here, but I think the reload is caused by the iframe being removed and re-added to the screen, alongside the bwd/fwd history changes.

According to the comment inside the set_iframe_src function…

/** @param {string} path */
function set_iframe_src(src) {
  // removing the iframe from the document allows us to
  // change the src without adding a history entry, which
  // would make back/forward traversal very annoying
  const parentNode = /** @type {HTMLElement} */ (iframe.parentNode);
  iframe.classList.remove('loaded');
  parentNode?.removeChild(iframe);
  iframe.src = src;
  parentNode?.appendChild(iframe);
}

…removing the iframe altogether and re-injecting it to the page is supposed to "change the src without adding a history entry" though I do not see that actually working in Chrome, even before my PR. Does that work in other browsers? As I navigate around one iframe, I still have to click the browser's back button repeatedly to traverse through every navigation from the iframe.

I can confirm that removing the set_iframe_src function and swapping its usage for iframe.src = resolves this issue, though again, I can't speak to whether any browsers were actually benefitting from set_iframe_src.

Let's go ahead and revert this PR for now, and either re-open this PR or I can create a new PR that builds on this one and addresses those concerns.

cc @dummdidumm

@brandonmcconnell
Copy link
Contributor Author

@Rich-Harris @dummdidumm One idea here which might remove much of the complexity around this is to sync the iframe history/URL state with the browser's so that as the iframe's URL changes, there could be URL param in the browser like frame_src=${path} so that clicking back in the browser or the iframe essentially trigger the same action and go back one step whether in the iframe or the browser depending on the user's actions. Of course, it would be better to just completely isolate the iframe's history apart from the browser's, but in all my testing and research, there doesn't appear to be a way to achieve that, including the "removing/adding the iframe node" hack.

I'm not sure if/how/where/when that work(ed), and I'm honestly not criticizing it; it may very well work in some places. It just hasn't worked in my testing, which granted was limited to Chrome.

Rich-Harris added a commit that referenced this pull request Jan 24, 2023
…window" (#187)

* Revert "feat: implement backward/forward navigation options to iframed window (#181)"

This reverts commit 8ff785c.

* finish merge

* fix

* tidy up

* tidy up

* oops
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants