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

Delay TinyMCE initialisation to focus #10723

Merged
merged 8 commits into from Oct 30, 2018

Conversation

Projects
None yet
8 participants
@iseulde
Member

iseulde commented Oct 18, 2018

Description

This PR is similar to #9040, with the difference that it does not destroy TinyMCE on blur. It merely delays TinyMCE initialisation to the point where the user focuses the field.

How has this been tested?

Screenshots

Types of changes

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 changed the title from Delay TinyMCE initialisat to focus to Delay TinyMCE initialisation to focus Oct 18, 2018

@iseulde iseulde force-pushed the try/mce-delay-init branch from b756ec9 to c6903c1 Oct 18, 2018

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Oct 18, 2018

Sounds like a safer approach to start with. I have a question:

What happens if we try to hit the formatting toolbar before initialization (should we try to trigger the initialization explicitely when we apply a format for instance?)

@iseulde

This comment has been minimized.

Member

iseulde commented Oct 18, 2018

What happens if we try to hit the formatting toolbar before initialization (should we try to trigger the initialization explicitely when we apply a format for instance?)

The format bar only appears when the editor is initialised. This is not needed though, as we no longer need TinyMCE to apply formats. So we can just show it without MCE being initialised.

@iseulde

This comment has been minimized.

Member

iseulde commented Oct 18, 2018

@youknowriad Atm it is needed that the toolbar only initialises after TinyMCE because we're registering shortcuts though TinyMCE. After the format API PR this dependency disappears.

@gziolo gziolo requested a review from aduth Oct 18, 2018

@iseulde

This comment has been minimized.

Member

iseulde commented Oct 18, 2018

There are some failing tests. Unsure why those specifically atm. I was actually expecting more failures.

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 18, 2018

There is one issue though, try to click on bold, anchor or italic in the middle of RichText. I don't know why but cursor moves sometimes to the start of RichText field. When I click on the regular text it all works properly.

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 18, 2018

I was wondering if we can initialize all TinyMCE instances inside a selected block, and destroy them when you deselect block. Maybe pre-initialize on hover if the issue I mentioned is still the case.

@iseulde

This comment has been minimized.

Member

iseulde commented Oct 18, 2018

pre-initialize

What's that?

I was wondering if we can initialize all TinyMCE instances inside a selected block, and destroy them when you deselect block.

That's exactly what #9040 tries to do. I think this approach is a bit less extreme as a first step.

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 18, 2018

pre-initialize

What's that?

It doesn't matter anymore since you fixed it :)

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Oct 18, 2018

The format bar only appears when the editor is initialised. This is not needed though, as we no longer need TinyMCE to apply formats. So we can just show it without MCE being initialised.

That's not true though. The format toolbar can be visible but the editor not focused (which means TinyMCE not initialized).

So we can just show it without MCE being initialised.

Yes, but if I remember properly we used to avoid rerendering the TinyMCE component which means if the content changes externally (by applying a format or something else), it's not refreshed. (Maybe this changed, I didn't follow everything)

@iseulde

This comment has been minimized.

Member

iseulde commented Oct 18, 2018

That's not true though.

That's not true anymore. See #10723 (comment).

@mcsf

Looks good overall.

Note: when starting a fresh Gutenberg session, I can feel the latency upon the first focusing of a block. Subsequent focus switches don't lead to any noticeable difference. I think the overall benefit of this PR clearly outweighs this.

@youknowriad youknowriad added this to the 4.2 milestone Oct 29, 2018

@youknowriad youknowriad force-pushed the try/mce-delay-init branch from ace7b50 to 8ff5fe5 Oct 29, 2018

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Oct 29, 2018

I added this in 4.2 because I think it's very important to get in beta. At least we can test it a little bit before RC.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Oct 29, 2018

I rebased and pushed the new snapshots for the unit tests as they seemed legit to me.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Oct 29, 2018

In dc5a085 I update the quote => paragraph transform and the test. I think "focusing RichText" initializes TinyMCE which probably produces an onChange call cleaning empty paragraphs so this slightly impacts the tests where <p></p> should be considered empty. I don't think it's very important so I updated the code and the tests but let me know if you think differently.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Oct 29, 2018

Here's the status of the two remaining failures in e2e tests:

  • Block Deletion: Can't reproduce locally, I suspect it's intermittent
  • Splitting and Merging: I think the breakage are probably legit, If I use SLOWMO in the test, it passes though and If I try manually, I can't reproduce the breakage. It's more likely a "timing" issue since now the initialization happens a bit later than before. I'm not certain we can do something about. We should see if we're fine with the breakage and introduce wait or waitForSomething in the tests. and if we're not fine with the breakage, maybe this approach of initializing TinyMCE on Focus is not viable.

Thoughs?

@@ -108,7 +108,7 @@ export const settings = {
blocks: [ 'core/paragraph' ],
transform: ( { value, citation } ) => {
const paragraphs = [];
if ( value ) {
if ( value && value !== '<p></p>' ) {

This comment has been minimized.

@aduth

aduth Oct 29, 2018

Member

I recall a discussion about this being non-ideal. Is this a "gotta get worse before getting better" thing? Do we have an issue to track the improvements?

This comment has been minimized.

@iseulde

iseulde Oct 29, 2018

Member

See #10763.

This comment has been minimized.

@iseulde

iseulde Oct 30, 2018

Member

@youknowriad After the enter "fix", do we still need these changes?

This comment has been minimized.

@youknowriad

youknowriad Oct 30, 2018

Contributor

yes, I think we do, that's something else I think. In this case we don't even focus the editor so the value is not changed.

@iseulde

This comment has been minimized.

Member

iseulde commented Oct 30, 2018

Slightly related PR, which may help alleviate issue here: #10328. It would move keyup and keydown event handlers from TinyMCE to the contentEditable element, so they take effect immediately. With delaying TinyMCE init, we may miss catching an early event?

Alternative might be to set a class at the end of TinyMCE init and check for that in the e2e test, instead of just the normal TinyMCE classes. Unsure at which stage those are added.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Oct 30, 2018

@iseulde Both these two things seem reasonable to me.

Slightly related PR, which may help alleviate issue here: #10328. It would move keyup and keydown event handlers from TinyMCE to the contentEditable element, so they take effect immediately. With delaying TinyMCE init, we may miss catching an early event?

If this fixes, the "delay" issue, it would be awesome.

Alternative might be to set a class at the end of TinyMCE init and check for that in the e2e test, instead of just the normal TinyMCE classes. Unsure at which stage those are added.

Seems like a good thing to avoid relying on TinyMCE classnames in the tests.


Even If I'd love to get this in ASAP, It does feel like we're not ready there to ship for 4.2 though, so thinking we should punt but make sure we follow-up quickly on it?

@iseulde iseulde force-pushed the try/mce-delay-init branch from dc5a085 to 783d168 Oct 30, 2018

@iseulde iseulde force-pushed the try/mce-delay-init branch from 783d168 to b5a36c9 Oct 30, 2018

@youknowriad youknowriad force-pushed the try/mce-delay-init branch from a751275 to 4cc3a13 Oct 30, 2018

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Oct 30, 2018

So I found a way to fix the issue: Call waitForRichTextInitialization after each Enter press creating a new paragraph/block.

This fix is a bug actually: A bug happening when you type really really fast. It's also a bug we already have even without this PR which results in inconsistent tests (splitting-merging anyone?).

So this PR makes this bug more "visible" because we delayed the initialization a bit. But at the moment I'm thinking the advantages of this PR are bigger than this bug and the removal of waitForRichTextInitialization should be addressed separately.

@iseulde

This comment has been minimized.

Member

iseulde commented Oct 30, 2018

See #6021. I can try to address it as soon as possible after this release. I think it's okay to merge this and work on a fix for enter separately.

@youknowriad

LGTM 👍

@youknowriad youknowriad merged commit 35c02bc into master Oct 30, 2018

2 checks passed

codecov/project 48.59% (-0.01%) compared to 9d46596
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the try/mce-delay-init branch Oct 30, 2018

@aduth

This comment has been minimized.

Member

aduth commented Oct 30, 2018

with the difference that it does not destroy TinyMCE on blur.

Was there a concern with this? It seemed to me one of the safer steps of this approach in #9040.

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 30, 2018

As far as I remember we discussed that we should take one step at the time. We should continue exploring destroying on blur - it might make sense to defer it until block gets unselected so you could retain fast interactions between multiple instances of RichText inside one block - use cases are navigating between content and cite of Quote block, or between table rows in Table block.

@talldan talldan referenced this pull request Nov 1, 2018

Merged

Fix rich text value for nested lists #10799

4 of 4 tasks complete
@talldan

This comment has been minimized.

Contributor

talldan commented Nov 1, 2018

This PR seems to have resulted in this bug:
#11348

Looking into it now, seems a pretty urgent one to fix.

The bug seems to be caused by focus not triggering reliably in the table cells, causing them to not be selected. When a table cell doesn't have selection the onChange does nothing and values in the table aren't updated:

onChange( content ) {
const { selectedCell } = this.state;
if ( ! selectedCell ) {
return;
}
const { attributes, setAttributes } = this.props;
const { section, rowIndex, columnIndex } = selectedCell;
setAttributes( updateCellContent( attributes, {
section,
rowIndex,
columnIndex,
content,
} ) );
}

Now I know what the bug is, I'm looking into the exact cause and what the best fix would be.

@talldan

This comment has been minimized.

Contributor

talldan commented Nov 1, 2018

My best guess right now is a race condition. Perhaps since Initialisation of TinyMCE now happens onFocus:
https://github.com/WordPress/gutenberg/pull/10723/files#diff-de07a4aefea1cc394e50163dd259882fR108

The editor event isn't always registered in time:

editor.on( 'focus', this.onFocus );

@iseulde

This comment has been minimized.

Member

iseulde commented Nov 1, 2018

@talldan Would #11287 fix it? Looking in a bit.

@talldan

This comment has been minimized.

Contributor

talldan commented Nov 1, 2018

I'll have a test of that - I've got a fix here as well that seems to do the trick. Not sure if there's a drawback to using the react event instead of the editor event:
https://github.com/WordPress/gutenberg/compare/fix/table-cell-focus-issue?expand=1

@talldan

This comment has been minimized.

Contributor

talldan commented Nov 1, 2018

Yep, looks like #11287 contains exactly the same fix. Confirmed it resolves the issue.

@iseulde

This comment has been minimized.

Member

iseulde commented Nov 1, 2018

Yes, it moves the keydown and keyup handlers as well. :)

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Nov 21, 2018

This PR appears to have caused #12113.

My method of figuring this out was kind of manual — I simply checked out commits to master, and got to this page: https://github.com/WordPress/gutenberg/commits/master?before=01be7ac89b97b76c490d57a15c16466657240770+371

screenshot 2018-11-21 at 12 01 24

☝️ "Port nextpage block to the ReactNative mobile app" is the last commit where the Enter key worked perfectly in IE11.

The regression as noted in #12113 appears right in the next commit, i.e. this PR.

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