Skip to content
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

Fix for Issue 8726 (Safari's usage of searchParams) #9197

Closed
wants to merge 2 commits into from
Closed

Fix for Issue 8726 (Safari's usage of searchParams) #9197

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 27, 2017

This commit addresses Issue #8726

Two fixes were required to satisfy Safari compatibility for searchParams().

  1. The usage of replace() in the JURL's constructor resulted in undefined being returned from the url parameter that Safari passes. This is because Safari passes a non-String object to the JURL constructor when performing new URL(document.location).searchParams.get("x") - which lacks the replace() method. Thus when doing replace() on it, url would become undefined and result in redirecting the browser to the URL with the supplied regular expression (i.e http://localhost:8888/^[%20/t/r/n/f]+|[%20/t/r/n/f]+$/g).

The fix applied for this was to make url a String object if running on Safari browsers. This allows replace() to succeed as the method exists for the String object.

  1. When the native URL object is replaced with the JURL object on the global scope, any future URL instances will result in a JURL instance. This backfires on Safari as it does not seem to fallback to the native URL object when it is unable to find a method within JURL (i.e searchParams). Other modern browsers (i.e FireFox, Chrome) seem to correctly fallback to the native URL object when attempting to look for a method not present on the JURL object.

The fix applied for this was to make a get searchParams function for the JURL object. It creates a URLSearchParams object with the appropriate search parameters and returns it to the caller.

// Issue 8726: Safari needs url to be a String object.
// Otherwise, replace() below will produce a blank string.
if (isSafari) {
url = String(url);
Copy link
Collaborator

@Snuffleupagus Snuffleupagus Nov 28, 2017

Choose a reason for hiding this comment

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

For reference: This part looks similar to PR #8727, which was closed based on #8727 (comment).
@yurydelendik What were the problems mentioned there?


Also, please see https://github.com/mozilla/pdf.js/wiki/Squashing-Commits.


// Issue 8726: Safari does not fallback to the original URL object.
get searchParams() {
return new URLSearchParams(this.search);
Copy link
Collaborator

@Snuffleupagus Snuffleupagus Nov 28, 2017

Choose a reason for hiding this comment

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

Also, given https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams/URLSearchParams#Browser_compatibility, I'm not sure how this helps in general given that it won't work in other browsers that rely on the URL polyfill (e.g. IE11).

@timvandermeij
Copy link
Contributor

timvandermeij commented Dec 30, 2017

I'm going to close this pull request. The comment above indicate why this won't work in general. The polyfill must keep working for IE. The issue will remain open.

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

Successfully merging this pull request may close these issues.

2 participants