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

Fix autocompletion in IE11 #6667

Merged
merged 2 commits into from Jun 6, 2018

Conversation

Projects
None yet
6 participants
@brandonpayton
Member

brandonpayton commented May 9, 2018

Description

This is a PR to fix autocompletion in IE11.

Fixes #3409.

Details

The cause for this breakage is that IE11 doesn't dispatch input events for contenteditable, only for <input> and <textarea> elements.

The proposed fix is to update our TinyMCE component to listen for IE11's comparable textinput event and manually creating and dispatching an input on the same target. In addition, because textinput isn't dispatched for deletions, we also dispatch input for Delete and Backspace keyup events.

How has this been tested?

Loaded Gutenberg in IE11, Chrome, Firefox, and Safari and exercised the block and user completers.

I also used the IE11 and Chrome debuggers to verify the fix is applied for just IE and that its event listeners are removed when unmounted.

Screenshots

screen shot 2018-05-09 at 1 18 09 pm

Checklist:

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

@brandonpayton brandonpayton requested review from aduth and jorgefilipecosta May 9, 2018

@brandonpayton

This comment has been minimized.

Member

brandonpayton commented May 9, 2018

I'm finding that compareDocumentPosition only appears to work for testing direct containment. It's breaking when selection enters a grandchild of the container.

We can still use it if we want, but we'd need to loop and check if parent nodes are contained until we reach the document root or find containment.

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented May 16, 2018

Hi @brandonpayton thank you for creating this PR. I like this version and I think it is better when compared to what was proposed in #5914 as it avoids the need for a new component.

I'm finding that compareDocumentPosition only appears to work for testing direct containment. It's breaking when selection enters a grandchild of the container.
Would using the check used in #5914 solve the problem:

				if ( ! container.contains( selection.anchorNode ) &&
					selection.anchorNode &&
					! container.textContent.includes( selection.anchorNode.textContent.trim() )
				) {
					throw new Error( 'Invalid assumption: expected selection to be within the autocomplete container' );
				}
@brandonpayton

This comment has been minimized.

Member

brandonpayton commented May 18, 2018

Hi @jorgefilipecosta, thanks for commenting on this. I'll work to clean this up for merge.

I haven't found a good feature test, so for now, I plan to conditionally add the textinput mapping to input when /Trident/.test( navigator.userAgent ) is true. I don't like it, but it's better than nothing IMO.

Would using the check used in #5914 solve the problem:

I was uncomfortable with the string-based containment check because we could have false positives. It's not really that important because this is just for a development warning, but since we may want to use Node#contains elsewhere, I went ahead an added a containsNode function to @wordpress/dom that should should work cross-browser. It may deserve its own PR, but I'll see if we can just discuss it here.

@brandonpayton

This comment has been minimized.

Member

brandonpayton commented May 18, 2018

I played around with this a bit more and found a couple of things that could be used for a feature test:

  1. Adding an empty text node to contenteditable in IE11 causes a textinput event to be dispatched.
  2. Deleting a zero-width space using document.execCommand( 'delete', false ) causes an input event to be dispatched in IE11 for <input> and <textarea> elements.

☝️ I'm posting this in case this is interesting to someone, but I don't want to use such complicated testing per TinyMCE instance, which is what we'd need to do since IE11 contenteditable input behavior depends on the element. Something like this is simpler and at least limited to IE11:

const listenForTextInput =
  /Trident/.test( navigator.userAgent ) &&
  ! /input/i.test( tagName ) &&
  ! /textarea/i.test( tagName );
@noisysocks

This comment has been minimized.

Member

noisysocks commented May 29, 2018

Is this still WIP?

@brandonpayton

This comment has been minimized.

Member

brandonpayton commented May 30, 2018

@noisysocks Yeah, I need to make the fix conditional and probably split out the change to @wordpress/dom to its own PR.

@brandonpayton brandonpayton self-assigned this May 30, 2018

@brandonpayton brandonpayton referenced this pull request May 30, 2018

Merged

Provide cross-browser node containment checking #7033

4 of 4 tasks complete
@brandonpayton

This comment has been minimized.

Member

brandonpayton commented May 31, 2018

It turns out that IE11's textinput isn't dispatched for deletions. We can probably use keydown or keyup to listen for backspaces and deletes, but we should be careful to preserve typical event ordering (keydown before input, input before keyup) since we're actually dispatching an input (or input-like) event in response.

I like that this fix produces a real input event, but on the other hand, I like how #5914 simply called the component's onInput method which skips the ordering concern.

I plan to look at this more tomorrow.

Provide `input` event for IE11 contenteditables
IE11 doesn't dispatch `input` events for contenteditable, just for
`<input>` and `<textarea>` elements. However, it does dispatch a similar
`textinput` event for contenteditable.  In this change, we listen for
`textinput` events and manually dispatches an `input` event on the same
target. In addition, because `textinput` isn't dispatched for deletions,
we also dispatch `input` for Delete and Backspace `keyup` events.
@brandonpayton

This comment has been minimized.

Member

brandonpayton commented Jun 1, 2018

This is ready for review.

@brandonpayton brandonpayton changed the title from WIP Fix autocompletion in IE11 to Fix autocompletion in IE11 Jun 1, 2018

@noisysocks

Tested in IE11, Edge and Chrome. It fixes the bug!

I left some minor comments but nothing blocking.

}
/**
* Applies a fix that provides `input` events for contenteditable in InternetExplorer.

This comment has been minimized.

@noisysocks

noisysocks Jun 4, 2018

Member

Minor: Internet Explorer has a space 👀

this.removeInternetExplorerInputFix =
editorNode && needsInternetExplorerInputFix( editorNode ) ?
applyInternetExplorerInputFix( editorNode ) :
null;

This comment has been minimized.

@noisysocks

noisysocks Jun 4, 2018

Member

Minor: I think this would read better as an if, e.g.

if ( editorNode && needsInternetExplorerInputFix( editorNode ) ) {
	this.removeInternetExplorerInputFix = applyInternetExplorerInputfix( editorNode );
}
@@ -96,6 +182,23 @@ export default class TinyMCE extends Component {
} );
}
saveEditorNode( editorNode ) {

This comment has been minimized.

@noisysocks

noisysocks Jun 4, 2018

Member

Minor: bindEditorNode is more consistent with how we name these sorts of functions elsewhere in the codebase.

function needsInternetExplorerInputFix( editorNode ) {
return (
// Rely on userAgent in the absence of a reasonable feature test for contenteditable `input` events.
/Trident/.test( window.navigator.userAgent ) &&

This comment has been minimized.

@noisysocks

noisysocks Jun 4, 2018

Member

Could we borrow what Modernizr does here? Or is that for a different input event?

https://github.com/Modernizr/Modernizr/blob/master/feature-detects/event/oninput.js

This comment has been minimized.

@brandonpayton

brandonpayton Jun 5, 2018

Member

I thought about that too. Unfortunately, the test returns true for even a non-contenteditable <div> in IE11 because of the first part that sets an oninput attribute and checks the type of the element's oninput property. The browser does support the input event but doesn't dispatch it for contenteditable text input.

I tried to adapt the latter half of the test that dispatches a KeyboardEvent and listens for input but didn't have any luck getting it to work in IE11. It's possible I missed something but didn't think I should spend more time on it.

This comment has been minimized.

@brandonpayton

brandonpayton Jun 5, 2018

Member

Also, thank you for taking the time to look and make this suggestion, @noisysocks . I actually forgot that I'd looked at this until I worked through it again. :)

/**
* A ref function can be used for cleanup because React calls it with
* `null` when unmounting.
*/

This comment has been minimized.

@noisysocks

noisysocks Jun 4, 2018

Member

Oh, that's cool!

This comment has been minimized.

@brandonpayton

brandonpayton Jun 5, 2018

Member

Yeah, I love being able to keep allocation and cleanup in one place while also handling real situations where a ref is removed and re-added during the life of a component.

This comment has been minimized.

@aduth

aduth Jun 6, 2018

Member

Shouldn't we just use componentDidMount and componentWillUnmount ?

Related: The latest version of React includes a new React.createRef function which may become the recommended standard in place of the ref callback:

https://reactjs.org/docs/refs-and-the-dom.html#creating-refs

@brandonpayton brandonpayton merged commit 581323e into master Jun 6, 2018

2 checks passed

codecov/project 46.3% (-0.06%) compared to 20293c4
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@brandonpayton brandonpayton deleted the fix/ie11-missing-input-for-contenteditable branch Jun 6, 2018

@mtias mtias added this to the 3.1 milestone Jun 20, 2018

// Rely on userAgent in the absence of a reasonable feature test for contenteditable `input` events.
/Trident/.test( window.navigator.userAgent ) &&
// IE11 dispatches input events for `<input>` and `<textarea>`.
! /input/i.test( editorNode.tagName ) &&

This comment has been minimized.

@iseulde

iseulde Nov 16, 2018

Member

Why do we check this? A rich text field can never be an input or textarea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment