Update TinyMCE to the latest version #2787

ellatrix · 2017-09-25T13:23:30Z

Description

This PR updates TinyMCE (4.6.5 to 4.7.1).
Changes: tinymce/tinymce-dist@4.6.5...4.7.1

How Has This Been Tested?

Make sure editable regions work, check things like paste, formatting, block splitting and merging...

Checklist:

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

codecov · 2017-09-25T13:36:53Z

Codecov Report

Merging #2787 into master will increase coverage by 2.58%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master    #2787      +/-   ##
==========================================
+ Coverage   33.93%   36.52%   +2.58%     
==========================================
  Files         193      200       +7     
  Lines        5702     6703    +1001     
  Branches     1000     1255     +255     
==========================================
+ Hits         1935     2448     +513     
- Misses       3187     3566     +379     
- Partials      580      689     +109

Impacted Files	Coverage Δ
blocks/editable/index.js	`10.5% <50%> (ø)`	⬆️
editor/modes/visual-editor/inserter.js	`94.44% <0%> (-5.56%)`	⬇️
blocks/library/gallery/block.js	`9.52% <0%> (-1.59%)`	⬇️
editor/selectors.js	`95.63% <0%> (-1.12%)`	⬇️
editor/actions.js	`37.87% <0%> (-1.02%)`	⬇️
blocks/library/html/index.js	`22.72% <0%> (-0.35%)`	⬇️
blocks/api/factory.js	`89.36% <0%> (-0.3%)`	⬇️
blocks/api/registration.js	`100% <0%> (ø)`	⬆️
editor/modes/visual-editor/index.js	`0% <0%> (ø)`	⬆️
editor/table-of-contents/index.js	`0% <0%> (ø)`	⬆️
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aec63ef...e6b1f48. Read the comment docs.

ellatrix · 2017-09-25T13:44:38Z

There's no good fix for the errors without addressing it in TinyMCE: tinymce/tinymce#3947

aduth · 2017-10-03T12:53:35Z

FYI 4.7.0 was released today:

tinymce/tinymce-dist@188f366

ellatrix · 2017-10-03T12:54:18Z

Yep :)

ellatrix · 2017-10-03T13:20:10Z

Does anyone know why this is a dependency in package.json?

youknowriad · 2017-10-03T13:40:46Z

@iseulde maybe unit tests?

ellatrix · 2017-10-03T16:13:16Z

Am I missing something? https://github.com/WordPress/gutenberg/search?utf8=✓&q=import+tinymce&type=

Is it the Editable component in a test? Why is it not the some TinyMCE file?

ellatrix · 2017-10-03T18:02:33Z

So now the tests have undefined global errors. That's not supposed the run in node...

aduth · 2017-10-03T21:00:47Z

The tests run entirely in Node. Even if the tests don't explicitly import TinyMCE, the files under test may do so (either directly or indirectly). While in the browser the import statements are converted to point to the window global, in the test environment they'd need the dependency to be available in node_modules. There are some issues with globals, notably in that the tests are set up to use JSDOM which is in many ways imperfect and doesn't implement all of the browser APIs. It appears this is now working well after adding to test/setup-globals.js?

ellatrix · 2017-10-04T13:50:35Z

@aduth Yep, adjusted that file. Thanks. I was more wondering why we don't use the NPM module for the browser as well, instead of pulling it from a CDN.

To me this branch looks good to merge.

aduth

One regression I'm noting: The "New Paragraph" placeholder text is showing as a darker color than that of master.

`master`	`update/tinymce`

aduth · 2017-10-04T14:48:25Z

test/setup-globals.js

@@ -19,6 +19,7 @@ global.window.tinyMCEPreInit = {
 };
 window.requestAnimationFrame = setTimeout;
 window.cancelAnimationFrame = clearTimeout;
+window.matchMedia = () => ( {} );


I wonder the extent to which we should try to mimic a proper return value here, i.e. MediaQueryList. This implementation could be problematic if a library using matchMedia attempted to treat the matches property on the return object as a boolean (i.e. false === window.matchMedia( '...' ).matches). Conversely, the value wouldn't be accurate, so we can't provide a useful value anyways. From what I can tell, this should be fine enough for how TinyMCE is using it:

https://github.com/tinymce/tinymce/blob/114e2d6878c58d6fe452db39353cc177f842d8bd/src/themes/mobile/src/main/js/touch/view/Orientation.js#L22

A more thorough stub might look like:

WickyNilliams/enquire.js#82 (comment)

Sure. Should we adjust it here to something like that comment?

I think it'd be an improvement, though in whatever solution we use, the result will be inaccurate at best. The implementation in the comment could at least avoid some strange behavior that could occur when trying to use properties of the return value (e.g. addListener as a noop at least prevents errors calling undefined as a function)

aduth · 2017-10-04T14:51:34Z

blocks/editable/index.js

@@ -119,7 +119,7 @@ export default class Editable extends Component {
 		editor.on( 'keyup', this.onKeyUp );
 		editor.on( 'selectionChange', this.onSelectionChange );
 		editor.on( 'BeforeExecCommand', this.maybePropagateUndo );
-		editor.on( 'BeforePastePreProcess', this.onBeforePastePreProcess );
+		editor.on( 'PastePreProcess', this.onPastePreProcess, true /* Add before core handlers */ );


Why was this change necessary?

The BeforePastePreProcess event has been removed.

aduth · 2017-10-04T15:00:42Z

I updated the package-lock.json file with the new dependency, but also seeing some warnings and unrelated changes to integrity I wouldn't expect. Might need to...revisit (maybe this known issue).

aduth · 2017-10-04T15:02:36Z

Thanks. I was more wondering why we don't use the NPM module for the browser as well, instead of pulling it from a CDN.

It probably doesn't make much sense when we're running a more bleeding-edge version than what's used elsewhere in Core, but the general point with aliasing to window globals is to play more nicely with shared dependencies of script loader, rather than potentially including multiple copies of the same dependency on the page by bundling it.

ellatrix · 2017-10-04T15:54:30Z

Looks like there's an extra div inserted.

youknowriad · 2017-10-06T09:17:53Z

This will close #1607

ellatrix · 2017-10-06T12:49:43Z

The placeholder issue is coming from tinymce/tinymce@40ff87c. Maybe we should add a class there after all, but don't really like TinyMCE inserting something outside its node.

ellatrix · 2017-10-06T12:56:12Z

Fix will be released Monday: https://wordpress.slack.com/archives/C0UCMQP0F/p1507294443000422.

ellatrix · 2017-10-09T15:30:11Z

Updated once more: tinymce/tinymce@4.7.0...4.7.1

aduth

Updated package-lock.json to latest version. Confirmed previous issue I saw is no longer present. Appears to work well 👍

ellatrix · 2017-10-12T20:37:08Z

Updated package-lock.json

Sorry, again forgot 🙈

ellatrix added [Component] TinyMCE [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended labels Sep 25, 2017

ellatrix changed the title ~~Update TinyMCE to the latest version~~ [WIP] Update TinyMCE to the latest version Sep 25, 2017

ellatrix requested a review from BoardJames September 25, 2017 13:24

ellatrix requested a review from spocke September 25, 2017 13:44

ellatrix force-pushed the update/tinymce branch from 9a92ef9 to 1c0ae06 Compare October 3, 2017 13:13

ellatrix changed the title ~~[WIP] Update TinyMCE to the latest version~~ Update TinyMCE to the latest version Oct 3, 2017

ellatrix force-pushed the update/tinymce branch from 1c0ae06 to f8c32d6 Compare October 3, 2017 13:18

ellatrix removed request for spocke and BoardJames October 3, 2017 13:19

ellatrix removed the [Status] In Progress Tracking issues with work in progress label Oct 3, 2017

ellatrix added this to the Beta 1.3 milestone Oct 3, 2017

ellatrix force-pushed the update/tinymce branch 2 times, most recently from b15b5a7 to c9bb6c5 Compare October 3, 2017 18:01

ellatrix force-pushed the update/tinymce branch from c9bb6c5 to 16a593d Compare October 3, 2017 18:07

aduth reviewed Oct 4, 2017

View reviewed changes

karmatosed modified the milestones: Beta 1.3, Beta 1.4 Oct 4, 2017

youknowriad mentioned this pull request Oct 6, 2017

TRY: Text Block: Sometimes the cursor gets moved behind the first character #2517

Closed

ellatrix force-pushed the update/tinymce branch from b9a8095 to de90d7d Compare October 6, 2017 12:58

Update TinyMCE to the latest version

a09ba26

ellatrix force-pushed the update/tinymce branch from de90d7d to a09ba26 Compare October 9, 2017 15:30

ellatrix requested a review from aduth October 9, 2017 15:32

mtias removed this from the Beta 1.4 milestone Oct 10, 2017

Update package-lock.json per TinyMCE 4.7.1 bump

e6b1f48

aduth approved these changes Oct 12, 2017

View reviewed changes

ellatrix merged commit 38bcd84 into master Oct 12, 2017

ellatrix deleted the update/tinymce branch October 12, 2017 21:24

youknowriad mentioned this pull request Oct 12, 2017

Text Block: Sometimes the cursor gets moved behind the first character #1607

Closed

This was referenced Oct 20, 2017

Heading Block: Unhandled DOMException when changing heading level #3091

Closed

TinyMCE: Assign dummy container during removal #3093

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update TinyMCE to the latest version #2787

Update TinyMCE to the latest version #2787

ellatrix commented Sep 25, 2017 •

edited

Loading

codecov bot commented Sep 25, 2017 •

edited

Loading

ellatrix commented Sep 25, 2017

aduth commented Oct 3, 2017

ellatrix commented Oct 3, 2017

ellatrix commented Oct 3, 2017

youknowriad commented Oct 3, 2017

ellatrix commented Oct 3, 2017

ellatrix commented Oct 3, 2017

aduth commented Oct 3, 2017

ellatrix commented Oct 4, 2017

aduth left a comment •

edited

Loading

aduth Oct 4, 2017

ellatrix Oct 4, 2017

aduth Oct 4, 2017

aduth Oct 4, 2017

ellatrix Oct 4, 2017

aduth commented Oct 4, 2017

aduth commented Oct 4, 2017

ellatrix commented Oct 4, 2017

youknowriad commented Oct 6, 2017

ellatrix commented Oct 6, 2017

ellatrix commented Oct 6, 2017

ellatrix commented Oct 9, 2017

aduth left a comment

ellatrix commented Oct 12, 2017

Update TinyMCE to the latest version #2787

Update TinyMCE to the latest version #2787

Conversation

ellatrix commented Sep 25, 2017 • edited Loading

Description

How Has This Been Tested?

Checklist:

codecov bot commented Sep 25, 2017 • edited Loading

Codecov Report

ellatrix commented Sep 25, 2017

aduth commented Oct 3, 2017

ellatrix commented Oct 3, 2017

ellatrix commented Oct 3, 2017

youknowriad commented Oct 3, 2017

ellatrix commented Oct 3, 2017

ellatrix commented Oct 3, 2017

aduth commented Oct 3, 2017

ellatrix commented Oct 4, 2017

aduth left a comment • edited Loading

Choose a reason for hiding this comment

aduth Oct 4, 2017

Choose a reason for hiding this comment

ellatrix Oct 4, 2017

Choose a reason for hiding this comment

aduth Oct 4, 2017

Choose a reason for hiding this comment

aduth Oct 4, 2017

Choose a reason for hiding this comment

ellatrix Oct 4, 2017

Choose a reason for hiding this comment

aduth commented Oct 4, 2017

aduth commented Oct 4, 2017

ellatrix commented Oct 4, 2017

youknowriad commented Oct 6, 2017

ellatrix commented Oct 6, 2017

ellatrix commented Oct 6, 2017

ellatrix commented Oct 9, 2017

aduth left a comment

Choose a reason for hiding this comment

ellatrix commented Oct 12, 2017

ellatrix commented Sep 25, 2017 •

edited

Loading

codecov bot commented Sep 25, 2017 •

edited

Loading

aduth left a comment •

edited

Loading