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. |
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. |
|
Tagging in a few folks who might be interested in taking this across the line for 6.7 cc @carolinan @Mamaduka @aristath @t-hamano. It would be awesome to see this finally added in. |
I've created this "Script Modules: Allow scripts to depend on modules" ticket for efforts to allow Scripts to depend on Script Modules. |
|
I've created #68191 on top of this PR to demonstrate how WordPress/wordpress-develop#8024 allows classic scripts to depend on script modules. If you'd like to help that work move forward, please leave feedback on WordPress/wordpress-develop#8024. |
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