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

Fix CJK-related crashes after tab character #1171

Merged
merged 2 commits into from
Jan 24, 2019
Merged

Fix CJK-related crashes after tab character #1171

merged 2 commits into from
Jan 24, 2019

Conversation

mirka
Copy link
Member

@mirka mirka commented Jan 23, 2019

Closes #780

Update: This seems to have been a bug in the Chrome version in Electron 2. It doesn't reproduce at all in Electron >3.0.0. So this workaround should be unnecessary once we update.

This is a really unsatisfying workaround that doesn't address any root causes and I don't like it 😐I first suspected a focus management bug in our indentCurrentBlock() code, but as far as I could tell that doesn't seem to be the case here. Hopefully we can dig a little deeper into the issue sometime in the future, but since this is a pretty serious bug for our CJK users this workaround should help in the meantime.

To test

Refer to this comment for repro steps: #780 (comment)

Note that the original bug only applies to Electron, and not Chrome/Firefox/Safari.

@mirka mirka added this to the 1.4.0 milestone Jan 23, 2019
@mirka mirka requested a review from roundhill January 23, 2019 20:57
@roundhill
Copy link
Contributor

Pretty sure there's a bug with this where characters get duplicated, I can't reproduce this in master:

  1. Using Hiragana input, type tab and then kanji.
  2. Press enter.
  3. The phrase is repeated twice, and pressing backspace doesn't work as expected:

tab

@mirka
Copy link
Member Author

mirka commented Jan 24, 2019

Pretty sure there's a bug with this where characters get duplicated, I can't reproduce this in master

Gaaah. I can reproduce it in master. It just manifests a little differently — no immediate input duplication, but the internal focus state and visible caret are definitely out of sync so it glitches once you hit backspace. I can confirm that this issue is also fixed by updating to Electron >3.0.0.

Despite this, I think we should merge in this PR for v1.4.0 as a band-aid while we work to get the Electron update out ASAP. Two reasons:

  • Most users who are using tabs are presumably typing a nested list item, which will trigger the crash rather than the glitch.
  • Glitches are at least better than crashes 😬

Thoughts?

@mirka mirka mentioned this pull request Jan 24, 2019
3 tasks
@roundhill
Copy link
Contributor

Thoughts?

I agree, duplicated content is better than a WSOD. And we'll have it really fixed soon enough with the electron update. :shipit:

*/
const tabDecorator = () => {
const strategy = (contentBlock, callback) => {
const tabRegex = /\t/g;
Copy link
Contributor

Choose a reason for hiding this comment

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

One of the smallest regex I've ever seen ;)

@mirka mirka merged commit 792c900 into master Jan 24, 2019
@mirka mirka deleted the fix/cjk-after-tab branch January 24, 2019 16:20
mirka added a commit that referenced this pull request Jan 24, 2019
mirka added a commit that referenced this pull request Jan 31, 2019
mirka added a commit that referenced this pull request Feb 5, 2019
* Update Electron to 4.0.2

* Adapt to new makeSingleInstance API

* Remove unneeded shortcut for Search Notes

Also conflicting with the Focus Mode shortcut (⌘⇧F) here due to a change in the KeyboardEvent. (Now needs an explicit check for shiftKey)

* Fix Note List navigation shortcuts

* Fix Close Note shortcut (narrow width screens)

* Remove unneeded filter on menu array

Rendered unnecessary by electron/electron#13992

* Update electron-builder and electron-updater

# Conflicts:
#	package.json

* Fix Select All & Copy in Markdown preview

* Remove CJK crash workaround

#1171

* Try electron-builder 20.38.4

* Revert "Remove unneeded filter on menu array"

This reverts commit 90a94e7.

* Revert electron-builder to 20.28.2

* Try electron-builder 20.33.2

* Try electron-builder 20.30.0

* Try electron-builder 20.31.2

* Try electron-builder 20.32.0

* Try electron-builder 20.33.1

* Revert "Try electron-builder 20.33.1"

This reverts commit d834229.
@mirka
Copy link
Member Author

mirka commented Feb 21, 2019

The duplication glitch #1171 (comment) is now fixed in v1.5.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants