-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Provide DOMRect implementation for IE11 #6666
Conversation
34691d5
to
ca73ef5
Compare
This may no longer be necessary with #6467 which removes the |
Thanks for letting me know. I'll hold onto this until that is merged. |
ca73ef5
to
14bc5d8
Compare
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 ); |
14bc5d8
to
b1e728f
Compare
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 |
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 |
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. |
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).
This would be reasonable too, also in the spirit of open source! |
I submitted a PR to add DOMRect to the Financial Times' polyfill-service: 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. |
Flagging as |
b1e728f
to
c70646e
Compare
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
lib/client-assets.php
Outdated
@@ -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(); }' . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
lib/client-assets.php
Outdated
@@ -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; }' . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Coding Guidelines: Needs whitespace between parentheses.
https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/#spacing
lib/client-assets.php
Outdated
'"DOMRect" in this && ( function( DOMRect ) {' . | ||
'try { return new DOMRect(); }' . | ||
'catch (e) { return false; }' . | ||
'}(this.DOMRect))' => 'wp-polyfill-domrect', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Coding Guidelines: Needs whitespace between parentheses.
https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/#spacing
lib/client-assets.php
Outdated
'wp-polyfill-domrect', | ||
gutenberg_url( 'build/polyfills/DOMRect.js' ), | ||
array(), | ||
filemtime( gutenberg_dir_path() . 'build/polyfills/DOMRect.js' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something we'd consider to publish as package(s)?
lib/client-assets.php
Outdated
'document.contains' => 'wp-polyfill-node-contains', | ||
'document.contains' => 'wp-polyfill-node-contains', | ||
'"DOMRect" in this && ( function( DOMRect ) {' . | ||
'try { return new DOMRect(); return true; }' . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return true;
is unreachable here. We don't want the return
on the new DOMRect();
statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦♂️ fixing...
16f57d7
to
72f1a02
Compare
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 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. |
Deleted our branch for this but keeping a reference with my fork: |
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?
build/
works.Checklist: