Temporarily remove query params from viewer iframe on navigate #25181
Conversation
dreamofabear
force-pushed
the
safari-viewer-params
branch
from
4b579f6
to
fb27fb3
Compare
dreamofabear
changed the title
|
/to @zhouyx @jridgewell |
|
|
||
| // For blank_, restore query params after the new page opens. | ||
| if (target === '_blank') { | ||
| win.setTimeout(restoreQuery, 0); |
There was a problem hiding this comment.
Does it buy us anything? Also:
This uses a micro task for 0
There was a problem hiding this comment.
Explicitly the micro task. Do we need a macro task for this to work?
src/service/navigation.js
Outdated
|
|
||
| const restoreQuery = () => { | ||
| dev().info(TAG, 'Restored URL with query string:', original); | ||
| win.history.replaceState(null, '', original); |
There was a problem hiding this comment.
Depending on the order of timeouts, we may need to check that the URL is still the noQuery URL before replacing it.
There was a problem hiding this comment.
I'll check and dev().error if it doesn't match.
There was a problem hiding this comment.
Does this also restore query params that also got appended before navigation. For example the linker param?
There was a problem hiding this comment.
Linker appends a query param to the viewer iframe? I thought it decorates the outgoing link.
In any case, this is the last code that runs before navigation including any URL expansion etc.
There was a problem hiding this comment.
oops, nvm I was confused by all the urls....😅
LGTM
|
I noticed you put temporarily to the PR description. are there any long term solution be considered? |
|
This is the long-term solution. :) "Temporarily" is only because we quickly restore the query params after opening a new tab or back navigation. |
|
|
||
| // For blank_, restore query params after the new page opens. | ||
| if (target === '_blank') { | ||
| win.setTimeout(restoreQuery, 0); |
There was a problem hiding this comment.
Explicitly the micro task. Do we need a macro task for this to work?
|
@jridgewell Need bundle size approval please. :) |
…oject#25181) * Implement viewer query param removal experiment. * Dev error when removing unknown params. * Tweak error message. * Restore query params async or on pageshow. * Enable in canary. * Fix lint. * Check that iframe URL hasn't changed before restoration. * s/location/fromLocation.
Closes #25179.
This prevents document.referrer downgrade to eTLD+1, but valid query params will still be lost e.g.
foo.example/article?id=12345.Tested on Safari 13 desktop with UA emulation.