fix(input[text]): composing function dosen't works properly in IE #16273

jileeon · 2017-10-13T06:47:49Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Bug fix

What is the current behavior? (You can also link to an open issue here)

Composing function dosen't works properly in IE. Please refer to #6656 and "Other information" below.

What is the new behavior (if this is a feature change)?

It works properly in IE.

Does this PR introduce a breaking change?

No.

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:

I have investigated on #6656 issue and I found composing function works differently in only IE.
I confirmed this by the scenario below.

Code changes to investigate this problem(just added console.log)
Scenario

Just type '이재익' and focusout.(in IE and Chrome)

Result(attached images show differences)
console.log in Chrome

console.log in IE

It shows that last values of $viewValue are different. '이재이'(IE) != '이재익'(Chrome)
Because 'compositionend' event is not occurred in IE. So $viewValue is not updated with last value in the textbox. It means that IE triggers composition events('compositionstart', 'compositionupdate', 'compositionend') wrong.
So I added additional condition(ev.originalEvent.data !== undefined) on 'if (composing) return;' statement.
As you saw these console logs(IE and Chrome) above, this additional condition makes composing function works properly in IE.(by checking ev.originalEvent.data value. if its value is undefined, $viewValue going to be updated with last value in the textbox in IE. So it works like 'compositionend').
In addition, When IE fixes this wrong event triggering, It works fine without any problems.

googlebot · 2017-10-13T06:47:55Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

googlebot · 2017-10-13T06:47:55Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

jileeon · 2017-10-13T07:37:55Z

I signed it!

googlebot · 2017-10-13T07:37:59Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

googlebot · 2017-10-13T07:37:59Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

jileeon · 2017-10-13T07:42:03Z

I signed it!

googlebot · 2017-10-13T07:50:18Z

CLAs look good, thanks!

googlebot · 2017-10-13T07:50:18Z

CLAs look good, thanks!

gkalpak · 2017-10-13T09:06:08Z

src/ng/directive/input.js

@@ -1261,7 +1261,7 @@ function baseInputType(scope, element, attr, ctrl, $sniffer, $browser) {
      $browser.defer.cancel(timeout);
      timeout = null;
    }
-    if (composing) return;
+    if (composing && ev.originalEvent.data !== undefined) return;


So, should composing be set to false if composing $$ ev.originalEvent.data === undefined?
Also, where does originalEvent come from?

That's right. Because, In IE, 'compositionupdate' event still occurred although an user focused out of text input. and originalEvent comes from this text input.

gkalpak · 2017-10-13T09:09:58Z

I guess we are not testing composition events, are we?

Narretz · 2017-10-13T09:34:25Z

We should be testing this. But I wonder if it's possible in a unit test.

Narretz · 2017-10-13T13:56:39Z

A few of the input tests fail with this change - related to the use of originalEvent: https://travis-ci.org/angular/angular.js/jobs/287472115

jileeon · 2018-01-11T13:52:07Z

@Narretz, I've rebased it on master.

Thanks.

googlebot · 2018-01-12T12:31:34Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2018-01-12T12:31:34Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

Narretz · 2018-01-12T12:41:28Z

I've added a test for this behavior.
If @gkalpak can give review the PR again, we should be good to go :)

jileeon · 2018-01-12T15:44:03Z

@Narretz, Thank you for improving my comment and your effort.

gkalpak · 2018-01-25T11:25:22Z

LGTM except for the failing Travis tests 😥

jileeon · 2018-01-25T13:04:49Z

@gkalpak FYI, I've rebased this on master.

Narretz · 2018-01-25T13:46:50Z

The problem isn't solved by rebasing unfortunately. There's a problem with the composition events when jquery is active,

jileeon · 2018-01-25T14:53:48Z

@Narretz, I ran 'should end composition on "compositionupdate" when event.data is ""' test. and I found compositionupdate event wasn't fired. So we have to change test code to trigger compositionupdate event. Unfortunately I don't know why.

Narretz · 2018-01-25T15:17:50Z

Did you run the tests with jquery? As you can see the tests work with jqlite. I suspect that browserTrigger works differently when jquery is included. We cannot simply change the tests so they pass, we must find the root cause

jileeon · 2018-01-25T15:38:41Z

@Narretz Sure I did(run with karma-jquery.conf.js). I hope we will find the root cause as soon as possible.

gkalpak · 2018-01-26T10:24:37Z

src/ng/directive/input.js

+      // End composition when ev.data is empty string on 'compositionupdate' event.
+      // When the input de-focusses (e.g. by clicking away), IE triggers 'compositionupdate'
+      // instead of 'compositionend'.
+      if (ev.data === '') composing = false;


Setting composing to false isn't enough. You also need to call the listener.
Maybe add a check here and in compositionend listener below, to only call listener() if composing was true before (to avoid calling it twice on browsers that may trigger both events - or is that impossible?)

Well, I've tested it in a real world scenario and it is enough, because IE also fires the change event afterwards.
We could still implement your suggestion. It might help with another IE issue: #13617

I see. Makes sense. Any idea why tests fail on Travis then?

Narretz · 2018-01-29T13:14:48Z

It appears that jquery sets the event.data property to undefined when it is explicitly set to empty string, at least for compositionupdate with dispatchEvent. @mgol do you know if this is expected behavior?

jileeon · 2018-01-30T01:00:25Z

@Narretz, You found the root cause. and Now I see this pull request is approved.

Thank You

Fixes angular#6656 Closes angular#16273

Fixes #6656 Closes #16273

googlebot added the cla: no label Oct 13, 2017

jileeon force-pushed the fix-composing-doesnt-work-properly-in-IE branch from 00f14a7 to 2db08fd Compare October 13, 2017 07:50

googlebot added cla: yes and removed cla: no labels Oct 13, 2017

jileeon mentioned this pull request Oct 13, 2017

textarea ng-model is not updated in IE with Korean IME. #6656

Closed

jileeon changed the title ~~fix(input): Composing function dosen't works properly in IE~~ fix(input): composing function dosen't works properly in IE Oct 13, 2017

jileeon force-pushed the fix-composing-doesnt-work-properly-in-IE branch from 2db08fd to 4528eec Compare October 13, 2017 09:07

gkalpak suggested changes Oct 13, 2017

View reviewed changes

gkalpak added browser: IE10 browser: IE11 browser: IE9 component: forms needs: work type: bug labels Oct 13, 2017

jileeon changed the title ~~fix(input): composing function dosen't works properly in IE~~ fix(input[text]): composing function dosen't works properly in IE Oct 13, 2017

gkalpak added this to the Backlog milestone Oct 13, 2017

Narretz added the needs: test label Oct 13, 2017

jileeon force-pushed the fix-composing-doesnt-work-properly-in-IE branch 2 times, most recently from 6d0b4f3 to 11b72b8 Compare October 13, 2017 15:30

jileeon force-pushed the fix-composing-doesnt-work-properly-in-IE branch from c058994 to b59f62d Compare January 11, 2018 13:48

googlebot added cla: no and removed cla: yes labels Jan 12, 2018

Narretz force-pushed the fix-composing-doesnt-work-properly-in-IE branch from 0fba027 to 7cd75e9 Compare January 12, 2018 16:42

jileeon force-pushed the fix-composing-doesnt-work-properly-in-IE branch from 7cd75e9 to 4a5283b Compare January 25, 2018 12:58

gkalpak reviewed Jan 26, 2018

View reviewed changes

gkalpak approved these changes Jan 29, 2018

View reviewed changes

Narretz and others added 3 commits February 8, 2018 18:02

fix(browserTrigger): support CompositionEvent

b88b283

fix(input): fix composition mode in IE for Korean input

f31a1e2

Fixes angular#6656 Closes angular#16273

test(input): add test for IE composition bug

da79f73

Narretz force-pushed the fix-composing-doesnt-work-properly-in-IE branch from 1f0688e to da79f73 Compare February 8, 2018 17:48

Narretz merged commit c484213 into angular:master Feb 8, 2018

Narretz pushed a commit that referenced this pull request Feb 8, 2018

fix(input): fix composition mode in IE for Korean input

2789ccb

Fixes #6656 Closes #16273

Narretz pushed a commit that referenced this pull request Feb 8, 2018

fix(input): fix composition mode in IE for Korean input

9a1b7c9

Fixes #6656 Closes #16273

jileeon deleted the fix-composing-doesnt-work-properly-in-IE branch February 9, 2018 01:49

This was referenced Apr 18, 2018

Update angular in package.json from 1.6.9 to 1.6.10 ui-router/sample-app-angular-hybrid#69

Closed

Update 4 dependencies from npm JetBrains/ring-ui#226

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(input[text]): composing function dosen't works properly in IE #16273

fix(input[text]): composing function dosen't works properly in IE #16273

jileeon commented Oct 13, 2017 •

edited

Loading

googlebot commented Oct 13, 2017

googlebot commented Oct 13, 2017

jileeon commented Oct 13, 2017

googlebot commented Oct 13, 2017

googlebot commented Oct 13, 2017

jileeon commented Oct 13, 2017

googlebot commented Oct 13, 2017

googlebot commented Oct 13, 2017

gkalpak Oct 13, 2017

jileeon Oct 13, 2017

gkalpak commented Oct 13, 2017

Narretz commented Oct 13, 2017

Narretz commented Oct 13, 2017

jileeon commented Jan 11, 2018

googlebot commented Jan 12, 2018

googlebot commented Jan 12, 2018

Narretz commented Jan 12, 2018

jileeon commented Jan 12, 2018

gkalpak commented Jan 25, 2018

jileeon commented Jan 25, 2018

Narretz commented Jan 25, 2018

jileeon commented Jan 25, 2018 •

edited

Loading

Narretz commented Jan 25, 2018

jileeon commented Jan 25, 2018

gkalpak Jan 26, 2018

Narretz Jan 26, 2018

gkalpak Jan 26, 2018

Narretz commented Jan 29, 2018

jileeon commented Jan 30, 2018 •

edited

Loading

fix(input[text]): composing function dosen't works properly in IE #16273

fix(input[text]): composing function dosen't works properly in IE #16273

Conversation

jileeon commented Oct 13, 2017 • edited Loading

googlebot commented Oct 13, 2017

googlebot commented Oct 13, 2017

jileeon commented Oct 13, 2017

googlebot commented Oct 13, 2017

googlebot commented Oct 13, 2017

jileeon commented Oct 13, 2017

googlebot commented Oct 13, 2017

googlebot commented Oct 13, 2017

gkalpak Oct 13, 2017

Choose a reason for hiding this comment

jileeon Oct 13, 2017

Choose a reason for hiding this comment

gkalpak commented Oct 13, 2017

Narretz commented Oct 13, 2017

Narretz commented Oct 13, 2017

jileeon commented Jan 11, 2018

googlebot commented Jan 12, 2018

googlebot commented Jan 12, 2018

Narretz commented Jan 12, 2018

jileeon commented Jan 12, 2018

gkalpak commented Jan 25, 2018

jileeon commented Jan 25, 2018

Narretz commented Jan 25, 2018

jileeon commented Jan 25, 2018 • edited Loading

Narretz commented Jan 25, 2018

jileeon commented Jan 25, 2018

gkalpak Jan 26, 2018

Choose a reason for hiding this comment

Narretz Jan 26, 2018

Choose a reason for hiding this comment

gkalpak Jan 26, 2018

Choose a reason for hiding this comment

Narretz commented Jan 29, 2018

jileeon commented Jan 30, 2018 • edited Loading

jileeon commented Oct 13, 2017 •

edited

Loading

jileeon commented Jan 25, 2018 •

edited

Loading

jileeon commented Jan 30, 2018 •

edited

Loading