fix iframe.js breaking links with hash URLs #1115
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
oshi97
commented
Dec 9, 2022
| @@ -13,6 +13,8 @@ webpack-config.js | |||
| !config/global_config.json | |||
| !config/locale_config.json | |||
| !config/index.json | |||
| !config/index.ar.json | |||
There was a problem hiding this comment.
I deleted these pages to speed up the testing loop. When I added them back I noticed they were missing from the gitignore. They were probably being tracked with git add --force or something similar.
nmanu1
reviewed
Dec 12, 2022
|
|
||
| /** | ||
| * Reloads the iframe with a recalculated src URL. | ||
| * Does not reload the iframe if the URL has only changed its hash param. |
There was a problem hiding this comment.
so we'd still want regular query params to trigger a reload?
There was a problem hiding this comment.
We don't want to touch the preexisting behavior because we're not sure how it's being used or for what purpose it's there. This code was copy and pasted all at once from the AEB (answers experience builder created by Mcgin) and it doesn't seem possible to track that info down.
nmanu1
approved these changes
Dec 12, 2022
oshi97
added a commit
that referenced
this pull request
Dec 12, 2022
oshi97
added a commit
that referenced
this pull request
Dec 12, 2022
oshi97
added a commit
that referenced
this pull request
Dec 12, 2022
Merged
oshi97
added a commit
that referenced
this pull request
Dec 12, 2022
Previously, iframe.js would not work with links contains hash URLs, i.e. links like ```html <a id="skip-to-content-link" href="#main-content">Skip to content</a> ``` notice the URL starts with a hashtag. These links are different than normal HTML links, in that instead of redirecting to a new page, they will scroll the browser down to the element with the id matching the link. For the example above, it would scroll down to an element like ```html <div id="main-content">main content!</div> ``` However, iframe.js has an issue where, when one of these links are clicked, the iframe.js will reload and the parent frame's URL will be changed. This issue was caused by the following process: 1. iframe.js's has a window.popstate listener, which reloads the iframe whenever a popstate event was triggered (for instance by clicking on one of these links), 2. clicking a hash link will trigger this listener, reloading the iframe 3. reloading the iframe causes the inner iframe to send messages to the outer frame 4. these messages are received by iframe.js's onMessage listener, which then performs a history.replaceState (for certain types of messages) To avoid this situation, this PR updates the popstate listener to be a no-op if the new URL of the page is the same as the old one, ignoring any hash URLs J=TECHOPS-7401 TEST=manual ran the theme's test site, created a basic iframe implementation before this change, hash urls would have the broken behavior after the change, they would scroll down the page without reloading the iframe or changing the parent's URL
oshi97
added a commit
that referenced
this pull request
Dec 12, 2022
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.
Previously, iframe.js would not work with links contains hash URLs, i.e. links like
notice the URL starts with a hashtag. These links are different than normal HTML links,
in that instead of redirecting to a new page, they will scroll the browser down to the element
with the id matching the link. For the example above, it would scroll down to an element like
However, iframe.js has an issue where, when one of these links are clicked, the iframe.js will reload
and the parent frame's URL will be changed.
This issue was caused by the following process:
whenever a popstate event was triggered (for instance by clicking on one of these links),
To avoid this situation, this PR updates the popstate listener to be a no-op if the new URL of the page is the same as the old one, ignoring any hash URLs
J=TECHOPS-7401
TEST=manual
ran the theme's test site, created a basic iframe implementation
before this change, hash urls would have the broken behavior
after the change, they would scroll down the page without reloading the iframe or changing the parent's URL