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

Broken URL polyfill #9358

Closed
rob-murray opened this issue Jan 12, 2018 · 5 comments
Closed

Broken URL polyfill #9358

rob-murray opened this issue Jan 12, 2018 · 5 comments

Comments

@rob-murray
Copy link

The URL polyfill has a couple of issues:

  1. The check for native URL API is incorrect - it fails on Safari >9 when this has a perfectly good native implementation
  2. The polyfill is incomplete and missing (afaict) searchParams

I use pdf.js in an application and the URL web api in various places, on Safari this is broken because the polyfill from this library is used instead of native code (Safari >9 has full implementation i believe) or the other polyfill I include (this fails the check used in this library incorrectly).

Configuration:

  • Web browser and its version: Safari >9
  • Operating system and its version: Anything
  • PDF.js version: latest (v1.8.188)
  • Is a browser extension: No

Steps to reproduce the problem:
1.
2.

What is the expected behavior?

If a browser does not have native URL api support then an up to date polyfill is used. If a browser does have native URL api support then the polyfill is not used.

What went wrong?

A browser with native URL api support was forced to use the broken URL polyfill from pdf.js

Link to a viewer (if hosted on a site other than mozilla.github.io/pdf.js or as Firefox/Chrome extension):

n/a

@Snuffleupagus
Copy link
Collaborator

The polyfill is incomplete and missing (afaict) searchParams

For reference, this part is already tracked in issue #8726.

@rob-murray
Copy link
Author

Sorry, I missed that one when searching for existing issues... feel free to close this then if its already got an open issue.

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Jan 12, 2018

Sorry, I missed that one when searching for existing issues... feel free to close this then if its already got an open issue.

No worries; #8726 only seem to concern the searchParams issue, but the first part of the current issue would appear separate and thus valid on its own:

The check for native URL API is incorrect - it fails on Safari >9 when this has a perfectly good native implementation

The following checks are used to determine if a browser has native URL support.

// Polyfill from https://github.com/Polymer/URL
/* Any copyright is dedicated to the Public Domain.
* http://creativecommons.org/publicdomain/zero/1.0/ */
(function checkURLConstructor() {
// feature detect for URL constructor
var hasWorkingUrl = false;
try {
if (typeof URL === 'function' &&
typeof URL.prototype === 'object' &&
('origin' in URL.prototype)) {
var u = new URL('b', 'http://a');
u.pathname = 'c%20d';
hasWorkingUrl = u.href === 'http://a/c%20d';
}
} catch (e) { }
if (hasWorkingUrl) {
return;
}

@rob-murray
Copy link
Author

rob-murray commented Jan 12, 2018

Safari seems to encode the % so u.href above is http://a/c%2520d, as a result that check is false - I don't know why Safari does this and if it is actually incorrect or even if it matters. The implementation of the polyfill in this library evaluates this check to be true, as does chrome, firefox, etc.

// safari
u = new URL('b', 'http://a'); 
u.pathname = 'c%20d'; 
u.href // "http://a/c%2520d"

// chrome
u = new URL('b', 'http://a'); 
u.pathname = 'c%20d'; 
u.href // "http://a/c%20d"

@timvandermeij
Copy link
Contributor

Fixed by #9868.

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

No branches or pull requests

3 participants