Skip to content

Conversation

@jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Mar 30, 2018

Autocompleter did not work on IE mainly because IE does not handle onInput events on editables. We now make use of a newly implemented BackCompatibleOnInput component that falls back to key events when onInput is not supported.

Fixes: #3409

How Has This Been Tested?

Test auto-completer, in IE and other browsers, verify it works well.

@jorgefilipecosta jorgefilipecosta added the [Type] Bug An existing feature does not function as intended label Mar 30, 2018
@jorgefilipecosta jorgefilipecosta self-assigned this Mar 30, 2018
@jorgefilipecosta jorgefilipecosta requested a review from a team April 2, 2018 19:21
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of the mutation observer. Not only does it invoke its callback excessively, even when input isn't occurring (once for each block at initialization, once or twice when clicking within a block), but it also adds a fair bit of code complexity.

As I see it, this feature is a convenience, not the only mechanism for block insertion or user mentions. The basic functionality is possible either way, so I'd be more inclined to file it under progressive enhancement and ignore the fact that IE bugs prevent it from working correctly, rather than impose a performance cost upon the larger majority of users, and maintenance cost on future developers.

That said, I'd be curious to explore targeting this compatibility to buggy browsers: Use onInput where supported, else onKeyDown or a mutation observer. Could even imagine it abstracted to some generalized component to reuse when we need to capture onInput for a non-input element.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/autocompleter-on-ie branch 2 times, most recently from 8d2cfd6 to 3b48498 Compare April 10, 2018 15:31
@jorgefilipecosta
Copy link
Member Author

Hi @aduth thank you for the feedback, it was applied. Now, we have a component that abstracts on input browser compatibility and if we detect support of onInput we just use it, if support is not detected we use key events.

@jorgefilipecosta jorgefilipecosta requested a review from a team April 18, 2018 08:39
const { isOnInputSupported } = this.state;
const { onInput, children, ...props } = this.props;
return (
<div
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering two options for avoiding the wrapping div:

  • Apply as a div, but use in place of, not as a child, of the original component's div (i.e. the one with className="components-autocomplete")
  • Use cloneElement to apply the normalized event to the original child

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not using a wrapper is challenging as the same component, may want a back-compatible onInput but also have an onKeyUp event (the case of autocomplete).
One option would be to use cloneElement but on the onKeyUp event pass a handler that does the logic of our component but also calls the existing onKeyUp event handler the component had.
I'm not certain if it the best way for this case, or if the additional complexity is valuable when compared to use a div wrapper.

@@ -0,0 +1,42 @@
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo on file name: back-compatile-on-input -> back-compatible-on-input

Aside: Not sure I love the name, mostly because it's an abbreviation of "backwards", though also thinking there might be some alternative like NormalizedOnInput or ContentEditable

}

onKeyUp( event ) {
this.props.onInput( event );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For supported browsers, both the onInput and onKeyUp will be triggered on the first keypress. Is there a way we can cancel handling of the onKeyUp if we'd already done onInput ? Maybe an instance variable which is checked here (or state, if it's already reflected).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was improved, we now handle that case so we don't fire the first key two times.

Autocompleter did not worked on IE because IE does not handle onInput events on editables. We now make use of a newly implemented BackCompatibleOnInput component that falls back to key events when onInput is not supported.
@jorgefilipecosta jorgefilipecosta force-pushed the fix/autocompleter-on-ie branch from 3b48498 to 262b4ae Compare April 27, 2018 08:52
@jorgefilipecosta jorgefilipecosta changed the title Fixed auto completer on IE. Fix auto completer on IE May 2, 2018
@jorgefilipecosta
Copy link
Member Author

The issue was fixed using an alternative approach: #6667. Thank you @brandonpayton!

@jorgefilipecosta jorgefilipecosta deleted the fix/autocompleter-on-ie branch June 6, 2018 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Bug An existing feature does not function as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants