Problem/Motivation
We "solved" the problem of supporting <ol start>
by using the SourceEditing
functionality for that.
But in CKEditor 5 32.0.0, they've added native support for <ol start>
(and even for <ol reversed>
).
Which means that rather than having to edit HTML by hand, the content creator can now do that through the UI!
Steps to reproduce
N/A
Proposed resolution
Use it!
Clearest example I could find of how to configure this: https://github.com/ckeditor/ckeditor5/commit/99c818c7b9ddf62aa958b9d265d...
Remaining tasks
After Drupal is updated to that version of CKEditor 5, we should modify the ckeditor5_list
plugin definition to:
- have a plugin class that allows the site builder to configure the list functionality to specify the starting index and whether the ordered list should be reversed (two separate booleans to store). See
\Drupal\ckeditor5\Plugin\CKEditor5Plugin\Heading
for an example — note this will also require an addition to the config schema, but that too has an example - this should cause
\Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest
to fail — because a better upgrade path becomes possible, so we should update that test coverage
User interface changes
TBD
API changes
None.
Data model changes
None.
Release notes snippet
TBD
Comment | File | Size | Author |
---|---|---|---|
#26 | 3261599-25-d94.patch | 36.54 KB | lauriii |
#20 | 3261599-20.patch | 36.96 KB | Wim Leers |
#13 | 3261599-13.patch | 31.95 KB | Wim Leers |
|
Issue fork drupal-3261599
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
Wim LeersThis is blocked on #3261600: Update to CKEditor5 v32.0.0.
Comment #3
Wim LeersSee #3261600-29: Update to CKEditor5 v32.0.0 — this will soon be unblocked. The configuration I used there:
which yielded:
Comment #4
Wim Leers#3261600: Update to CKEditor5 v32.0.0 landed — this is now unblocked!
Comment #7
bnjmnmI was checking this out with @hooroomoo and ran into a test failure that (at least at the moment) cant tell if it's a problem with the test, this implementation, or a new edge case.
SmartDefaultSettingsTest
is failing on thefilter_only__filter_html can be switched to CKEditor 5 without problems (3 upgrade messages)
iteration. With the errorThe following tags/attributes are not allowed in the updated text format:<ol type="1 A I">
This failure is when$unsupported_tags_attributes
isn't emptyHere's how
$unsupported_tags_attributes
made:So it's the diff of
$allowed_tags
against$updated_allowed_tags
.$updated_allowed_tags
includes<ol type>
(now offered in the), which implicitly allows the<code><ol type="1 A I">
that is deemed unsupported.diff()
is intentionally written this way - if the caller is more restrictive it is returned as part of the diff.So two questions come to mind:
type
attribute in this issue's scope?These may answer themselves once I'm away from the IDE for a moment, but now it's documented.
Comment #8
Wim Leers#7:
Review
HtmlRestrictions
. Opened #3274648: HTMLRestrictions::merge() and ::toGeneralHtmlSupportConfig() fail on allowed attribute values that can be interpreted as integers for that. It's not a blocker for this issue, because … the metadata being added here is wrong (see review on the MR): this MR does not add support for<ol type>
nor<ul type>
. I added #3274635: [upstream] Use CKEditor 5's native <ol type> and <ul type> UX for that. And while investigating that … found #3274651: Impossible to enable <ol type> or <ul type> with GHS: switch to List's successor, DocumentList…SmartDefaultSettingsTest::test with data set "basic_html_with_h1 can be switched to […]
) → this is because #3273527: Upgrade path never configures the ckeditor5_heading plugin to allow <h1> just landed and added one more test case.Comment #9
hooroomooComment #10
Wim LeersLooking great! There's only a few coding style nits and one missing comment … and then this is RTBC! 🥳
Comment #11
hooroomooComment #12
Wim LeersComment #13
Wim LeersAll commits up to 97b690a6a5c476895eb4878dfc686e8fc6355a79 in patch form, for testing against all 3 branches.
Comment #14
Wim LeersDiscussed with @hooroomoo — un-RTBC'ing only to add the unit test that #3229078: Unit tests for all @CKEditor5Plugin plugin classes would otherwise have to add.
Comment #15
Wim LeersWell that was a perfect example how making a slight mistake can lead one into a fall sense of confidence — unit tests may pass but the whole may not be telling y ou much: https://giphy.com/gifs/healthy-VldGsyiAmMW9a 😄
Comment #16
hooroomooComment #17
Wim LeersComment #18
hooroomooComment #19
Wim LeersPerfect! Time to land this one! 🥳🚀
Comment #20
Wim LeersNote that this patch applies cleanly to all 3 branches — except for
9.3.x
until #3261943: Confusing behavior after pressing "Apply changes to allowed tags" with invalid value gets cherry-picked onto9.3.x
. So intentionally not yet testing against9.3.x
.Comment #21
Wim LeersBumping to
priority because this is a CKE5 stable blocker.Comment #22
Wim Leers#3261943: Confusing behavior after pressing "Apply changes to allowed tags" with invalid value was just cherry-picked to
9.3.x
— queued9.3.x
test of #20 👍Comment #23
bnjmnmJust a few nits that can be easily taken care of as part of the reroll.
Lets either get rid of the line breaks or make this HEREDOC. (No line breaks is fine, though, it's clear what is being added).
NIT: This, and $reversed_order_button and $start_index_element should get renamed to make it clear they are selectors, not the actual element (such as what might be returned by $page->find())
Comment #24
hooroomooSelf-RTBC'ing because super minor changes made
Comment #26
lauriiiCommitted e935c43 and pushed to 10.0.x. Thanks!
Leaving open for 9.4.x and 9.3.x backport.
Comment #29
bnjmnmBackported to 9.4.x + 9.3.x
Comment #31
Wim LeersThis failed to provide an update path 😅 Doing that now at #3396628: Fix <ol start> → native CKEditor 5 functionality and fix bug in SourceEditingRedundantTagsConstraintValidator that allowed it to slip by.