Add CodeMirror to Additional CSS / Custom HTML block #60155
Conversation
|
Size Change: +2.03 kB (0%) Total Size: 1.92 MB
ℹ️ View Unchanged
|
okmttdhr
changed the title
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
talldan
added
[Type] Enhancement
|
Thanks for the contribution. It may be good to search the archive for "codemirror" as well, there have been past conversations exploring adding it to the code editor section, which ultimately failed. It would be good to see what lessons could potentially be learned adding it here. I'd like to see this land, but there are some checks and balances that need to be considered. Perhaps an option even is to turn it on or off. This could be in preferences and/or onboarding, that might even let it be available for the code editor too. A few notes from a superficial review, here's a GIF showing some light CSS editing and tabbing:
Probably related to the past conversations on codemirror, IMO the biggest potential benefit of using codemirror is tabbing to indent/outdent code. Yet that's also one of the main challenges, as we still need a way for keyboard users to enter and exit this field. That's something to consider the best practices for. |
|
Thank you for your review, @jasmussen!
I have addressed these points and updated the video in the PR description accordingly 🙂
I think line numbers serve as a clear & common indicator for developers that "Oh, this is the place I can write code". However, I agree that the space is limited in terms of the Additional CSS input, so I've removed it, while retaining it for a Custom HTML block.
I implemented tab-based indentation following the documentation: CodeMirror Language Config Example. It mentions that using Tab for indentation is not enabled by default because it could conflict with Success Criterion 2.1.2 No Keyboard Trap. I attempted to address this by implementing Esc & Tab to focus out (which is the same behavior as focusing on/out blocks in the editor) and by adding instructions using
Technically, altering the syntax highlighting colors is straightforward — it's about overriding the original CSS. However, it's tricky to select the appropriate colors, IMO. For instance, the following CSS already contains some colors, necessitating consideration in determining which colors should be defined and used in which state (like when highlighting the text). 
As far as I know, CodeMirror was first introduced to Gutenberg in this PR: #4348 but removed later in this PR: #10396. It was mainly due to the difficulty of lazy loading at that time. Now, importmap is coming soon in WP 6.5, and it's ready to get prepared by using I'm aware there have been several UX discussions, but I may need more time to find/review them thoroughly. 🙂 Please let me know if anything can be improved. |
@jasmussen I'm trying to understand your concerns more clearly. |
| const { EditorView, basicSetup } = await import( 'codemirror' ); | ||
| const { indentWithTab } = await import( '@codemirror/commands' ); | ||
| const { keymap } = await import( '@codemirror/view' ); | ||
| const { css } = await import( '@codemirror/lang-css' ); |
There was a problem hiding this comment.
Importing these individual packages will create four webpack chunks, and the loading will take more time than necessary, because the next load starts only after the previous one has finished.
Better to move the EditorView code into a separate module and load it all at once:
const editorRef = useRef();
useEffect( () => {
import( './code-mirror-view' ).then( ( { default: createEditor } ) => {
createEditor( {
parent: editorRef.current,
mode: 'css', // or 'html'
onChange: handleOnChange,
onBlur: handleOnBlur,
} );
} );
}, [] );Then there will be just one code-mirror-view chunk that's loaded on demand. There's a bit of trouble with the lang-css and lang-html modules. Do we want to bundle them both in a single chunk? That depends on how big they are.
Or the dynamic module can contain the React component that shows the editor: the <div> together with the useRef and the useEffect (this time the effect doesn't do dynamic import). Then it can be used like:
const CodeEditor = React.lazy( () => import( './code-mirror-view' ) );
<Suspense fallback={ <Placeholder /> }>
<CodeEditor id className aria-describedBy value onChange onBlur />
</Suspense>It would be also a good idea to not load the CodeMirror editor not when the AdvancedPanel is rendered, that will load it too often. Load it only after clicking on the CSS field, only after the user shows a clear intent to edit.
There was a problem hiding this comment.
Thank you for your review/insights, @jsnajdr! 😄
and the loading will take more time than necessary, because the next load starts only after the previous one has finished
Not exactly, in terms of import. Fundamentally, based on my understanding of how modules are waited for and executed, the following code
import { a } from './a.mjs';
import { b } from './b.mjs';
import { c } from './c.mjs';
console.log(a, b, c);would be roughly equivalent to the following:
import { promise as aPromise, a } from './a.mjs';
import { promise as bPromise, b } from './b.mjs';
import { promise as cPromise, c } from './c.mjs';
export const promise = Promise.all([aPromise, bPromise, cPromise]).then(() => {
console.log(a, b, c);
});At least, Webpack behaves in this manner, even for import(). Here’s a screenshot showing that the requests for chunks run in parallel when using the original approach.
However, this does not seem to be stated explicitly in the spec of import(), and my tests confirmed that native dynamic imports execute requests sequentially both at the top level and within function scopes. I’ll address this later in this comment.
Importing these individual packages will create four webpack chunks
This also applies even if we:
- consolidate everything into one React component,
- AND use
React.lazy() - AND use static
import {} from 'codemirror'
because we are splitting the vendor's chunk.
This approach does not create a single chunk, even with all logic extracted into a component and using static import (although we can tweak chunks further by using the optimization.splitChunks option).
Thus, the requests for the “one-component approach” looked like this, showing no difference from the original approach:
Given the above, I propose the following;
- For explicit parallelization, use
Promise.all([import(), import(), …])rather than fetching a single larger chunk at once. This ensures that requests run in parallel, even after transitioning to native dynamic import. - Extract “EditorView”: I favor the one-component approach for its cohesiveness (rather than for performance reasons), so I encapsulated the logic into a component for reuse. 🙂
For now, parallel chunk requests seem to be more efficient.
Then there will be just one
code-mirror-viewchunk that's loaded on demand. There's a bit of trouble with thelang-cssandlang-htmlmodules. Do we want to bundle them both in a single chunk? That depends on how big they are.
I don’t think it’s a good idea to load all lang-* modules in this component because we may want to support additional language modes in the future. (Even with the “parallel requests” strategy, this could potentially max out the browser's limit on simultaneous requests.)
It would be also a good idea to not load the CodeMirror editor not when the AdvancedPanel is rendered, that will load it too often. Load it only after clicking on the CSS field, only after the user shows a clear intent to edit.
On a slow 3G network, this delay could be around 8 seconds. Making users wait that long for syntax highlighting isn't ideal, IMO.
I'd consider more eager loading strategies (if that doesn't interfere with user interaction). For instance, for Additional CSS, we could load the chunks when the three-dots menu is opened, or for a Custom HTML block, load them when the block appears in the search results. Alternatively, prefetch might be a viable approach. WDYT?
Once the chunks are loaded, subsequent network requests are unnecessary, even if users access Additional CSS multiple times. Thus, “loading them too often” shouldn’t pose an issue, in my opinion. We just need to be careful about how it affects users’ actual interactions/experiences.
There was a problem hiding this comment.
we are splitting the vendor's chunk
We should consider not doing this. After all, currently Gutenberg doesn't do it on purpose, but only because it's a webpack default and we didn't do any dynamic chunks until now.
In my block lazy loading experiment I disabled it, see this comment: https://github.com/WordPress/gutenberg/pull/55585/files#r1383350711
It helps to keep a nice file structure with human-comprehensible names.
There was a problem hiding this comment.
Great work, it would be really nice to have this feature!
I commented on things I noticed on the first pass, but before code specifics, we need to make sure there are no accessibility blockers. If we can make it work for everyone, great. If not, that might mean making it a toggleable option.
I think the accessibility-related notes I made are addressable, but I don't have enough familiarity with the text editing mechanics inside the editor itself. The code completion popover works as expected for me in VoiceOver, so that's good.
packages/block-editor/src/components/global-styles/advanced-panel.js
Outdated
Show resolved
Hide resolved
| // We only want to run this once, so we can ignore the dependency array. | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [] ); |
There was a problem hiding this comment.
I'm not sure this is tenable in a React context. I'm assuming from this code that the content, onChange, onBlur, and mode props are all basically initial config and cannot be dynamically updated. As a component consumer, that is unexpected to me. We could for example tweak the component API to make it clearer that they're initial config (like have consumers pass in the config through the output of a useEditorView( config ) hook or something).
There was a problem hiding this comment.
Thanks for your comment!
I'm assuming from this code that the content, onChange, onBlur, and mode props are all basically initial config and cannot be dynamically updated
Yes, the approach was chosen to initialize new CmEditorView just once, allowing CodeMirror to handle subsequent updates, as I was to minimize complexity.
As a component consumer, that is unexpected to me.
It makes sense to me! I have just updated the component API to use the term initialConfig to clarify that these properties are intended for initial setup only, but WDYT?
| spellCheck={ false } | ||
| /> | ||
| <label | ||
| htmlFor={ EDITOR_ID } |
There was a problem hiding this comment.
Apparently a generic div is not labelable. (Confirmed not labeled in the accessibility tree.)
| > | ||
| { editorInstructionsText } | ||
| { __( | ||
| `Press Escape then Tab to move focus out of the editor.` |
There was a problem hiding this comment.
Can we actually make the editor field blur on Escape? Currently, it just maintains the focus outline and I can still type in the field. At the very least I think we need some visual feedback for the Escape.
There was a problem hiding this comment.
I've taken a look at the current implementation, and it seems to work as intended on my end. Here's what it looks like:
Screen.Recording.2024-04-02.at.15.38.15.mov
I'm wondering if there might be specific conditions or environments where this behavior isn't consistent. Could you share more details about the issue you're encountering?
packages/block-editor/src/components/global-styles/advanced-panel.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
For the editor in the Custom HTML block, we'll need to style it in a way that works across different color schemes. This is how it currently looks in a dark color scheme:
Co-authored-by: Lena Morita <lena@jaguchi.com>
Thanks for starting this work, I don't think we need to wait for all the packages to be modules to be able to use import maps. Import maps is already supported in Core today and scripts can load modules so I think by making code mirror a module, we can start using it today. |
From an accessibility perspective, I'd like to avoid to repeat all the concerns that were originally raised for the core implementation in https://core.trac.wordpress.org/ticket/12423. Basically, Codemirror drastically alters the DOM making it hardly readable with assistive technology. Also, the Codemirror UI isn't greatly accessible. That's the reason why in https://core.trac.wordpress.org/ticket/12423 the final decision was that codemirror can be entirely disabled by users. This whole feature must honor the user preference |
| { | ||
| name: 'codemirror', | ||
| message: | ||
| 'Please use dynamic import (`import()`) instead since it is a large dependency.', | ||
| }, | ||
| { | ||
| name: '@codemirror/commands', | ||
| message: | ||
| 'Please use dynamic import (`import()`) instead since it is a large dependency.', | ||
| }, | ||
| { | ||
| name: '@codemirror/lang-css', | ||
| message: | ||
| 'Please use dynamic import (`import()`) instead since it is a large dependency.', | ||
| }, | ||
| { | ||
| name: '@codemirror/lang-html', | ||
| message: | ||
| 'Please use dynamic import (`import()`) instead since it is a large dependency.', | ||
| }, | ||
| { | ||
| name: '@codemirror/view', | ||
| message: | ||
| 'Please use dynamic import (`import()`) instead since it is a large dependency.', | ||
| }, |
There was a problem hiding this comment.
This is a bit repetitive. Does it make sense to use the patterns option to combine them into a single restricted import?
| * (height of all the elements in the sidebar except the editor) + (height of the header) + (height of the footer) | ||
| * Currently, it's desktop-optimized. | ||
| */ | ||
| const MARGIN = 234 + 60 + 25; |
There was a problem hiding this comment.
If we're going to keep these static, can we move them up there in the module, right below the module imports?
I'd also suggest splitting each into a separate constant, which will allow separate comments for each value, perhaps making it all a bit more readable.
| */ | ||
| function ensureMinLines( content ) { | ||
| const MIN_LINES = 10; | ||
| const LINE_HEIGHT = 18.2; // Height of one line in the editor |
There was a problem hiding this comment.
Same as above in terms of where the constants live.
| initialConfig={ | ||
| { | ||
| callback: () => ensureMaxHeight(instanceId), | ||
| content: ensureMinLines( inheritedValue?.css ), |
There was a problem hiding this comment.
The optional chaining here hints that we could have an undefined passed to ensureMinLines(). But ensureMinLines() expects content to be defined as it does content.split(). Should we add some checks or coalesce to a default value?
| @include input-style__focus(); | ||
| } | ||
| } | ||
| .cm-editor { |
There was a problem hiding this comment.
I wonder if we can make things less specific here. Right now we have 4 levels:
.edit-site-global-styles-screen-css.components-v-stack.cm-editor.cm-line
Do we need all of that nesting?
| placeholder={ __( 'Write HTML…' ) } | ||
| aria-label={ __( 'HTML' ) } | ||
| <EditorView | ||
| editorId={'block-library-html__editor'} |
There was a problem hiding this comment.
| editorId={'block-library-html__editor'} | |
| editorId="block-library-html__editor" |
| aria-label={ __( 'HTML' ) } | ||
| <EditorView | ||
| editorId={'block-library-html__editor'} | ||
| editorInstructionsText={__( |
There was a problem hiding this comment.
Spacing seems to be a bit off here. Linter didn't run for some reason.
| <EditorView | ||
| editorId={'block-library-html__editor'} | ||
| editorInstructionsText={__( | ||
| `This editor allows you to input your custom HTML.` |
There was a problem hiding this comment.
Seems to be no need for a template literal, it can be a quoted string.
| `This editor allows you to input your custom HTML.` | |
| "This editor allows you to input your custom HTML." |
| onChange={ ( content ) => setAttributes( { content } ) } | ||
| placeholder={ __( 'Write HTML…' ) } | ||
| aria-label={ __( 'HTML' ) } | ||
| <EditorView |
There was a problem hiding this comment.
This is a big change to the html block and may need a CHANGELOG mention.
|
I created #60362 to explore reorganizing the When we import the CodeMirror modules with individual dynamic imports, like: await import( 'codemirror' )
await import( '@codemirror/commands' )
await import( '@codemirror/view' )
await import( '@codemirror/lang-css' )then webpack assumes that the imports are independent, i.e., any of the modules can be imported at any time, without importing the others. That's why it creates 7 independent chunks for CodeMirror modules: 
For example, But when I move the imports to be synchronous in a separate import 'codemirror'
import '@codemirror/commands'
import '@codemirror/view'
async function importLanguage() {
await import( '@codemirror/lang-css' )
}and import the file dynamically with
The result is that there are only three chunks, with minimal size and no code duplication: 
Much better layout. I'm also specifying a 
|
Thank you for the valuable resources, @afercia! |
Thanks for your suggestion, @youknowriad! In reference to Script Modules in 6.5, my understanding is that for utilization of a module (in this PR use case), it must be declared as a dependent of the consumer to be added into the wp_register_script_module(
'@my-plugin/shared',
plugin_dir_url( __FILE__ ) . 'shared.js'
);
wp_enqueue_script_module(
'@my-plugin/entry',
plugin_dir_url( __FILE__ ) . 'entry.js',
array( '@my-plugin/shared' )
);Thus, |
Oh It's unfortunate that this slipped through the cracks, I wasn't aware of it. I think we should have a way to add a module to the import map without it being a dependency of another module that gets loaded manually in order to be used by scripts. @sirreal how do we solve this, it feels like a small thing to allow without necessarily making all scripts modules. |
Just for compelteness, it does it also for the Theme File Editor and for the Plugin File Editor. |
Scripts and modules are completely separate right now, there's no interoperability or interdependence right now. I hope to have a good solution for WordPress 6.6. Work is happening in this Trac Ticket. |
|
📝 I plan to revisit this PR again in a while, as I currently don't have much time. But feel free to take this over if anyone is interested. |
What?
This PR introduces inline code completion and linting into Global Style's Additional CSS and Custom HTML block by using CodeMirror.
Additional CSS
before
Screen.Recording.2024-03-26.at.14.30.24.mov
after
Screen.Recording.2024-03-27.at.15.16.21.mov
previous commits
Screen.Recording.2024-03-26.at.14.29.24.mov
Custom HTML
before
Screen.Recording.2024-03-26.at.14.39.08.mov
after
Screen.Recording.2024-03-26.at.14.38.12.mov
Chunks are loaded on demand and reused
Screen.Recording.2024-03-26.at.14.41.57.mov
Why?
CodeMirror is first introduced to Gutenberg in this PR: #4348 but removed in this PR: #10396. The follow-up issue: #10423 was created, but there haven't been any further updates.
This PR addresses #10423 and #47945.
How?
Branched from PR #53380, this update utilizes Webpack’s dynamic
import()as a temporary measure to lazy-load CodeMirror, as I agree with this comment.This code should work even when we adopt importmap later. Once we turn all
@wordpress/*packages (specificallyblock-editorin this case) into modules by usingoutput.module=true, we can externalizecodemirrorby usingdefaultRequestToExternalModuleinDependencyExtractionWebpackPluginand register the script as a module bywp_register_script_module('@wordpress/block-editor')settingcodemirroras a dependency module.Testing Instructions
Additional CSS
Custom HTML