fix: upgrade TinyMCE to v7 and remove the premium plugin stub package - #3245
bradenmacdonald wants to merge 4 commits into
Conversation
TinyMCE 5 -> 7 removed several APIs and plugins this app relied on: - `tinymce.editors` is gone; the problem editor now uses `tinymce.get()` to collect answer/hint/feedback content and check dirty state. - The `hr` and `imagetools` plugins were removed (the `hr` button is now core). The "Edit Image Settings" context toolbar that imagetools used to provide is re-registered as a plain context toolbar. - The `formatselect` toolbar item was renamed to `blocks`. - The `dom` model and each core plugin must now be imported explicitly. - TinyMCE 7 is GPL-2.0-or-later; `licenseKey="gpl"` silences its evaluation-mode warning. `frontend-components-tinymce-advanced-plugins` is removed as a dependency. It pinned TinyMCE 5 (pulling a second core into the bundle that clobbered `window.tinymce`) and only shipped no-op stubs for the paid a11ychecker and powerpaste plugins. Operators who have licensed those plugins can now load them via `TINYMCE_EXTERNAL_PLUGINS`, `TINYMCE_LICENSE_KEY` and `TINYMCE_PLUGIN_OPTIONS` in env.config.jsx (see README). The jest setup now stubs `tinymce/themes/silver` so the editor never finishes initializing under JSDOM, which the test suite has always implicitly relied on (previously the stubbed plugins hung initialization). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks for the pull request, @bradenmacdonald! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
openedx-webhooks
added
open-source-contribution
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3245 +/- ##
==========================================
- Coverage 96.03% 96.03% -0.01%
==========================================
Files 1407 1407
Lines 34287 34296 +9
Branches 7882 7879 -3
==========================================
+ Hits 32928 32936 +8
- Misses 1318 1319 +1
Partials 41 41 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@ChrisChV Could you please review this? @arbrandes @brian-smith-tcril Do you also want to review this, and/or are there any other implications for our other MFEs for bumping TinyMCE up two major versions? |
213f7e1 to
0c5a998
Compare
Sure 👍 |
Discussions and Communications also use it, so ideally they'd be bumped, too. If we were already fully in frontend-base, I'd suggest tracking this as a global peer dependency so we'd have to bump them. |
That's not something I have time to take on at the moment, but hopefully it's easier to bump them using the learning from this PR. And yeah, based on this forum discussion, I think we should eventually have a |
ChrisChV
left a comment
ChrisChV
left a comment
There was a problem hiding this comment.
Looks good! I commented some nits 👍
- I tested this: I followed the testing instructions. Also I tested the iframes
- I read through the code and considered the security, stability and performance implications of the changes.
- I tested that the UI can be used with a keyboard only (tab order, keyboard controls).
- Includes tests for bugfixes and/or features added.
- Includes documentation
| .tox:not(.tox-tinymce-inline) .tox-editor-header { | ||
| box-shadow: none; | ||
| padding: 0; | ||
| border-bottom: 1px solid #E3E3E3; |
There was a problem hiding this comment.
Can we use a root var for this color? Or can we use var(--pgn-color-gray-100)?
| powerpaste_html_import: 'prompt', | ||
| powerpaste_googledoc_import: 'prompt', | ||
| autoresize_bottom_margin: autoresizeBottomMargin, | ||
| external_plugins: externalPlugins, |
There was a problem hiding this comment.
The PR description says:
Pasting images: pasting moved into core in v6 with paste_data_images defaulting to true, so pasted screenshots now land in the content as base64 data URIs (v5 dropped them). Left at the default; easy to set to false if we would rather not allow it.
I think it would be good to add the parameter here with a comment so that the change is reflected and documented in the code.
| external_plugins: externalPlugins, | |
| external_plugins: externalPlugins, | |
| // Paste moved into core in v6 with paste_data_images defaulting to true, so pasted screenshots | |
| // now land in the content as base64 data URIs (v5 dropped them). Left at the default; easy to set to | |
| // false if we would rather not allow it. | |
| paste_data_images: true, |
| powerpaste_word_import: 'prompt', | ||
| powerpaste_html_import: 'prompt', | ||
| powerpaste_googledoc_import: 'prompt', | ||
| autoresize_bottom_margin: autoresizeBottomMargin, |
There was a problem hiding this comment.
The migration guide 6 to 7 says that a new security mechanism for iframes, sandbox_iframes, has been added and is set to true by default. This changes the editor's behavior. Previously, you could embed iframes, such as Google Forms, and they worked, but now that same iframe no longer functions. This issue only affects newly added iframes, but it would be good to add a comment here noting this change in behavior.
5 participants
Description
Upgrades TinyMCE from 5.10 to 7.9 (supersedes #3176, renovate's security bump, which fails tests and would break the editor at runtime) and removes the
frontend-components-tinymce-advanced-pluginsdependency, replacing it with a configuration-based way to load premium plugins.This affects Course Authors and Library Authors (all rich text editors: HTML/Text, Problem, hints, feedback, expandable text areas, course updates, schedule & details) and Operators (new configuration keys, license change, dependency removal).
Why the renovate PR alone was not mergeable
tinymce.editorswas removed in v7. The problem editor used it to collect answer/hint/feedback content and to check dirty state. This was the cause of the two failing tests. It now uses the publictinymce.get()API and reads each editor'sid.frontend-components-tinymce-advanced-pluginspinstinymce ^5.10.4, so with v7 at the root npm nested a separate TinyMCE 5 copy under it. The app imported all of its standard plugins through that package, so the bundle contained two cores, and the v5 one overwrotewindow.tinymce, which is what@tinymce/tinymce-reactreads. The editor would have been broken in the browser even though the unit tests didn't catch it.hrandimagetoolsplugins no longer exist (hris now a core button). The "Edit Image Settings" context toolbar thatimagetools_toolbarused to provide is re-registered as a plain context toolbar on<img>nodes.formatselecttoolbar item was renamed toblocks.dommodel and each core plugin now have to be imported explicitly.licenseKey="gpl"on the ReactEditor).Removal of
frontend-components-tinymce-advanced-pluginsThat package only shipped no-op stubs for the paid
a11ycheckerandpowerpasteplugins, is still on TinyMCE 5 upstream (a renovate PR bumping it was closed), and re-exports plugins that no longer exist. It is removed entirely. Operators who have licensed the premium plugins can now load them throughenv.config.jsx:TINYMCE_EXTERNAL_PLUGINS:{ pluginName: 'https://.../plugin.min.js' }, passed to TinyMCE's built-inexternal_pluginsloader. Configuringa11ycheckerrestores its toolbar button; configuringpowerpasterestores the paste defaults the app previously set.TINYMCE_LICENSE_KEY: the commercial license key that premium plugins in v7 require (defaults togpl).TINYMCE_PLUGIN_OPTIONS: extra init options for those plugins.Documented in the README under "Feature: New React XBlock Editors", with a test in
pluginConfig.test.Behavior changes to be aware of
valid_elements: '*[*]'.<script>tags and inline event handlers are preserved exactly as v5 did, so existing course content is not stripped. The one change is the fix for CVE-2024-29881 itself:convert_unsafe_embedsis on by default, so<object>/<embed>are rewritten to<iframe>/<img>/<video>/<audio>(by MIME type) when content is loaded and re-saved.paste_data_imagesdefaulting totrue, so pasted screenshots now land in the content as base64 data URIs (v5 dropped them). Left at the default; easy to set tofalseif we would rather not allow it.highlight_on_focusby default, adding a blue outline around the focused editor.Test-suite note
Removing the stub plugin names exposed that the jest suite has always relied on TinyMCE never finishing initialization under JSDOM: previously the editor silently hung trying to fetch the (mocked)
a11ychecker/powerpastescripts over the network. With every plugin genuinely registered, the editor initialized for real and crashed inside the theme's sizing code, which JSDOM can't support.src/setupTest.jsnow stubstinymce/themes/silverto make that dependency explicit. Browser behavior is unaffected.Supporting information
Manual Testing instructions
Other information
frontend-components-tinymce-advanced-pluginspackage no longer needs a TinyMCE 7 release for this MFE.TINYMCE_*configuration keys.Best Practices Checklist
.ts,.tsx).propTypesanddefaultPropsin any new or modified code. (No new usages; the pre-existing ones inTinyMceWidgetare untouched.)src/testUtils.tsx(specificallyinitializeMocks) (N/A: the new test is a pure config test; existing tests were adapted in place.)messages.tsfiles have adescription. (N/A)../in import paths. (No new relative imports added.)🤖 Generated with Claude Code, with edits from me.
Private ref: MNG-4670