files_panel dynamic navigation fixes #2111
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
October 16, 2023 12:50
When text is included as a string in an inline JavaScript expression, it must be escaped. Currently, the upload form does not permit creating files or directories with names containing quotes or control characters, but the templates should nonetheless protect against that possibility.
In the files section of a preview or published project, clicking a
link to a subdirectory or parent directory should behave like a normal
link:
- The browser's window.location should be updated, so that you can
bookmark it.
- The former location should be saved in the browser's history stack,
so that you can navigate with Back and Forward buttons.
Doing these things requires calling history.pushState when we navigate
to a new location, and setting window.onpopstate to a handler function
that restores the former location.
Moreover, it is cleaner and safer to avoid using inline scripts.
When a directory link is clicked, we will asynchronously load the
directory contents; once they're loaded, call pushState to update the
page URL and then parse the new directory HTML. (pushState must be
called first so that relative links are handled correctly.)
When the back button is clicked ("onpopstate" function is called), we
likewise load the previous HTML directory contents, but in this case
call replaceState instead of pushState (again, we need to update the
page URL before parsing the HTML.) (You might think this replaceState
is unnecessary, but because the HTML is loaded asynchronously, there
could be multiple attempts in flight to change the page location, so
it's better to keep the DOM and page URL and history state all in sync
with each other.)
To avoid overly complicated transition logic, the existing
preview_files_panel and published_files_panel views are kept as-is.
Only when somebody loads the new project_preview page or
published_project page will they use the new script, and only when
using the new script (which adds a "v=2" query parameter) will
preview_files_panel and published_files_panel return the new
inline-script-free responses.
Eventually, preview_files_panel and published_files_panel can be
changed to return an error if the v=2 parameter is missing, and the
old files_panel.html can be removed.
I should mention that |
|
looks great benjamin, thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When you go to the "Files" section of a preview or published project, and click on a link to a subdirectory (with javascript enabled), then instead of reloading the entire page, a script will load just the files section for that directory.
I think this is generally a convenient thing, but there are a bunch of significant problems with the current implementation:
After you navigate to a subdirectory in this way, the page URL is not modified (so you can't bookmark it.)
After you navigate to a subdirectory, the back button doesn't take you back to where you were.
After you navigate to a file within a subdirectory, the back button might or might not take you back to where you were.
After you navigate to a subdirectory, other subdirectory links (if you right-click on one and say "copy link") are broken.
The current implementation has theoretical XSS vulnerabilities.
Inline scripts are a bad idea and should never be used.
The first three issues are variously fixed by calling
history.replaceStateorhistory.pushStatebefore loading the new HTML. The other issues are fixed by putting the logic in a separate, static, script file.For example, if you go to
https://localhost:8000/content/demoecg/10.5.24/#files-panel:If you click on "app", the page URL should change to
https://localhost:8000/content/demoecg/10.5.24/app/#files-panel.If you then right-click on "Parent Directory" and click "Copy Link", the copied text should be
https://localhost:8000/content/demoecg/10.5.24/#files-panel(nothttps://localhost:8000/content/demoecg/#files-panel.)If you then click on "12lead.pro", it should display the file; then, clicking the Back button should take you back to the "app" subdirectory (not to the toplevel directory of the project.)
If you click the Back button again, it should return you to the toplevel directory (not back to some other page.)
It would be quite complicated to make this change in a truly backward-compatible way (i.e., returning the correct URLs and loading the new script when somebody invokes the old
navigateDirfunction), and I thought this wasn't worth the effort. So this is set up as a parallel template; old clients can still use the old one. After a while the old template can be removed.