Provide DOMRect implementation for IE11 #6666

brandonpayton · 2018-05-09T19:07:02Z

Description

This PR creates a space for maintaining our own polyfills and uses it to provide a DOMRect polyfill. In the spirit of open source, this same polyfill has been submitted to the Financial Times' polyfill repository, but we need to maintain a local copy until it is merged and can be included as a vendor script.

Fixes #7036.

How has this been tested?

Ran unit tests.
Loaded Gutenberg in IE11 and noted that there is no longer a DOMRect-related error when clicking in a paragraph block to focus.
Loaded Gutenberg in Chrome, clicked in a paragraph block to focus, noted no error, and observed what appeared to be normal behavior.
Ran the build in Windows and macOS and confirmed polyfill copying to build/ works.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

aduth · 2018-05-09T20:19:59Z

This may no longer be necessary with #6467 which removes the DOMRect instantiation.

brandonpayton · 2018-05-09T20:34:14Z

Thanks for letting me know. I'll hold onto this until that is merged.

SymbolicallyMe · 2018-06-01T18:17:48Z

TinyMCE provides methods that contain various tools for rect/position calculation.

Reference: https://www.tinymce.com/docs/api/tinymce.geom/tinymce.geom.rect/

Example:

import tinymce from 'tinymce';

// Create a new DOMRect
return tinymce.geom.Rect.create( x, y, width, height );

brandonpayton · 2018-06-01T21:25:18Z

Hi @SymbolicallyMe, I didn't know that, and it's good to know. For this PR, I just want a simple fix to stop an error in IE11. I was hoping that TinyMCE's Rect had the same interface as DOMRect, but that doesn't appear to be the case.

@aduth, despite our recent conversation about maintaining these sorts of things, what do you think of adding this as a stopgap while we await #6467? It's an implementation detail that could be removed later and seems better than IE11 throwing an error on new DOMRect. EDIT: Actually, it could be worse with a faulty DOMRect, but I think this implementation is good.

aduth · 2018-06-06T20:03:57Z

I'd be open to it, but do you think we could do it in a pattern more like what was used in #7033 to polyfill Node#contains ?

brandonpayton · 2018-06-08T06:09:17Z

I'd prefer that but haven't found anything that is consumable à la carte via our polyfill mechanism. Actually, the only one I've found is also incorrect because it doesn't properly handle negative widths and heights. :) It's possible I'm not great at searching, but I have tried.

Honestly, this is simple enough, and I am frustrated by the absence of such a polyfill. Maybe I should just take a little time and submit it to FT's polyfill-service. We don't really want to own it, and it's easy to consume their polyfills via unpkg.

aduth · 2018-06-11T14:28:47Z

If there are no other options and the implementation is simple enough (like the one included already in these changes), it seems fine to maintain ourselves, at least near-term, in a manner for inclusion aligned with what we're doing with other polyfills (i.e. separate script enqueue).

Maybe I should just take a little time and submit it to FT's polyfill-service.

This would be reasonable too, also in the spirit of open source!

brandonpayton · 2018-06-16T09:48:28Z

I submitted a PR to add DOMRect to the Financial Times' polyfill-service:
polyfillpolyfill/polyfill-service#1732

It took longer than I expected, but I am now more comfortable with their approach and will be able to contribute more easily in the future.

danielbachhuber · 2018-06-27T15:31:12Z

Flagging as [Status] Blocked per waiting on upstream PR merge.

brandonpayton · 2018-07-13T04:08:40Z

I updated this PR to use a local version of the polyfill submitted to the FT polyfill service. Since we don't control whether or when contributions are accepted and published, it makes sense that we need a place to maintain polyfills locally, and this PR adds one.

@aduth, are you up for taking another look at this?

aduth

I'm very understanding of the busy-ness factor, but it's not very inspiring to have a lack of even simple acknowledgement from the maintainers in over a month. I agree we should implement here. If we continue to have issues, we could consider publishing as a module proper as well.

Noted a few changes to consider.

aduth · 2018-07-16T15:28:43Z

lib/client-assets.php

@@ -186,6 +193,10 @@ function gutenberg_register_scripts_and_styles() {
 		'wp-dom',
 		gutenberg_get_script_polyfill( array(
 			'document.contains' => 'wp-polyfill-node-contains',
+			'"DOMRect" in this && ( function( DOMRect ) {' .
+				'try { return new DOMRect(); }' .


This relies on gutenberg_get_script_polyfill interpreting the result of the test as truthy, not explicitly true, since this will return an instance of DOMRect. While it may work as-is, I don't think it's a very strong guarantee. I might suggest simply changing this to new DOMRect(); return true;

aduth · 2018-07-16T15:29:35Z

lib/client-assets.php

@@ -186,6 +193,10 @@ function gutenberg_register_scripts_and_styles() {
 		'wp-dom',
 		gutenberg_get_script_polyfill( array(
 			'document.contains' => 'wp-polyfill-node-contains',
+			'"DOMRect" in this && ( function( DOMRect ) {' .
+				'try { return new DOMRect(); }' .
+				'catch (e) { return false; }' .


Nit: Coding Guidelines: Needs whitespace between parentheses.

https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/#spacing

aduth · 2018-07-16T15:29:39Z

lib/client-assets.php

+			'"DOMRect" in this && ( function( DOMRect ) {' .
+				'try { return new DOMRect(); }' .
+				'catch (e) { return false; }' .
+			'}(this.DOMRect))'  => 'wp-polyfill-domrect',


Nit: Coding Guidelines: Needs whitespace between parentheses.

https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/#spacing

aduth · 2018-07-16T15:31:01Z

lib/client-assets.php

+		'wp-polyfill-domrect',
+		gutenberg_url( 'build/polyfills/DOMRect.js' ),
+		array(),
+		filemtime( gutenberg_dir_path() . 'build/polyfills/DOMRect.js' ),


For consistency and to avoid potential issues with OS-specific treatment of capitalization in file names, I might suggest we call this file dom-rect.js instead.

brandonpayton · 2018-07-16T19:28:07Z

Thanks for taking a look. I made the updates you suggested.

Yeah, the response time has not been great on that PR. It may be because DOMRect is not an exciting addition, but some response would be good.

aduth

Is this something we'd consider to publish as package(s)?

aduth · 2018-07-23T18:08:06Z

lib/client-assets.php

-			'document.contains' => 'wp-polyfill-node-contains',
+			'document.contains'   => 'wp-polyfill-node-contains',
+			'"DOMRect" in this && ( function( DOMRect ) {' .
+				'try { return new DOMRect(); return true; }' .


return true; is unreachable here. We don't want the return on the new DOMRect(); statement.

🤦‍♂️ fixing...

brandonpayton · 2018-07-24T15:17:20Z

Is this something we'd consider to publish as package(s)?

Given the current lack of response to the FT polyfill-service DOMRect PR, providing our own polyfill packages seems like a reasonable idea.

For now though, it looks like we no longer use the DOMRect constructor with new DOMRect(), so this PR is unnecessary.

I'm closing this, but if you think there's value in keeping it going, let me know. I'm up for converting to a polyfill package. Thanks for your time and attention on this.

brandonpayton · 2018-07-24T15:19:54Z

Deleted our branch for this but keeping a reference with my fork:
https://github.com/brandonpayton/gutenberg/tree/fix/ie11-missing-DOMRect

brandonpayton force-pushed the fix/ie11-missing-DOMRect branch from 34691d5 to ca73ef5 Compare May 9, 2018 19:08

brandonpayton requested a review from youknowriad May 9, 2018 19:10

brandonpayton force-pushed the fix/ie11-missing-DOMRect branch from ca73ef5 to 14bc5d8 Compare May 9, 2018 20:36

brandonpayton force-pushed the fix/ie11-missing-DOMRect branch from 14bc5d8 to b1e728f Compare June 1, 2018 21:16

SymbolicallyMe mentioned this pull request Jun 4, 2018

DOMRect doesn't work in some browsers #7036

Closed

danielbachhuber added the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Jun 27, 2018

brandonpayton force-pushed the fix/ie11-missing-DOMRect branch from b1e728f to c70646e Compare July 13, 2018 03:32

brandonpayton removed the [Status] Blocked Used to indicate that a current effort isn't able to move forward label Jul 13, 2018

brandonpayton self-assigned this Jul 13, 2018

aduth reviewed Jul 16, 2018

View reviewed changes

aduth reviewed Jul 23, 2018

View reviewed changes

brandonpayton added 8 commits July 24, 2018 07:34

Add local DOMRect polyfill

5f556ff

Include local polyfills with build

1fa859d

Add README explaining purpose of polyfills dir

6fd4e8c

Use local DOMRect polyfill

4b91746

Explain why require is used

3fe4314

Fix PHPCS error

20652bd

Update in response to review

5be6726

Remove return mistakenly left in polyfill test

72f1a02

brandonpayton force-pushed the fix/ie11-missing-DOMRect branch from 16f57d7 to 72f1a02 Compare July 24, 2018 14:44

brandonpayton closed this Jul 24, 2018

brandonpayton deleted the fix/ie11-missing-DOMRect branch July 24, 2018 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide DOMRect implementation for IE11 #6666

Provide DOMRect implementation for IE11 #6666

brandonpayton commented May 9, 2018 •

edited

Loading

aduth commented May 9, 2018

brandonpayton commented May 9, 2018

SymbolicallyMe commented Jun 1, 2018 •

edited

Loading

brandonpayton commented Jun 1, 2018 •

edited

Loading

aduth commented Jun 6, 2018

brandonpayton commented Jun 8, 2018

aduth commented Jun 11, 2018

brandonpayton commented Jun 16, 2018

danielbachhuber commented Jun 27, 2018

brandonpayton commented Jul 13, 2018

aduth left a comment

aduth Jul 16, 2018

aduth Jul 16, 2018

aduth Jul 16, 2018

aduth Jul 16, 2018

brandonpayton commented Jul 16, 2018

aduth left a comment

aduth Jul 23, 2018

brandonpayton Jul 24, 2018

brandonpayton commented Jul 24, 2018

brandonpayton commented Jul 24, 2018

Provide DOMRect implementation for IE11 #6666

Provide DOMRect implementation for IE11 #6666

Conversation

brandonpayton commented May 9, 2018 • edited Loading

Description

How has this been tested?

Checklist:

aduth commented May 9, 2018

brandonpayton commented May 9, 2018

SymbolicallyMe commented Jun 1, 2018 • edited Loading

brandonpayton commented Jun 1, 2018 • edited Loading

aduth commented Jun 6, 2018

brandonpayton commented Jun 8, 2018

aduth commented Jun 11, 2018

brandonpayton commented Jun 16, 2018

danielbachhuber commented Jun 27, 2018

brandonpayton commented Jul 13, 2018

aduth left a comment

Choose a reason for hiding this comment

aduth Jul 16, 2018

Choose a reason for hiding this comment

aduth Jul 16, 2018

Choose a reason for hiding this comment

aduth Jul 16, 2018

Choose a reason for hiding this comment

aduth Jul 16, 2018

Choose a reason for hiding this comment

brandonpayton commented Jul 16, 2018

aduth left a comment

Choose a reason for hiding this comment

aduth Jul 23, 2018

Choose a reason for hiding this comment

brandonpayton Jul 24, 2018

Choose a reason for hiding this comment

brandonpayton commented Jul 24, 2018

brandonpayton commented Jul 24, 2018

brandonpayton commented May 9, 2018 •

edited

Loading

SymbolicallyMe commented Jun 1, 2018 •

edited

Loading

brandonpayton commented Jun 1, 2018 •

edited

Loading