Provide cross-browser node containment checking

[brandonpayton]

Description

IE11's Node#contains implementation does not support checking whether text nodes are contained within an element, and Node#contains is used by at least the Autocomplete component. This PR adds a nodeContains function to the @wordpress/dom package to provide containment checking that works cross-browser.

Honestly, I'd prefer to add a polyfill so Node#contains just works throughout our code base, but with the breakdown of packages and dependencies, I'm not sure where and how it should be added. I see how we are including external polyfills in lib/client-assets.php but have not identified a good external polyfill for this.

I'm putting this out there because it is working, and we can convert it to a polyfill if desired.

This update is part of getting autocompletion working in IE11. It was originally part of #6667.

How has this been tested?

Unit tests.

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]

Honestly, I'd prefer to add a polyfill so Node#contains just works throughout our code base, but with the breakdown of packages and dependencies, I'm not sure where and how it should be added.

This is a good question, and one which we should address directly, as it impacts consistency of a few different things:

• We have a polyfill utility on the server, and its primary purpose as used today is to serve as a progressive enhancement for unsupporting / misbehaving browsers without impacting those using competent browsers.
  • Benefits here are: Less to send over the wire, less to maintain, more deferring to a standard spec-defined behavior as assumed to be present.
    • Because how else are we going to expect people to know to use nodeContains? See also: Use Lodash endsWith equivalent #2072 Components: Use Lodash's find in place of Array#find #3502 FIX: use lodash version of startsWith because IE does not support startsWith natively #3415 Plugin: Drop Array.prototype.find calls and replace with lodash #925
• We have an element-closest dependency which is pulled into various files where we use Node#closest to bring support for IE11
  • This has shown to be problematic, because (a) the code itself should probably not be responsible for ensuring the polyfill exists, (b) the same issue above of developer needing to know it exists, (c) each bundle is probably including its own copy, and (d) it stagnates and is often not removed if future refactoring drops usage.
    • Case in point, these imports in current master are totally unnecessary, as Element#closest exists nowhere in the files they're used.

      • gutenberg/blocks/api/raw-handling/index.js

        Line 6 in c93b778

        import 'element-closest';

      • gutenberg/editor/components/block-list/layout.js

        Line 14 in c93b778

        import 'element-closest';

      • gutenberg/editor/components/rich-text/index.js

        Line 16 in c93b778

        import 'element-closest';

• This proposed set of changes would be a new third pattern for handling browser incompatibilities

I think our progressive enhancement polyfill utility is the best path forward, because developers shouldn't have to think about this, they should use the browser APIs as the specification dictates it should behave. These are isolated and can be dropped easily when browser support requirements change.

But, as you noted, a challenge here is knowing how to configure the dependencies such that the polyfills are ensured to be loaded. In that sense, and given that these are true progressive enhancements which do nothing if a browser is detected to support a given feature, I'd say: Load it everywhere; every WordPress admin screen; for any features we ever use.

[aduth]

We should have a test:

it( 'returns true when passed node itself', () => {

...which I have a feeling will fail as current implemented. See also my concluding notes about us wanting to maintain this.

From specification:

The contains(other) method, when invoked, must return true if other is an inclusive descendant [...] An inclusive descendant is an object or one of its descendants.

https://dom.spec.whatwg.org/#dom-node-contains
https://dom.spec.whatwg.org/#concept-tree-inclusive-descendant

[brandonpayton]

Thanks for catching that! I added a test. Surprisingly, it does work due to using Node#contains if the node is an element. I don't love that this is IE-specific fix, but I was keeping my understanding of our browser support in mind and only know about IE's broken implementation.

See also my concluding notes about us wanting to maintain this.

It's possible I'm not reading well because it's later here, but I don't see direct commentary on wanting to maintain this. What am I missing?

I'd rather add an existing polyfill, but when I searched the NPM registry, all I found were implementations like nodeContains and nothing to patch in a working Node#contains. It wasn't an exhaustive search, and I'll look again in the morning.

[aduth]

Surprisingly, it does work due to using Node#contains if the node is an element.

There are some non-element Node#contains we should want to support, e.g.

const node = document.createTextNode( '' );
node.contains( node );
// true

document.contains( document );
// true

I added a test case for the latter, which fails at the moment.

It's possible I'm not reading well because it's later here, but I don't see direct commentary on wanting to maintain this. What am I missing?

Sorry, in retrospect it wasn't a direct commentary. Indirect in the sense that we're already discovering bugs, and if a stable normalization solution already exists and is well-vetted, we should opt for it instead than try to recreate it ourselves.

I'd rather add an existing polyfill, but when I searched the NPM registry, all I found were implementations like nodeContains and nothing to patch in a working Node#contains.

The one from Financial Times' polyfill.io looks promising:

https://github.com/Financial-Times/polyfill-service/blob/master/packages/polyfill-library/polyfills/Node/prototype/contains/polyfill.js

Typically it's used through the polyfill.io API (which is pretty neat in detecting, by user-agent, which features to polyfill), we could maybe reference directly by the link:

https://unpkg.com/polyfill-library@3.26.0-0/polyfills/Node/prototype/contains/polyfill.js

The polyfill condition looks quite simple too, just document.contains

https://unpkg.com/polyfill-library@3.26.0-0/polyfills/Node/prototype/contains/detect.js

[brandonpayton]

I added a test case for the latter, which fails at the moment.

Thanks!
🤦‍♂️ The ironic thing is that this is related to the bug I wanted to solve in the IE implementation: Not working for non-element nodes.

Thanks for finding a good external polyfill for this. It's good to now know about that library.

I updated this PR to add the polyfill before the 'wp-dom' script since appears to be a foundational script for the packages (even for wp-element through its wp-utils dependencies). I believe this may fall short of how widely you wanted to load this, but what do you think?

[aduth]

I'd say: Load it everywhere; every WordPress admin screen; for any features we ever use.

This was intentionally exaggerated to drive home the point. There might be some techniques we can leverage to limit its impact. For example, we should probably not need to load it if:

• There are zero scripts on a screen otherwise
• We have some way of knowing / inferring that modern authoring practices are employed
  • Only those which are transpiled (from a build directory)?
  • Some other metric akin to @babel/preset-env@next's useBuiltIns: 'usage'?
  • Register scripts through a new abstracted registration function, e.g. wp_register_script_with_polyfill?

[brandonpayton]

This was intentionally exaggerated to drive home the point. There might be some techniques we can leverage to limit its impact.

Got it. I will give this some thought in the morning. And please feel free to comment if anything else occurs to you. Thanks!

[aduth]

Confirmed that document.contains( document ) works in IE11 while on editor:

And that the polyfill is loaded in IE11, but not in Chrome.

I updated this PR to add the polyfill before the 'wp-dom' script since appears to be a foundational script for the packages (even for wp-element through its wp-utils dependencies). I believe this may fall short of how widely you wanted to load this, but what do you think?

I don't think it's correct, but neither is our other current usage. There's a couple things I'd like to see, but not necessarily as part of these changes:

• Avoid polyfills being considered as dependencies of specific scripts, unless we're certain we can maintain those dependencies.
  • There's nothing about wp-dom that depends on the Node#contains polyfill. Wherever we're using it should be the dependency
  • But then again, this highlights the bigger problem that this is a source of maintenance headaches and is likely not worth maintaining, vs. load always
• Better names for the script enqueues, which avoids potential plugin incompatibilities. Probably something with a WP-specific prefix, e.g. wp-polyfill-node-contains

[brandonpayton]

Thanks for the review and discussing a better approach to polyfills. I'll merge this so I can work on things that depend on it, but I agree attaching to wp-dom is not ideal.

It seems like it would be good to simply print the polyfill conditions before printing other scripts if there are scripts to be printed. Doing so appears less straightforward than I expected, but I'm hoping to look at it again later today.

[gziolo]

I strongly agree that we should remove all usages of import 'element-closest' and move this handling to polyfill architecture. It's still not an easy task to decide where to put it, as more than one module can depend on it.

Unless we mitigate it by wrapping all DOM operations into functions that are stored in @wordpress/dom package and therefore by design depend on those polyfills.

[aduth]

Created task at #7159 for tracking future improvements.