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

Focus: Fix focus jumps when splitting a text block #480

Merged
merged 1 commit into from
Apr 24, 2017

Conversation

youknowriad
Copy link
Contributor

We have to be very careful while dealing the with the different TinyMCE events. Triggering unnecessary events can make the focus jump to other blocks.

Testing instructions

  • Split a text block (double enter)
  • Make sure the focus is correctly set to the beginning of the newly created block.

@youknowriad youknowriad added Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Bug An existing feature does not function as intended labels Apr 21, 2017
@youknowriad youknowriad self-assigned this Apr 21, 2017
@youknowriad youknowriad requested a review from nylen April 21, 2017 10:34
@nylen
Copy link
Member

nylen commented Apr 21, 2017

Just the setTimeout for me is enough to resolve the focus jumping issues you described in the PR text. What specific issues does the save() call prevent?

Neither one of these fixes feels like a great solution to me; it would be better to find the specific event(s) that cause the focus to jump, and then remove/suppress those.

@youknowriad
Copy link
Contributor Author

youknowriad commented Apr 21, 2017

@nylen I know these are not ideal but it's really hard to solve.

1- the setTimeout fix the side-effect introduced by theSTART_TYPING event. This event is triggered

onKeyDown={ onStartTyping }
and when we split a text block this event is triggered right after the split happens which causes the focus to jump back to the original block event if the focus has changed before, when inserting the newly created block. The setTimeout ensures the START_TYPING event happened before splitting the content.

2- the save: I think this is a good change no matter the fact that it fixes an issue here. Because logically if we just receive a prop indicating that the content of TinyMCE is some value, we set the content in TinyMCE and we tell him, this value is already saved, so its status is not dirty. This happens to fix a focus jumping bug (which does not occur systematically). I'm seeing it especially when the block we're splitting is an empty text and it's really hard to track.
Here's the workflow: When we split the text, we trigger INSERT and UPDATE_BLOCK to update the current block content, the insert switches the focus to the currently inserted block, which, in the same time, triggers another UPDATE_BLOCK for the original block because we focused out of this block and I don't know exactly why, this second UPDATE_BLOCK is responsible of focusing the original editable again (I couldn't find why exactly, maybe there's another bug here while UPDATE_BLOCK triggers a focus on the editable, but regardless, the fix is logic for me).

Copy link
Member

@nylen nylen left a comment

Choose a reason for hiding this comment

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

I see, thanks for explaining. I think we should merge it

@youknowriad youknowriad merged commit 53b8417 into master Apr 24, 2017
@youknowriad youknowriad deleted the fix/focus-jump branch April 24, 2017 07:53
@aduth
Copy link
Member

aduth commented Apr 24, 2017

I don't know if it was caused by or meant to be addressed by the changes here, but there's still a noticeable flicker of selected block on double-enter even though the focus is now correctly moved to the second block.

flicker

@youknowriad
Copy link
Contributor Author

@aduth I already noticed this, but it was not the intent of this PR to fix this bug.

It's caused by the fact that we're triggering two consecutive Redux Actions which triggers two rerenders. I was thinking of batching redux subscribtions to resolve this in a transparent way. https://github.com/tappleby/redux-batched-subscribe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants