Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#49360 closed defect (bug) (fixed)

Scripts: Add polyfill for window.URL

Reported by: aduth's profile aduth Owned by: aduth's profile 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)

url-polyfill.diff (4.6 KB) - added by aduth 5 years ago.
49360-url-dom-rect-polyfills.diff (5.4 KB) - added by aduth 5 years ago.
49360-url-dom-rect-polyfills-with-package-lock.diff (52.9 KB) - added by jorgefilipecosta 5 years ago.
49360-core-js-url.diff (8.9 KB) - added by aduth 5 years ago.
vendor-webpack.diff (2.0 KB) - added by aduth 5 years ago.

Download all attachments as: .zip

Change History (22)

@aduth
5 years ago

#1 @aduth
5 years ago

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 and URL also includes a polyfill for DOMRect. 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 and DOMRect polyfills, both sourced from polyfill-library. It also bumps the polyfill-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 running npm install (possibly due to differing versions of NPM in use by contributors).

This ticket was mentioned in PR #148 on WordPress/wordpress-develop by jorgefilipecosta.


5 years ago
#2

#3 @jorgefilipecosta
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.

#4 @youknowriad
5 years ago

  • Milestone changed from Awaiting Review to 5.4

#5 @aduth
5 years ago

  • Owner set to aduth
  • Resolution set to fixed
  • Status changed from new to closed

In 47238:

Script Loader: Add polyfill for window.URL, window.DOMRect.

Pending block editor revisions for WordPress 5.4 will make use of window.URL and window.DOMRect. These are not available in Internet Explorer (or pre-Chromium Edge for DOMRect) and must be polyfilled to avoid script errors.

The changes make use of the existing polyfill pattern, and existing polyfill-library dependency. The dependency is bumped to the latest version, since the previous version did not include the DOMRect polyfill.

Props jorgefilipecosta.
Fixes #49360.

#6 @aduth
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:

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

#8 @jorgefilipecosta
5 years ago

  • Type changed from enhancement to defect (bug)

#9 @aduth
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

@aduth
5 years ago

#10 follow-up: @aduth
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 CommonJS require statements and does not include any browser-ready bundle
  • whatwg-url implementation is quite large (90kb+ minified)
  • core-js-bundle is browser-ready, but is the entire core-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 @aduth
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

#16 @jorgefilipecosta
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.

#17 @jorgefilipecosta
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 47416:

Scripts: Use core-js url as polyfill for window.URL.

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.
There was another issue in r47238, which is that the test used to check whether the polyfill should be included is not accurate. This commit uses a different check and fixes the issue.

Props aduth.
Fixes: #49360.

Note: See TracTickets for help on using tickets.