From 5efc86069ff92d980bb460c15deccb1e0fff9363 Mon Sep 17 00:00:00 2001 From: Kara Erickson Date: Mon, 20 Mar 2017 17:38:33 -0700 Subject: [PATCH] fix(forms): make composition event buffering configurable (#15256) This commit fixes a regression where `ngModel` no longer syncs letter by letter on Android devices, and instead syncs at the end of every word. This broke when we introduced buffering of IME events so IMEs like Pinyin keyboards or Katakana keyboards wouldn't display composition strings. Unfortunately, iOS devices and Android devices have opposite event behavior. Whereas iOS devices fire composition events for IME keyboards only, Android fires composition events for Latin-language keyboards. For this reason, languages like English don't work as expected on Android if we always buffer. So to support both platforms, composition string buffering will only be turned on by default for non-Android devices. However, we have also added a `COMPOSITION_BUFFER_MODE` token to make this configurable by the application. In some cases, apps might might still want to receive intermediate values. For example, some inputs begin searching based on Latin letters before a character selection is made. As a provider, this is fairly flexible. If you want to turn composition buffering off, simply provide the token at the top level: ```ts providers: [ {provide: COMPOSITION_BUFFER_MODE, useValue: false} ] ``` Or, if you want to change the mode based on locale or platform, you can use a factory: ```ts import {shouldUseBuffering} from 'my/lib'; .... providers: [ {provide: COMPOSITION_BUFFER_MODE, useFactory: shouldUseBuffering} ] ``` Closes #15079. PR Close #15256 --- build.sh | 2 +- packages/forms/package.json | 3 +- packages/forms/rollup.config.js | 1 + .../src/directives/default_value_accessor.ts | 52 +++++++- packages/forms/src/directives/ng_model.ts | 14 +- packages/forms/src/forms.ts | 2 +- packages/forms/test/directives_spec.ts | 2 +- .../forms/test/reactive_integration_spec.ts | 83 +++++++++++- .../forms/test/template_integration_spec.ts | 124 +++++++++++++----- packages/forms/tsconfig-build.json | 3 +- .../src/web_workers/worker/worker_adapter.ts | 2 +- tools/public_api_guard/forms/forms.d.ts | 10 +- 12 files changed, 237 insertions(+), 61 deletions(-) diff --git a/build.sh b/build.sh index c7efa9507c..6701cb8ccc 100755 --- a/build.sh +++ b/build.sh @@ -12,9 +12,9 @@ PACKAGES=(core compiler common animations - forms platform-browser platform-browser-dynamic + forms http platform-server platform-webworker diff --git a/packages/forms/package.json b/packages/forms/package.json index 77944bb6c5..739bd68695 100644 --- a/packages/forms/package.json +++ b/packages/forms/package.json @@ -10,7 +10,8 @@ "license": "MIT", "peerDependencies": { "@angular/core": "0.0.0-PLACEHOLDER", - "@angular/common": "0.0.0-PLACEHOLDER" + "@angular/common": "0.0.0-PLACEHOLDER", + "@angular/platform-browser": "0.0.0-PLACEHOLDER" }, "repository": { "type": "git", diff --git a/packages/forms/rollup.config.js b/packages/forms/rollup.config.js index 195b9141fe..eacc9f7a3b 100644 --- a/packages/forms/rollup.config.js +++ b/packages/forms/rollup.config.js @@ -16,6 +16,7 @@ export default { '@angular/core': 'ng.core', '@angular/common': 'ng.common', '@angular/compiler': 'ng.compiler', + '@angular/platform-browser': 'ng.platformBrowser', 'rxjs/Observable': 'Rx', 'rxjs/Subject': 'Rx', 'rxjs/observable/fromPromise': 'Rx.Observable', diff --git a/packages/forms/src/directives/default_value_accessor.ts b/packages/forms/src/directives/default_value_accessor.ts index d4aaa60005..26df6411e9 100644 --- a/packages/forms/src/directives/default_value_accessor.ts +++ b/packages/forms/src/directives/default_value_accessor.ts @@ -6,8 +6,8 @@ * found in the LICENSE file at https://angular.io/license */ -import {Directive, ElementRef, Renderer, forwardRef} from '@angular/core'; - +import {Directive, ElementRef, Inject, InjectionToken, Optional, Renderer, forwardRef} from '@angular/core'; +import {ɵgetDOM as getDOM} from '@angular/platform-browser'; import {ControlValueAccessor, NG_VALUE_ACCESSOR} from './control_value_accessor'; export const DEFAULT_VALUE_ACCESSOR: any = { @@ -16,6 +16,21 @@ export const DEFAULT_VALUE_ACCESSOR: any = { multi: true }; +/** + * We must check whether the agent is Android because composition events + * behave differently between iOS and Android. + */ +function _isAndroid(): boolean { + const userAgent = getDOM() ? getDOM().getUserAgent() : ''; + return /android (\d+)/.test(userAgent.toLowerCase()); +} + +/** + * Turn this mode on if you want form directives to buffer IME input until compositionend + * @experimental + */ +export const COMPOSITION_BUFFER_MODE = new InjectionToken('CompositionEventMode'); + /** * The default accessor for writing a value and listening to changes that is used by the * {@link NgModel}, {@link FormControlDirective}, and {@link FormControlName} directives. @@ -32,15 +47,29 @@ export const DEFAULT_VALUE_ACCESSOR: any = { 'input:not([type=checkbox])[formControlName],textarea[formControlName],input:not([type=checkbox])[formControl],textarea[formControl],input:not([type=checkbox])[ngModel],textarea[ngModel],[ngDefaultControl]', // TODO: vsavkin replace the above selector with the one below it once // https://github.com/angular/angular/issues/3011 is implemented - // selector: '[ngControl],[ngModel],[ngFormControl]', - host: {'(input)': 'onChange($event.target.value)', '(blur)': 'onTouched()'}, + // selector: '[ngModel],[formControl],[formControlName]', + host: { + '(input)': '_handleInput($event.target.value)', + '(blur)': 'onTouched()', + '(compositionstart)': '_compositionStart()', + '(compositionend)': '_compositionEnd($event.target.value)' + }, providers: [DEFAULT_VALUE_ACCESSOR] }) export class DefaultValueAccessor implements ControlValueAccessor { onChange = (_: any) => {}; onTouched = () => {}; - constructor(private _renderer: Renderer, private _elementRef: ElementRef) {} + /** Whether the user is creating a composition string (IME events). */ + private _composing = false; + + constructor( + private _renderer: Renderer, private _elementRef: ElementRef, + @Optional() @Inject(COMPOSITION_BUFFER_MODE) private _compositionMode: boolean) { + if (this._compositionMode == null) { + this._compositionMode = !_isAndroid(); + } + } writeValue(value: any): void { const normalizedValue = value == null ? '' : value; @@ -53,4 +82,17 @@ export class DefaultValueAccessor implements ControlValueAccessor { setDisabledState(isDisabled: boolean): void { this._renderer.setElementProperty(this._elementRef.nativeElement, 'disabled', isDisabled); } + + _handleInput(value: any): void { + if (!this._compositionMode || (this._compositionMode && !this._composing)) { + this.onChange(value); + } + } + + _compositionStart(): void { this._composing = true; } + + _compositionEnd(value: any): void { + this._composing = false; + this._compositionMode && this.onChange(value); + } } diff --git a/packages/forms/src/directives/ng_model.ts b/packages/forms/src/directives/ng_model.ts index 44a779328c..c27b28d026 100644 --- a/packages/forms/src/directives/ng_model.ts +++ b/packages/forms/src/directives/ng_model.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {Directive, EventEmitter, Host, HostListener, Inject, Input, OnChanges, OnDestroy, Optional, Output, Self, SimpleChanges, forwardRef} from '@angular/core'; +import {Directive, EventEmitter, Host, Inject, Input, OnChanges, OnDestroy, Optional, Output, Self, SimpleChanges, forwardRef} from '@angular/core'; import {FormControl} from '../model'; import {NG_ASYNC_VALIDATORS, NG_VALIDATORS} from '../validators'; @@ -114,7 +114,6 @@ export class NgModel extends NgControl implements OnChanges, _control = new FormControl(); /** @internal */ _registered = false; - private _composing = false; viewModel: any; @Input() name: string; @@ -124,15 +123,6 @@ export class NgModel extends NgControl implements OnChanges, @Output('ngModelChange') update = new EventEmitter(); - @HostListener('compositionstart') - compositionStart(): void { this._composing = true; } - - @HostListener('compositionend') - compositionEnd(): void { - this._composing = false; - this.update.emit(this.viewModel); - } - constructor(@Optional() @Host() parent: ControlContainer, @Optional() @Self() @Inject(NG_VALIDATORS) validators: Array, @Optional() @Self() @Inject(NG_ASYNC_VALIDATORS) asyncValidators: Array, @@ -176,7 +166,7 @@ export class NgModel extends NgControl implements OnChanges, viewToModelUpdate(newValue: any): void { this.viewModel = newValue; - !this._composing && this.update.emit(newValue); + this.update.emit(newValue); } private _setUpControl(): void { diff --git a/packages/forms/src/forms.ts b/packages/forms/src/forms.ts index f0ec1cbd00..81caaa2dca 100644 --- a/packages/forms/src/forms.ts +++ b/packages/forms/src/forms.ts @@ -23,7 +23,7 @@ export {AbstractFormGroupDirective} from './directives/abstract_form_group_direc export {CheckboxControlValueAccessor} from './directives/checkbox_value_accessor'; export {ControlContainer} from './directives/control_container'; export {ControlValueAccessor, NG_VALUE_ACCESSOR} from './directives/control_value_accessor'; -export {DefaultValueAccessor} from './directives/default_value_accessor'; +export {COMPOSITION_BUFFER_MODE, DefaultValueAccessor} from './directives/default_value_accessor'; export {Form} from './directives/form_interface'; export {NgControl} from './directives/ng_control'; export {NgControlStatus, NgControlStatusGroup} from './directives/ng_control_status'; diff --git a/packages/forms/test/directives_spec.ts b/packages/forms/test/directives_spec.ts index d5335f8c88..642886c991 100644 --- a/packages/forms/test/directives_spec.ts +++ b/packages/forms/test/directives_spec.ts @@ -44,7 +44,7 @@ export function main() { describe('Form Directives', () => { let defaultAccessor: DefaultValueAccessor; - beforeEach(() => { defaultAccessor = new DefaultValueAccessor(null, null); }); + beforeEach(() => { defaultAccessor = new DefaultValueAccessor(null, null, null); }); describe('shared', () => { describe('selectValueAccessor', () => { diff --git a/packages/forms/test/reactive_integration_spec.ts b/packages/forms/test/reactive_integration_spec.ts index 692f00e4d8..8d5ba36e43 100644 --- a/packages/forms/test/reactive_integration_spec.ts +++ b/packages/forms/test/reactive_integration_spec.ts @@ -8,7 +8,7 @@ import {Component, Directive, EventEmitter, Input, Output, Type, forwardRef} from '@angular/core'; import {ComponentFixture, TestBed, fakeAsync, tick} from '@angular/core/testing'; -import {AbstractControl, AsyncValidator, AsyncValidatorFn, ControlValueAccessor, FormArray, FormControl, FormGroup, FormGroupDirective, FormsModule, NG_ASYNC_VALIDATORS, NG_VALIDATORS, NG_VALUE_ACCESSOR, NgControl, ReactiveFormsModule, Validators} from '@angular/forms'; +import {AbstractControl, AsyncValidator, AsyncValidatorFn, COMPOSITION_BUFFER_MODE, ControlValueAccessor, FormArray, FormControl, FormGroup, FormGroupDirective, FormsModule, NG_ASYNC_VALIDATORS, NG_VALIDATORS, NG_VALUE_ACCESSOR, NgControl, ReactiveFormsModule, Validators} from '@angular/forms'; import {By} from '@angular/platform-browser/src/dom/debug/by'; import {getDOM} from '@angular/platform-browser/src/dom/dom_adapter'; import {dispatchEvent} from '@angular/platform-browser/testing/src/browser_util'; @@ -1908,6 +1908,87 @@ export function main() { }); }); + + describe('IME events', () => { + + it('should determine IME event handling depending on platform by default', () => { + const fixture = initTest(FormControlComp); + fixture.componentInstance.control = new FormControl('oldValue'); + fixture.detectChanges(); + + const inputEl = fixture.debugElement.query(By.css('input')); + const inputNativeEl = inputEl.nativeElement; + expect(inputNativeEl.value).toEqual('oldValue'); + + inputEl.triggerEventHandler('compositionstart', null); + + inputNativeEl.value = 'updatedValue'; + dispatchEvent(inputNativeEl, 'input'); + const isAndroid = /android (\d+)/.test(getDOM().getUserAgent().toLowerCase()); + + if (isAndroid) { + // On Android, values should update immediately + expect(fixture.componentInstance.control.value).toEqual('updatedValue'); + } else { + // On other platforms, values should wait for compositionend + expect(fixture.componentInstance.control.value).toEqual('oldValue'); + + inputEl.triggerEventHandler('compositionend', {target: {value: 'updatedValue'}}); + fixture.detectChanges(); + expect(fixture.componentInstance.control.value).toEqual('updatedValue'); + } + }); + + it('should hold IME events until compositionend if composition mode', () => { + TestBed.overrideComponent( + FormControlComp, + {set: {providers: [{provide: COMPOSITION_BUFFER_MODE, useValue: true}]}}); + const fixture = initTest(FormControlComp); + fixture.componentInstance.control = new FormControl('oldValue'); + fixture.detectChanges(); + + const inputEl = fixture.debugElement.query(By.css('input')); + const inputNativeEl = inputEl.nativeElement; + expect(inputNativeEl.value).toEqual('oldValue'); + + inputEl.triggerEventHandler('compositionstart', null); + + inputNativeEl.value = 'updatedValue'; + dispatchEvent(inputNativeEl, 'input'); + + // should not update when compositionstart + expect(fixture.componentInstance.control.value).toEqual('oldValue'); + + inputEl.triggerEventHandler('compositionend', {target: {value: 'updatedValue'}}); + + fixture.detectChanges(); + + // should update when compositionend + expect(fixture.componentInstance.control.value).toEqual('updatedValue'); + }); + + it('should work normally with composition events if composition mode is off', () => { + TestBed.overrideComponent( + FormControlComp, + {set: {providers: [{provide: COMPOSITION_BUFFER_MODE, useValue: false}]}}); + const fixture = initTest(FormControlComp); + fixture.componentInstance.control = new FormControl('oldValue'); + fixture.detectChanges(); + + const inputEl = fixture.debugElement.query(By.css('input')); + const inputNativeEl = inputEl.nativeElement; + expect(inputNativeEl.value).toEqual('oldValue'); + + inputEl.triggerEventHandler('compositionstart', null); + + inputNativeEl.value = 'updatedValue'; + dispatchEvent(inputNativeEl, 'input'); + fixture.detectChanges(); + + // formControl should update normally + expect(fixture.componentInstance.control.value).toEqual('updatedValue'); + }); + }); }); } diff --git a/packages/forms/test/template_integration_spec.ts b/packages/forms/test/template_integration_spec.ts index 1f7d957dae..4ae9c7cd08 100644 --- a/packages/forms/test/template_integration_spec.ts +++ b/packages/forms/test/template_integration_spec.ts @@ -8,7 +8,7 @@ import {Component, Directive, Input, Type, forwardRef} from '@angular/core'; import {ComponentFixture, TestBed, async, fakeAsync, tick} from '@angular/core/testing'; -import {AbstractControl, AsyncValidator, ControlValueAccessor, FormsModule, NG_ASYNC_VALIDATORS, NG_VALUE_ACCESSOR, NgForm} from '@angular/forms'; +import {AbstractControl, AsyncValidator, COMPOSITION_BUFFER_MODE, ControlValueAccessor, FormsModule, NG_ASYNC_VALIDATORS, NG_VALUE_ACCESSOR, NgForm} from '@angular/forms'; import {By} from '@angular/platform-browser/src/dom/debug/by'; import {getDOM} from '@angular/platform-browser/src/dom/dom_adapter'; import {dispatchEvent} from '@angular/platform-browser/testing/src/browser_util'; @@ -42,39 +42,6 @@ export function main() { expect(fixture.componentInstance.name).toEqual('updatedValue'); })); - it('should ngModel hold ime events until compositionend', fakeAsync(() => { - const fixture = initTest(StandaloneNgModel); - // model -> view - const inputEl = fixture.debugElement.query(By.css('input')); - const inputNativeEl = inputEl.nativeElement; - - fixture.componentInstance.name = 'oldValue'; - - fixture.detectChanges(); - tick(); - - expect(inputNativeEl.value).toEqual('oldValue'); - // view -> model - inputEl.triggerEventHandler('compositionstart', null); - - inputNativeEl.value = 'updatedValue'; - dispatchEvent(inputNativeEl, 'input'); - tick(); - - // should ngModel not update when compositionstart - - expect(fixture.componentInstance.name).toEqual('oldValue'); - - inputEl.triggerEventHandler('compositionend', null); - - fixture.detectChanges(); - tick(); - - // should ngModel update when compositionend - - expect(fixture.componentInstance.name).toEqual('updatedValue'); - })); - it('should support ngModel registration with a parent form', fakeAsync(() => { const fixture = initTest(NgModelForm); fixture.componentInstance.name = 'Nancy'; @@ -1166,6 +1133,95 @@ export function main() { }); + describe('IME events', () => { + it('should determine IME event handling depending on platform by default', fakeAsync(() => { + const fixture = initTest(StandaloneNgModel); + const inputEl = fixture.debugElement.query(By.css('input')); + const inputNativeEl = inputEl.nativeElement; + fixture.componentInstance.name = 'oldValue'; + fixture.detectChanges(); + tick(); + expect(inputNativeEl.value).toEqual('oldValue'); + + inputEl.triggerEventHandler('compositionstart', null); + + inputNativeEl.value = 'updatedValue'; + dispatchEvent(inputNativeEl, 'input'); + tick(); + + const isAndroid = /android (\d+)/.test(getDOM().getUserAgent().toLowerCase()); + if (isAndroid) { + // On Android, values should update immediately + expect(fixture.componentInstance.name).toEqual('updatedValue'); + } else { + // On other platforms, values should wait until compositionend + expect(fixture.componentInstance.name).toEqual('oldValue'); + + inputEl.triggerEventHandler('compositionend', {target: {value: 'updatedValue'}}); + + fixture.detectChanges(); + tick(); + + expect(fixture.componentInstance.name).toEqual('updatedValue'); + } + })); + + it('should hold IME events until compositionend if composition mode', fakeAsync(() => { + TestBed.overrideComponent( + StandaloneNgModel, + {set: {providers: [{provide: COMPOSITION_BUFFER_MODE, useValue: true}]}}); + const fixture = initTest(StandaloneNgModel); + const inputEl = fixture.debugElement.query(By.css('input')); + const inputNativeEl = inputEl.nativeElement; + fixture.componentInstance.name = 'oldValue'; + fixture.detectChanges(); + tick(); + expect(inputNativeEl.value).toEqual('oldValue'); + + inputEl.triggerEventHandler('compositionstart', null); + + inputNativeEl.value = 'updatedValue'; + dispatchEvent(inputNativeEl, 'input'); + tick(); + + // ngModel should not update when compositionstart + expect(fixture.componentInstance.name).toEqual('oldValue'); + + inputEl.triggerEventHandler('compositionend', {target: {value: 'updatedValue'}}); + + fixture.detectChanges(); + tick(); + + // ngModel should update when compositionend + expect(fixture.componentInstance.name).toEqual('updatedValue'); + })); + + it('should work normally with composition events if composition mode is off', + fakeAsync(() => { + TestBed.overrideComponent( + StandaloneNgModel, + {set: {providers: [{provide: COMPOSITION_BUFFER_MODE, useValue: false}]}}); + const fixture = initTest(StandaloneNgModel); + + const inputEl = fixture.debugElement.query(By.css('input')); + const inputNativeEl = inputEl.nativeElement; + fixture.componentInstance.name = 'oldValue'; + fixture.detectChanges(); + tick(); + expect(inputNativeEl.value).toEqual('oldValue'); + + inputEl.triggerEventHandler('compositionstart', null); + + inputNativeEl.value = 'updatedValue'; + dispatchEvent(inputNativeEl, 'input'); + tick(); + + // ngModel should update normally + expect(fixture.componentInstance.name).toEqual('updatedValue'); + })); + + }); + describe('ngModel corner cases', () => { it('should update the view when the model is set back to what used to be in the view', fakeAsync(() => { diff --git a/packages/forms/tsconfig-build.json b/packages/forms/tsconfig-build.json index 949c0bcaca..74d0fa0488 100644 --- a/packages/forms/tsconfig-build.json +++ b/packages/forms/tsconfig-build.json @@ -13,7 +13,8 @@ "@angular/common": ["../../dist/packages/common"], "@angular/common/testing": ["../../dist/packages/common/testing"], "@angular/compiler": ["../../dist/packages/compiler"], - "@angular/compiler/testing": ["../../dist/packages/compiler/testing"] + "@angular/compiler/testing": ["../../dist/packages/compiler/testing"], + "@angular/platform-browser": ["../../dist/packages/platform-browser"] }, "rootDir": ".", "sourceMap": true, diff --git a/packages/platform-webworker/src/web_workers/worker/worker_adapter.ts b/packages/platform-webworker/src/web_workers/worker/worker_adapter.ts index c8509b6e3d..f2bb27b29a 100644 --- a/packages/platform-webworker/src/web_workers/worker/worker_adapter.ts +++ b/packages/platform-webworker/src/web_workers/worker/worker_adapter.ts @@ -150,7 +150,7 @@ export class WorkerDomAdapter extends DomAdapter { getLocation(): Location { throw 'not implemented'; } getBaseHref(doc: Document): string { throw 'not implemented'; } resetBaseElement(): void { throw 'not implemented'; } - getUserAgent(): string { throw 'not implemented'; } + getUserAgent(): string { return 'Fake user agent'; } setData(element: any, name: string, value: string) { throw 'not implemented'; } getComputedStyle(element: any): any { throw 'not implemented'; } getData(element: any, name: string): string { throw 'not implemented'; } diff --git a/tools/public_api_guard/forms/forms.d.ts b/tools/public_api_guard/forms/forms.d.ts index 888e2d4566..889a1fc264 100644 --- a/tools/public_api_guard/forms/forms.d.ts +++ b/tools/public_api_guard/forms/forms.d.ts @@ -121,6 +121,9 @@ export declare class CheckboxRequiredValidator extends RequiredValidator { validate(c: AbstractControl): ValidationErrors | null; } +/** @experimental */ +export declare const COMPOSITION_BUFFER_MODE: InjectionToken; + /** @stable */ export declare class ControlContainer extends AbstractControlDirective { readonly formDirective: Form; @@ -140,7 +143,10 @@ export interface ControlValueAccessor { export declare class DefaultValueAccessor implements ControlValueAccessor { onChange: (_: any) => void; onTouched: () => void; - constructor(_renderer: Renderer, _elementRef: ElementRef); + constructor(_renderer: Renderer, _elementRef: ElementRef, _compositionMode: boolean); + _compositionEnd(value: any): void; + _compositionStart(): void; + _handleInput(value: any): void; registerOnChange(fn: (_: any) => void): void; registerOnTouched(fn: () => void): void; setDisabledState(isDisabled: boolean): void; @@ -426,8 +432,6 @@ export declare class NgModel extends NgControl implements OnChanges, OnDestroy { readonly validator: ValidatorFn; viewModel: any; constructor(parent: ControlContainer, validators: Array, asyncValidators: Array, valueAccessors: ControlValueAccessor[]); - compositionEnd(): void; - compositionStart(): void; ngOnChanges(changes: SimpleChanges): void; ngOnDestroy(): void; viewToModelUpdate(newValue: any): void;