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

TinyMCE: fix IE11 lost focus after init #12206

Merged
merged 2 commits into from Nov 22, 2018

Conversation

Projects
None yet
3 participants
@iseulde
Member

iseulde commented Nov 22, 2018

Description

Fixes #12113.

I'm not sure what is causing the focus loss. An easy fix is to just set it back if focus is gone.

How has this been tested?

See #12113.

Screenshots

Types of changes

Bug fix.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@iseulde iseulde added this to the 4.6 milestone Nov 22, 2018

@iseulde iseulde requested review from jasmussen and WordPress/gutenberg-core Nov 22, 2018

@jasmussen

Hoooray! I can confirm this PR fixes #12113 and fixes #12181.

ie11

I would approve, but I would appreciate a sanity check by a better JS developer than myself.

AMAZING WORK. Thank you.

@iseulde

This comment has been minimized.

Member

iseulde commented Nov 22, 2018

Unfortunately we now have failing e2e test, which is was expecting a bit. The reason for this is that the user might already have move the focus in the meantime, and we'd still be setting back focus.

Thinking...

I'm slightly tempted to just not initialise TinyMCE in IE11.

@iseulde

This comment has been minimized.

Member

iseulde commented Nov 22, 2018

For some reason focus moves to:

<div tabindex="0" class="wp-block editor-block-list__block is-typing" id="block-d7cdbeb7-cdab-4e94-8539-1fa71d1463f1" aria-label="Block: Paragraph" data-type="core/paragraph">...</div>

So the tab-able element seems to capture focus?

@iseulde

This comment has been minimized.

Member

iseulde commented Nov 22, 2018

To reproduce the bug this PR would be causing: hold enter to create a lot of paragraphs (in any browser). You'll see that focus unexpectedly moves to an earlier created paragraph.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 22, 2018

@iseulde The issue might be that the paragraph RichText never gets focused? It could be something in focusTabbable in block.js

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 22, 2018

otherwise I'd be fine with this fix but only in IE11 as the issue it creates is less critical than the one it solves for IE

@iseulde

This comment has been minimized.

Member

iseulde commented Nov 22, 2018

The issue might be that the paragraph RichText never gets focused? It could be something in focusTabbable in block.js

No, it does receive focus. This PR checks for it, to then set it back. If you don't init TinyMCE at all, everything works. Something happens during TinyMCE init that moves away the focus to the block.

@iseulde

This comment has been minimized.

Member

iseulde commented Nov 22, 2018

@jasmussen Does the latest commit work for you?

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 22, 2018

Testing now.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 22, 2018

This works well when pressing Enter. But sadly tabbing took a downturn. It doesn't work as it should:

ie11

@iseulde

This comment has been minimized.

Member

iseulde commented Nov 22, 2018

😭

@iseulde iseulde force-pushed the fix/tinymce-ie11-focus branch from 0249ca8 to 837903e Nov 22, 2018

@iseulde

This comment has been minimized.

Member

iseulde commented Nov 22, 2018

@jasmussen How about now (force pushed)?

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 22, 2018

Trying.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 22, 2018

I'm happy to report this seems to work exactly as it should to me! Very nice work.

ie11

// TinyMCE, so we have to set it back.
if (
document.activeElement !== this.editorNode &&
document.activeElement.contains( this.editorNode )

This comment has been minimized.

@youknowriad

youknowriad Nov 22, 2018

Contributor

This is a bit concerning. I wonder for example how this works for quotes where we have two RichTexts that might be inside the selection. Can't we check the isFocused element before the init call and only focus if it's the current one?

This comment has been minimized.

@iseulde

iseulde Nov 22, 2018

Member

I'm not sure what you mean. Could you elaborate?

This comment has been minimized.

@iseulde

iseulde Nov 22, 2018

Member

This commit is more restrictive than the last.

This comment has been minimized.

@youknowriad

youknowriad Nov 22, 2018

Contributor

I'm suggesting doing this before the init call.

const wasFocused = document.activeElement === this.editorNode

And changing this condition to

wasFocused &&
document.activeElement !== this.editorNode &&
document.activeElement.contains( this.editorNode )

This comment has been minimized.

@youknowriad

youknowriad Nov 22, 2018

Contributor

Basically making it a little bit more strict

This comment has been minimized.

@youknowriad

youknowriad Nov 22, 2018

Contributor

You're totally right :) I forgot that part.

It's still a bit concerning that if we move the focus quickly between two RichTexts in the same block (quote block for instance) while the initialization is in progress, we'll revert the focus. But I guess it's very rare.

This comment has been minimized.

@iseulde

iseulde Nov 22, 2018

Member

Yes, that's concerning to me too, which is why I tried some other things to fix it. Don't know what would work though. The best thing would be to figure out why focus is lost and prevent that. Another thing we can do is just not initialise TinyMCE in IE11, but perhaps it's better to wait a bit with that.

This comment has been minimized.

@youknowriad

youknowriad Nov 22, 2018

Contributor

CAn we test that we're just in IE11 to do that?

This comment has been minimized.

@iseulde

iseulde Nov 22, 2018

Member

Yeah, we can detect IE and not initialise. But maybe it's better to wait for the list commands (indent and change type) to work with the rich text value, otherwise they won't work.

This comment has been minimized.

@youknowriad

youknowriad Nov 22, 2018

Contributor

I was thinking detect if IE and apply the current patch only in IE. (still initialize the editor) just to make sure we avoid side effects in other browsers.

@iseulde iseulde merged commit 9cc03c0 into master Nov 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@iseulde iseulde deleted the fix/tinymce-ie11-focus branch Nov 22, 2018

@iseulde iseulde referenced this pull request Nov 22, 2018

Merged

Add IE check for IE fix #12234

4 of 4 tasks complete

youknowriad added a commit that referenced this pull request Nov 29, 2018

TinyMCE: fix IE11 lost focus after init (#12206)
* TinyMCE: fix IE11 lost focus after init

* Attempt 3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment