Skip to content
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

Microsoft Edge - easily enters a confused state via ctrl-a + keypress and crashes #7270

Closed
wants to merge 1 commit into from

Conversation

butchmarshall
Copy link

Microsoft Edge exposes TextInput event, but it's not useful either.

See opened issue here

Microsoft Edge exposes TextInput event, but it's not useful either.
@ghost
Copy link

ghost commented Jul 13, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

@ghost ghost added the CLA Signed label Jul 13, 2016
@butchmarshall
Copy link
Author

butchmarshall commented Jul 13, 2016

I should be more clear about TextEvent issue in Edge.

This issue is caused because "TextEvent" in Edge fires twice in the final key sequence. There is an extra TextEvent with data="" if "ctrl-a" is used to make the selection after "j" is pressed. Only ctrl-a. It's very strange.

Edge

type a (TextEvent data="a")
type return
type ctrl-a
type j (TextEvent data="")(TextEvent data="j")

Chrome

type a (TextEvent data="a")
type return
type ctrl-a
type j (TextEvent data="j")

Bypassing TextEvent support seemed to be the easiest fix. I didn't want to go into why "ctrl-a" was firing a blank event. Even more strange is if you do "shift-arrow-up" to select the text instead, you don't get the double text-events fired ((TextEvent data="")(TextEvent data="j")) but instead what is expected (just (TextEvent data="a"))

@butchmarshall butchmarshall changed the title Issue #7269 Fix #7269 Jul 13, 2016
@ghost
Copy link

ghost commented Jul 13, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@ghost ghost added the CLA Signed label Jul 13, 2016
@butchmarshall butchmarshall changed the title Fix #7269 Microsoft Edge - easily enters a confused state via ctrl-a + keypress and crashes Jul 19, 2016
@ghost ghost added the CLA Signed label Jul 19, 2016
@sophiebits
Copy link
Collaborator

@msmania Any idea what's going on here or if this fix seems reasonable? I am hesitant to add UA tests without a version bound.

@ghost ghost added the CLA Signed label Jul 21, 2016
@msmania
Copy link
Contributor

msmania commented Aug 3, 2016

Sorry for the late response.. I agree with @spicyj because there are known issues in the fallback logic, especially in IME-related scenarios such as #7107 .
I noticed Edge in the latest Win10 (= Anniversary Update; released today) did not repro this issue. I still see an event of textInput with an empty string, but no JavaScript errors mentioned in facebook/draft-js#501.
Let me look into this further and see what we can do for other platforms.

@ghost ghost added the CLA Signed label Aug 3, 2016
@ghost ghost added the CLA Signed label Aug 3, 2016
@msmania
Copy link
Contributor

msmania commented Aug 13, 2016

I confirmed the JS exception 'NotFoundError' was caused by incorrect DOM operation in Edge, and it was irrespectively fixed in the Anniversary Update to address another DOM issue.

@butchmarshall can you please install the Anniversary Update and check the behavior?

Given that the latest Edge does not have this issue, I'd like to keep React using textInput events for Edge, instead of the fallback logic.

I tried to apply the same fix to Windows 10 1511, but unfortunately it cannot be a simple backport due to refactoring and design changes. You can report this issue here, but because there is higher risk, I'm afraid that this single case would not rise to the bar needed for backporting.

@sophiebits
Copy link
Collaborator

Can we add a UA test for older versions of Edge? @msmania Maybe you can suggest what version strings we can look for?

@ghost ghost added the CLA Signed label Aug 13, 2016
@butchmarshall
Copy link
Author

It's great to see Microsoft fixing these issues! Thanks for keeping this bug up to date everyone.

@aweary
Copy link
Contributor

aweary commented Jan 26, 2017

Ping @msmania, is there a recommended version string we can add here to catch only releases with this issue?

@msmania
Copy link
Contributor

msmania commented Feb 1, 2017

@aweary, Sorry for not responding for long. It's a good idea to detect Edge's version from UA, but using the fallback logic in older Edge still looks risky to me.

I remember the root cause of this issue was that Edge removed an element which should not be removed. After that, when React tries to remove that element which does no longer exist, an exception occurs, then React enters a confused state. So the workaround would be to handle such an unexpected DOM operation.

Anyway, Edge's UA string format is described here, and the build versions of Win10 family can be found here. Either "EdgeVersion >= 14" or "Win10Build >= 14393" is good for this purpose.

Below is an example of UA string from Edge in Win10 1607 and 1511:

<Win10 1607>
Mozilla/5.0 (Windows NT 10.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.79 Safari/537.36 Edge/14.14393

<Win10 1511>
Mozilla/5.0 (Windows NT 10.0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2486.0 Safari/537.36 Edge/13.10586

@gaearon
Copy link
Collaborator

gaearon commented Jan 3, 2018

Seems like this got fixed in Edge.
#7269 (comment)

@gaearon gaearon closed this Jan 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants