Opened 5 years ago
Closed 5 years ago
#49360 closed defect (bug) (fixed)
Scripts: Add polyfill for window.URL
Reported by: | aduth | Owned by: | aduth |
---|---|---|---|
Milestone: | 5.4 | Priority: | normal |
Severity: | normal | Version: | 5.4 |
Component: | Script Loader | Keywords: | has-patch |
Focuses: | javascript | Cc: |
Description
Related Gutenberg pull request: https://github.com/WordPress/gutenberg/pull/19871
Gutenberg code planned to be included as part of the upcoming WordPress 5.4 will make use of the `URL` global constructor. This is not supported in Internet Explorer 11, but a polyfill exists. Following the existing patterns of polyfills in the script loader, the accompanying patch proposes to add the polyfill for window.URL
. It is able to reuse the polyfill-library
dependency used for the Node#contains
polyfill, so no new dependencies are required.
Once the patch is applied, it can be verified by building the project and visiting the Posts > New Post editor page. Viewing page source, you should see a polyfill inline script when searching the window.URL
key phrase. You can further verify that when loading the editor in Internet Explorer 11, you should be able to see that window.URL
exists using the F12 Tools Console.
Attachments (5)
Change History (22)
This ticket was mentioned in PR #148 on WordPress/wordpress-develop by jorgefilipecosta.
5 years ago
#2
#3
@
5 years ago
- Keywords has-patch added
- Version set to trunk
I @aduth I verified the patch https://core.trac.wordpress.org/attachment/ticket/49360/49360-url-dom-rect-polyfills.diff worked on expected and that the polyfills were added on IE.
I verified the CI is green for this patch https://github.com/WordPress/wordpress-develop/pull/148.
I added a patch with the package-lock as it is on my machine (as I was the last one to update the file). Feel free to commit either my patch or yours.
#5
@
5 years ago
- Owner set to aduth
- Resolution set to fixed
- Status changed from new to closed
In 47238:
#6
@
5 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
I recently discovered that the URL polyfill from the polyfill-library
library is not spec-conformant, in a way which negatively impacts its usability for pending revisions to the block editor. Specifically, there were revisions to the implementation of the wp-url
script to detect URL validity by relying on thrown errors from the URL constructor, but this specific behavior is not implemented in this version of the polyfill.
More information:
- https://github.com/Financial-Times/polyfill-library/issues/462
- https://github.com/WordPress/gutenberg/pull/19871
- https://github.com/WordPress/gutenberg/pull/20172#issuecomment-584781181
The impact of this is that usage of wp.url.isURL
will return true
for invalid URLs in browsers where the polyfill is used. Specifically, this affects a few behaviors such as:
- Selecting a URL text in a paragraph and clicking "Add Link" would normally prepopulate that URL.
- Pasting a URL from an oEmbed source into an empty paragraph will normally insert an Embed block automatically.
These will not work correctly in Internet Explorer using the polyfill committed in r47238.
On my assessment, none of these behaviors are critical, but an alternative polyfill should be explored.
The following alternative implementations have been confirmed to have correct error throwing behavior:
However, unlike our existing polyfills, neither of these come pre-packaged as a browser-ready distributable.
The following options may be viable:
- Use a package which re-distributes one of the above as a browser-ready bundle (e.g. universal-url, core-js-bundle)
- Since sourcing/copying the polyfills already occurs as part of the Webpack build, the build configuration could be adapted to generate the browser-ready bundles on-demand. This would be a slightly larger task, considering that the current implementation of vendor scripts build only supports basic file copying.
This ticket was mentioned in Slack in #core by aduth. View the logs.
5 years ago
#9
@
5 years ago
There's another issue in r47238, which is that the test used to check whether the polyfill should be included is not accurate.
Specifically, this line:
https://github.com/WordPress/wordpress-develop/blob/d6352c54/src/wp-includes/script-loader.php#L129
This instructs the browser to load the polyfill if window.URL
evaluates to a falsey value. However, it was overlooked that while IE11 does not support the URL
constructor specifically, it does have the URL
global and supports some of its static members.
See: https://developer.mozilla.org/en-US/docs/Web/API/URL#Browser_compatibility
Currently, the polyfill is not being loaded at all in Internet Explorer. The impact of this is not much different than it is with the polyfill as implemented without the expected throwing behavior. Since the implementation of `isURL` already wraps itself in a try
/ catch
, the error will be caught. The main difference is that rather than always return true
from this function, it will always return false
. For most practical purposes, this is probably a better outcome than if the faulty polyfill implementation were loaded.
Thus, the test should be updated to one which specifically targets whether URL
can be used as a constructor.
Some options that might be available include:
- Try/catch on a test construction
( function() { try { new URL( 'http://w.org' ); return true; } catch ( e ) { return false; } } )()
- Minified:
!function(){try{new URL("http://w.org")}catch(e){return!1}}()
- Other detection of ability to use as constructor
URL.prototype
#10
follow-up:
↓ 11
@
5 years ago
Continuing to explore this, specifically on whether there are options to pull in an existing browser-ready solution:
universal-url
is written with CommonJSrequire
statements and does not include any browser-ready bundlewhatwg-url
implementation is quite large (90kb+ minified)core-js-bundle
is browser-ready, but is the entirecore-js
library, not something where individual parts can be used independently
It wouldn't be much effort to create or maintain a minimal wrapper which just creates a browser-ready bundle. I just now created one published as core-js-url-browser which could be used. The patch 49360-core-js-url.diff shows how this would be integrated much the same as other polyfills without too much trouble. I've confirmed this works effectively in Internet Explorer.
But as I look more at the existing packages Webpack configuration, I do feel that it would be worth exploring to extract a separate build configuration specific to vendor dependencies. I think this would simplify things greatly, allowing bundling one of the source polyfills, and incorporating minification by leveraging built-in Webpack functionality. The patch vendor-webpack.diff is a partial implementation of what this could look like. It feels like a larger task than we probably want to take on for the simple fix of the polyfill, but I think it would be a worthwhile improvement to vendor-script builds in general.
#11
in reply to:
↑ 10
@
5 years ago
Replying to aduth:
But as I look more at the existing packages Webpack configuration, I do feel that it would be worth exploring to extract a separate build configuration specific to vendor dependencies. I think this would simplify things greatly, allowing bundling one of the source polyfills, and incorporating minification by leveraging built-in Webpack functionality. The patch vendor-webpack.diff is a partial implementation of what this could look like. It feels like a larger task than we probably want to take on for the simple fix of the polyfill, but I think it would be a worthwhile improvement to vendor-script builds in general.
I created a separate ticket for this: #49423
This ticket was mentioned in Slack in #core by david.baumwald. View the logs.
5 years ago
This ticket was mentioned in Slack in #core-js by aduth. View the logs.
5 years ago
This ticket was mentioned in Slack in #core by david.baumwald. View the logs.
5 years ago
This ticket was mentioned in PR #175 on WordPress/wordpress-develop by jorgefilipecosta.
5 years ago
#15
#16
@
5 years ago
I tested this patch https://core.trac.wordpress.org/attachment/ticket/49360/49360-core-js-url.diff.
Before the patch on IE11:
window.wp.url.isURL('test');-> false
window.wp.url.isURL('http://test.com');-> false
After the patch on IE11:
window.wp.url.isURL('test');-> false
window.wp.url.isURL('http://test.com');-> true
It seems the patch fixes the problem.
While not strictly related to the original ticket purpose, it was brought to my attention that there is another feature which requires polyfilling in the Gutenberg changes coming with WordPress 5.4.0. Notably, a few of the editor components make use of
window.DOMRect
, which is not supported in Internet Explorer or the pre-Chromium Microsoft Edge:https://developer.mozilla.org/en-US/docs/Web/API/DOMRect#Browser_compatibility
There is a corresponding Gutenberg pull request at: https://github.com/WordPress/gutenberg/pull/20110
The same polyfill library used for
Node#contains
andURL
also includes a polyfill forDOMRect
. However, it was added in a version later than the one currently used in WordPress. Thus, it requires bumping the dependency.The patch at 49360-url-dom-rect-polyfills.diff includes a combination of the
URL
andDOMRect
polyfills, both sourced frompolyfill-library
. It also bumps thepolyfill-library
dependency.NOTE: The patch does not include expected changes to
package-lock.json
, since these may be more prone to conflicts in applying the patch, and anecdotally I had observed some unexpected and unrelated changes when runningnpm install
(possibly due to differing versions of NPM in use by contributors).