IE11: browser crashes on pressing Enter to switch to edit mode

[tellthemachines]

Describe the bug

On IE11, with a block selected and with the editor in select mode, pressing Enter to switch to edit mode causes the editor to crash with the following error message:

TypeError: Unable to get property 'closest' of undefined or null reference
   at vi (http://isabel:8888/wp-content/plugins/gutenberg/build/block-editor/index.js?ver=4af3ae3fe13f697ebe49daa5a278cc59:49:122078)
   at Anonymous function (http://isabel:8888/wp-content/plugins/gutenberg/build/block-editor/index.js?ver=4af3ae3fe13f697ebe49daa5a278cc59:49:142265)
   at Anonymous function (http://isabel:8888/wp-content/plugins/gutenberg/build/block-editor/index.js?ver=4af3ae3fe13f697ebe49daa5a278cc59:49:142254)
   at commitHookEffectList (http://isabel:8888/wp-includes/js/dist/vendor/react-dom.js?ver=16.9.0:20124:9)
   at commitPassiveHookEffects (http://isabel:8888/wp-includes/js/dist/vendor/react-dom.js?ver=16.9.0:20154:11)
   at callCallback (http://isabel:8888/wp-includes/js/dist/vendor/react-dom.js?ver=16.9.0:341:9)

The console also reports:

The above error occurred in the <ForwardRef> component:
    in ForwardRef (created by ForwardRef)
    in ForwardRef
    in Unknown (created by t)
    in t (created by WithInstanceId(t))
    in WithInstanceId(t) (created by WithSpokenMessages(WithInstanceId(t)))
    in WithSpokenMessages(WithInstanceId(t))
    in Unknown (created by Context.Consumer)
    in WithBlockEditContext(Component) (created by t)
    in t
    in Unknown (created by WithSafeTimeout(Component))
    in WithSafeTimeout(Component) (created by ForwardRef)
    in ForwardRef (created by ForwardRef(e))
    in ForwardRef(e)
    in Unknown
    in Unknown (created by WithToolbarControls(Component))
    in WithToolbarControls(Component) (created by WithInspectorControl(WithToolbarControls(Component)))
    in WithInspectorControl(WithToolbarControls(Component)) (created by WithInspectorControl(WithInspectorControl(WithToolbarControls(Component))))
    in WithInspectorControl(WithInspectorControl(WithToolbarControls(Component))) (created by WithToolba

To reproduce
Steps to reproduce the behavior:

1. On IE11, navigate to a post in the editor.
2. With a block selected, press Esc to switch select mode, then press Enter to switch back to edit mode
3. See error

Expected behavior

Expected all keyboard shortcuts to work correctly.

[azaozz]

Crashes on pasting too (Ctrl + V and Paste menu), in WP 5.4.

Seems to come from here:

gutenberg/packages/blocks/src/api/raw-handling/html-formatting-remover.js

Line 45 in 0413162

if ( node.parentElement.closest( 'pre' ) ) {

This remains the same in the built file, but IE11 only supports Element.parentElement (see the hidden note here: https://developer.mozilla.org/en-US/docs/Web/API/Node/parentElement#Browser_compatibility), not on all nodes. However this is a text node, see the conditional right above on L40-L42.

Changing this to if ( node.parentNode.closest( 'pre' ) ) seems to fix it.

[aduth]

@azaozz Do you suspect it's the same issue for the original report? It would seem to be, based on the error message.

Changing this to if ( node.parentNode.closest( 'pre' ) ) seems to fix it.

Mentioned in Slack as well (link requires registration), since closest is of the Element interface, which extends Node, there's no strong guarantee we can use this method. It's more of a problem when we start to enforce this via type-checking (#18838).

I sense there's a few ways we could accommodate this. To me, it might just be simplest to bypass closest and step through the parents:

let parent = node;
while ( ( parent = parent.parentNode ) ) {
	if ( parent.nodeType === Node.ELEMENT_NODE && parent.nodeName === 'PRE' ) {
		return;
	}
}

[azaozz]

Tested only with pasting in the block editor in WP 5.4, but here is a similar use of node.parentElement for non-Element nodes in https://github.com/WordPress/gutenberg/blob/master/packages/block-editor/src/utils/dom.js#L86 which seems the more likely culprit.

it might just be simplest to bypass closest and step through the parents

Yep, sounds good.

The node.closest() is polyfilled in IE11, but it works only on Element nodes. Confirming that node.parentElement is an actual Element node would be good in all cases as parentElement may return null sometimes: https://developer.mozilla.org/en-US/docs/Web/API/Node/parentElement.

[aduth]

For what it's worth, if we had type-checking enabled for @wordpress/blocks, we would have caught this earlier:

Edit: Though, this one may be more about how a parent may not exist, not necessarily the behavior of IE. Still an issue, though one we might have been "okay" to let through in the case of assuming the candidate as having a parent.

[aduth]

The issue from the original comment is that we're passing null as the second argument to isInsideRootBlock from the block wrapper:

gutenberg/packages/block-editor/src/components/block-list/block-wrapper.js

Line 106 in 4b8d1f8

isInsideRootBlock( wrapper.current, document.activeElement )

gutenberg/packages/block-editor/src/utils/dom.js

Lines 59 to 60 in 4b8d1f8

	export function isInsideRootBlock( blockElement, element ) {
	const parentBlock = element.closest( '.block-editor-block-list__block' );

It is important to note that document.activeElement can be null.

When there is no selection, the active element is the page's <body> or null.

https://developer.mozilla.org/en-US/docs/Web/API/DocumentOrShadowRoot/activeElement

This varies by browser implementation. Unsurprisingly, Internet Explorer is one such browser where activeElement is a null value when there is no active element.

This is a nearly identical problem as in #20594.

The solution could be one of two approaches:

• We can allow isInsideRootBlock to accept a null value for its element argument, and then guard any attempts to reference Element#closest.
• Or, we could avoid calling isInsideRootBlock from the block wrapper if document.activeElement is null.

Separately, given that there have been multiple instances of this issue, I would suggest we consider to create some new utility to normalize the behavior of document.activeElement, so that we can safely operate on its value.

Example:

function getActiveElement() {
    return document.activeElement && document.activeElement !== document.body
        ? document.activeElement
        : null
}

[aduth]

Fix at #21361
Partial type-checking proposed at #21362

[aduth]

@azaozz Is there anything special about the type of text you're trying to paste (e.g. HTML formatted, etc)? I'm having trouble reproducing the crash.

[azaozz]

Is there anything special about the type of text you're trying to paste.

No, nothing specific. Happens here in WP 5.4 regardless of where the text was copied from. Copying some text from a Paragraph block and pasting it back in triggers it.

The errors with SCRIPT_DEBUG enabled are:

[aduth]

Oh, I see now. I was expecting a crash. It "just" errors, and doesn't paste. I can reproduce now, at least 👍

[azaozz]

Ah, sorry, not really a crash :)

[azaozz]

I would suggest we consider to create some new utility to normalize the behavior of document.activeElement

That would be best imho. Also seeing some "jumpiness" when clicking in the classic block that might be related.

[aduth]

Fix for #21276 (comment) is at #21366.

[aduth]

On reflection, a getActiveElement utility could be rather redundant as we move toward more type-checking, since the type-checking itself would prevent those sorts of errors. The other problem is that it would assume that a contributor even know that it exists to consider using it. If they're at the point to know it exists, it probably enters their awareness to consider document.activeElement as possibly-null. In other words, if we can't expect developers to anticipate document.activeElement as null, we probably also can't expect them to use a getActiveElement utility.