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

Make sure autocompleted values make it into the block's saved content #7486

Merged
merged 3 commits into from Jul 5, 2018

Conversation

Projects
None yet
5 participants
@notnownikki
Member

notnownikki commented Jun 22, 2018

Description

Potential fix for #7444

When an autocompleter does an insert-at-caret, the block isn't notified of the change,
and so the content doesn't get updated unless the user carries on typing.

This change dispatches an input event, which the editable component listens for and updates the block's attributes.

How has this been tested?

Carry out the steps shown in #7444
and the username should be saved correctly.

Types of changes

Bug fix

Checklist:

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

@notnownikki notnownikki requested a review from brandonpayton Jun 22, 2018

@notnownikki

This comment has been minimized.

Member

notnownikki commented Jun 22, 2018

This is the first time I've looked at this code, so I'm suggesting this as a potential approach. If it's acceptable, I'll write tests :)

@notnownikki notnownikki requested a review from aduth Jun 22, 2018

@notnownikki notnownikki changed the title from When inserting a value, Autocomplete should call onChange to Make sure autocompleted values make it into the block's saved content Jun 25, 2018

@@ -243,6 +243,9 @@ export class Autocomplete extends Component {
range.setStartAfter( child );
}
range.deleteContents();
if ( undefined !== this.props.onChange ) {

This comment has been minimized.

@gziolo

gziolo Jun 25, 2018

Member

Should we default to noop instead? Can we update docs to reflect this new change introduced?

This comment has been minimized.

@notnownikki

notnownikki Jun 25, 2018

Member

Sounds good to me, at this point I just want confirmation that this is the right type of fix, and I'll add a default noop and write tests if it is :)

@brandonpayton

Hi @notnownikki, thanks for working on this! I was thinking I'd need to look into this issue this morning, and it was a pleasant surprise to see you already had.

I'm thinking it might be better if we manually dispatch an input event on the contenteditable element since we already listen for an input event and it represents what is occurring and wouldn't require any interface change.

Maybe something like this at the end of insertCompletion:

// IE11 doesn't provide an InputEvent constructor.
const inputEvent = typeof InputEvent !== 'undefined' ? new InputEvent() : (() => {
  const e = document.createEvent( 'UIEvent' );
  e.initEvent( 'input', true /* bubbles */, false /* cancelable */ );
  return e;
})();
range.commonAncestorContainer.closest( '[contenteditable=true]' ).dispatch( inputEvent );

What do you think?

@notnownikki

This comment has been minimized.

Member

notnownikki commented Jun 26, 2018

@brandonpayton that's a much cleaner solution! I've pushed up a new change that dispatches in input event, and it all seems to work fine :)

@mtias mtias added this to the 3.2 milestone Jun 27, 2018

@mcsf

This fix looks good to me, though at the moment my IE11 virtual machine is not cooperating.

inputEvent = document.createEvent( 'UIEvent' );
inputEvent.initEvent( 'input', true /* bubbles */, false /* cancelable */ );
}
range.commonAncestorContainer.closest( '[contenteditable=true]' ).dispatchEvent( inputEvent );

This comment has been minimized.

@mcsf

mcsf Jun 27, 2018

Contributor

To be strict, the polyfill for Element#closest should be imported at the top of this module:

import 'element-closest';

inputEvent = new window.InputEvent( 'input', { bubbles: true, cancelable: false } );
} else {
// IE11 doesn't provide an InputEvent constructor.
inputEvent = document.createEvent( 'UIEvent' );

This comment has been minimized.

@mcsf

mcsf Jun 27, 2018

Contributor

Honest question: what makes UIEvent more apt than Event? Can we document that choice?

This comment has been minimized.

@notnownikki

notnownikki Jun 27, 2018

Member

I was following @brandonpayton's lead on this... but looking at it, for consistency, shouldn't it be an InputEvent?

This comment has been minimized.

@mcsf

mcsf Jun 27, 2018

Contributor

Perhaps. :) Since this is an IE quirk, I don't know. I brought up Event because the typical examples out there on how to "deal with IE" use it instead of other event types.

This comment has been minimized.

@mcsf

mcsf Jun 27, 2018

Contributor

Actually, InputEvent is not supported at all, even with the fallback interface. It has to be either Event or UIEvent.

This comment has been minimized.

@brandonpayton

brandonpayton Jun 27, 2018

Member

I think we would be fine using Event, but my reason is that the InputEvent interface is based on UIEvent in the spec:
https://w3c.github.io/uievents/#idl-inputevent

@mcsf

mcsf approved these changes Jun 27, 2018

I don't feel strongly enough about the Event/UIEvent choice and don't mean to block this PR — so, if this PR also correctly fixes the problem in IE11, feel free to merge it!

@notnownikki

This comment has been minimized.

Member

notnownikki commented Jun 27, 2018

Tested in IE11, works fine!

@brandonpayton

brandonpayton approved these changes Jun 27, 2018 edited

I tested in IE11, and the fix is effective to address #7444. However, I am seeing our manually dispatched input event trigger another display of the completion menu. This also occurs in Firefox.
reopened-completion-menu

If I hackinsertCompletion to insert a space after a completion, the menu is not shown after the completion is inserted. This makes some sense as the originally inserted text would indeed trigger the completer menu if it was input by the user.

Once the autocomplete token PR #6577 is merged (hopefully later today), this will no longer occur because the cursor is placed outside completion tokens after insertion.

I am happy with this change but think we should wait to merge after #6577.

@notnownikki

This comment has been minimized.

Member

notnownikki commented Jun 27, 2018

@brandonpayton thanks! I'm happy to wait :)

@gziolo

This comment has been minimized.

Member

gziolo commented Jul 3, 2018

@brandonpayton - should we still wait for #6577?

@brandonpayton

This comment has been minimized.

Member

brandonpayton commented Jul 3, 2018

@gziolo, I don't want to wait, but there is a bug with this solution if we do not have separation because the input event triggers Autocomplete to redisplay the menu since the cursor is still at the end of a pattern matching the user autocompleter. The autocomplete token update would cause this not to occur because it separates the cursor from the token on insertion.

A temporary workaround might be to insert a non-breaking zero-width space following a completion. That would avoid this issue. It might introduce a surprising stop when moving the cursor with arrow keys, but that seems more tolerable than this bug. What do you think?

@brandonpayton

This comment has been minimized.

Member

brandonpayton commented Jul 4, 2018

@gziolo I realized there is another solution to the menu-redisplay issue that doesn't require waiting on #6577. I pushed a commit with that fix. If the change looks good to you, I think this is ready to merge.

@gziolo

This comment has been minimized.

Member

gziolo commented Jul 5, 2018

Yes, works for me. I can see the user handle properly published 🎉

@gziolo gziolo merged commit 18d832e into master Jul 5, 2018

2 checks passed

codecov/project 46.4% (-0.02%) compared to 8b21632
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gziolo gziolo deleted the fix/autocomplete-should-update-block branch Jul 5, 2018

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