Replace jQuery UI sortable with Sortable js

Added quick implementation. For ckeditor buttons.
First buttons managed with sortable.js and second with jqueryUI.

Sortable seems like an excellent choice to replace jquery.ui.sortable.
It does have a different API, and does not depend on jQuery which makes it a sensible option for the long term. I continued the work @finnsky started in #3, applying Sortable to the other 2 sections in this ckEditor admin page, as well as the start and stop events so that it registers the sorting.

Sortable has a "group" option which replaces the "connectWith" method of jquery.ui.sortable, as well as the "clone" which is used by the separator button.

For accessibility, using the keyboard works- you can move buttons into the toolbar and sort toolbar groups.

Sortable also works better for touch events, so the "draggable" and "touch-punch" asset libraries are no longer necessary. Tested this page with an iPhone.

Some other notes:

• Removed the box shadow property from stable/css/ckeditor/ckeditor.admin.css so that the keyboard focus is visible
• Removed the "!success" condition from endGroupDrag since Drupal.ckeditor.registerGroupMove does not have a callback.

There are 38 contrib projects that use the jquery.ui.sortable asset library.
Hopefully a contrib module with a could be a replacement for any of those projects that don't want to update their javascript and an API bridge won't be necessary.
See: https://www.drupal.org/project/drupal/issues/3067251 for examples.

I searched in core and confirmed @effulgentsia report that only ckeditor, media and layout_builder require the jquery.ui.sortable asset library.

If Sortable ends up as the replacement choice it might be best to open issues for each of those projects separately instead of continuing work here.

Did it pass a11y review?

Warning: file_get_contents(core/assets/vendor/sortable/Sortable.min.js): failed to open stream: No such file or directory

The new asset library js was missing from my last patch, I didn't notice that it was git ignored on my local and was not committed.

Uploading the (hopefully) correct patch now, with 2 changes:

• Fixed how the cloned "divider" buttons get added to ckeditor.
• Removed the css edit to stable, that should be a separate issue.

re: #6 Sortable seems to be as accessible as jquery.ui.sortable, for this ckeditor admin page the keyboard actions and "focus" are handled separately from these libraries. I will do a more thorough review though and report back.

• #3: Looks like we're off to a great start! :D
• #5 answered the question that #3 triggered: what about keyboard support. It's present. Great! Touch support present out-of-the-box without the need for some additional work-around library: excellent.
• Manually tested the CKEditor toolbar configuration UI. Works great with both keyboard and mouse. Exactly like before.

Patch review

1. +++ b/core/modules/ckeditor/js/views/VisualView.es6.js
   @@ -214,21 +214,34 @@
   +            buttons.classList.add('js-sortable');
   

   Is this class name arbitrarily chosen or does it have special meaning?

2. +++ b/core/themes/stable/css/ckeditor/ckeditor.admin.css
   @@ -227,7 +227,7 @@
   -.ckeditor-button-placeholder,
   +.ckeditor-buttons .ckeditor-button-placeholder a,
   

   Why this change?

@Wim Leers hi!
1. i needed some `processed` class. so yes arbitrarily chosen
2. all css should be rewritten i guess. here i needed to force blue bg of active item placeholder. quick implementation.

but i think we shouldn't continue patching here. we need child issues with each (ckeditor, media_library, and layout_builder) implementation. So if this library is fine we may start:)

1. 👍
2. We should avoid CSS changes if at all possible, because this is going to make it more difficult for existing code to change from core/jquery.ui.sortable to something else. If it's absolutely necessary, we should talk about it in more detail. So: is it absolutely necessary, and why?

@Wim Leers
From my point of view it is a bit wierd to have 2 equals css files in ckeditor module and in stable theme and they both were attached.
https://git.drupalcode.org/project/drupal/blob/8.8.x/core/modules/ckedit...
https://git.drupalcode.org/project/drupal/blob/8.8.x/core/themes/stable/...
i think it was done to cover cases when ckeditor will be enabled with parent classy theme. But in this case what for it is in stable since it is only ckeditor related?

But anyway. As i said it was library example PR. Not real one. I think css changes will needed because libraries not absolutely equal. some styles were taken from jquery UI. Sortable allows to assign elements classnames. But not for all of required i guess.

From my point of view it is a bit wierd to have 2 equals css files in ckeditor module and in stable theme […]

All Drupal core modules' CSS files MUST be duplicated into the Stable theme, see https://www.drupal.org/node/2580687. The Stable theme provides a stable foundation for custom/contrib CSS (hence the name), to avoid backwards compatibility breaks for those themes.

I think css changes will needed because libraries not absolutely equal. some styles were taken from jquery UI. Sortable allows to assign elements classnames. But not for all of required i guess.

Adding CSS to Drupal core is fine, but it'd be great if not every consumer of core/jquery.ui.sortable needs to add or change CSS. Ideally, Drupal core would automatically attach whatever CSS is necessary, and no other CSS tweaks would be necessary.

The old jquery.ui.sortable added an empty <li> to the markup but Sortable just adds a single class to the existing element, so without the override in #3 there isn't a visible placeholder.
To avoid breaking BC both lines of css could be added since there's no conflict- the markup is different in each implementation as mentioned in #14.

If I understand #15 correctly the change should be applied the same in both core/modules/ckeditor/css/ckeditor.admin.css and core/themes/stable/css/ckeditor/ckeditor.admin.css

but i think we shouldn't continue patching here. we need child issues with each (ckeditor, media_library, and layout_builder) implementation.

I agree with this but each of these child issues would have the same dependency on the Sortable js and change to core.libraries.yml
Since there is only one part of media_library and layout_builder each that use it, it won't make the patch so much more complex to keep all 3 together in this issue.

If it is easier for the maintainers of these modules to review separate issues though I am happy to split up this patch.

In the meantime, attached is an update to layout_builder

Made a small css change, since the dragged image did not have a background and it appeared transparent in Chrome. This seemed like the easiest fallback style since the browser controls how the dragged element looks, and matches the background color of the parent element so it doesn't introduce any other new visual changes.

#16:

• Is this in response to #13.2?
• RE: #15: yep.

#17:

• +1 to doing it all in this issue. How else are we able to validate the use of a particular JS library? This is why it's very valuable (and a big relief) that we have multiple consumers of this already in Drupal core: it helps us be more confident about replacing the existing asset library with another one.
• Yay for updating Layout Builder to use the new library! We'll need to get the tests to pass though. And then it's down to the last consumer of the old library 🥳

Sortable is "Built using native HTML5 drag and drop API"
It seems that the chromedriver used by selenium does not correctly use the html5 drag events.
https://bugs.chromium.org/p/chromedriver/issues/detail?id=2695

https://github.com/SeleniumHQ/selenium/issues/3269#issuecomment-406378953

This means that the current tests will fail whenever the dragTo method is called while using Sortable.

I'm not sure which version of chromedriver the testbot is using but I was able to get the latest 76.0.3809.68 running locally and confirmed that dragTo doesn't work.

I'm not sure what the next steps are with this- remove the tests? Pick a different (non html5) javascript library? Postpone this issue until chromium gets updated? Install html-dnd, or another fix like chromedriver-html5-dragdrop?

Is there method to mark tests skipped if some requirements are not met?

#20: Darn! 😕So as we move from jQuery UI's workaround-based drag-and-drop to a standards-based one, we actually lose some testing ability 😭 Precisely this is explicitly mentioned in https://bugs.chromium.org/p/chromedriver/issues/detail?id=2695#c22.

Note that https://bugs.chromium.org/p/chromedriver/issues/detail?id=2695#c28 points to https://gist.github.com/druska/624501b7209a74040175#file-native_js_drag_... as being a way to work around this limitation. Can we override our dragTo() to use this instead until Chromium is fixed?

Note I said Chromium, not Chromedriver, because as of 15 days ago Chromedriver ticket 2695 is marked as blocked on https://bugs.chromium.org/p/chromium/issues/detail?id=850071.) A googler recently advocated for that issue, so fingers crossed: https://bugs.chromium.org/p/chromium/issues/detail?id=850071#c7 🤞

#21: Yes. Grep *Test.php files for @required to find a bunch of examples. Alternatively, $this->markTestAsSkipped() in ::setUp().

If you are testing FunctionalJavascript tests and starting with a newer install of chromedriver make sure to add { "chromeOptions": { "w3c": false } } to your MINK_DRIVER_ARGS_WEBDRIVER settings.

I couldn't even get those tests to run at all at first and spent quite a bit of time debugging with @tedbow yesterday, and from talking with @lendude narrowed my issues down to the chromedriver version.

https://www.drupal.org/project/drupal/issues/3073342

The current dragTo in the Selenium2Driver creates an event which triggers the jquery.ui.sortable library to respond, moves the mouse and fires a drop event.

It's using a very outdated version of Syn
https://github.com/minkphp/MinkSelenium2Driver/pull/233

which is

...a synthetic event library that pretty much handles typing, clicking, moving, and dragging exactly how a real user would perform those actions.

Unfortunately these events are not sufficient to trigger anything in Sortable.js or in the chromedriver so the 2 tests using dragTo fail.
I tested several libraries that aim to provide the correct custom events to allow for html5 drag and drop in functional tests,
however none of these triggered Sortable programatically.

I converted several for use in mink, but I also created jsfiddles to check each one in the browser.

• druska native_js_drag_and_drop_helper.js (jsfiddle)
• photonios dndsim.js (jsfiddle)
• andywer drag-mock (jsfiddle)
• rcorreia drag and drop helper (jQuery) (jsfiddle)
• SortableJS dndsimulator.js (jsfiddle)

The last one (ember-sortable dndsimulator) does visibly trigger the pointerdown event in the jsfiddle.

Even if one of these libraries did work for triggering the SortableJS drag event programmatically it might mean getting some new methods in Mink.
For example, the private executeJsOnElement method only accepts a single element as an argument while the drag event libraries above all need a source and a destination.

Since the native drag and drop events aren't supported by the chromedriver and these solutions are all using javascript to simulate those events, it might not be so bad to just "fake" the drag and drop in the test.
This patch adds some new methods which manipulate the DOM directly, moving an element into a new container or after a sibling. The existing "update" method from the layoutbuilder js which Sortable calls after finishing a move is now separated out of the Drupal behavior so that the callback can be called from the test as well.

I'm wary of changing the test to get the result I want but it seemed preferable to using a non html5 drag and drop sortable library, or skipping the tests entirely.

It's using a very outdated version of Syn
https://github.com/minkphp/MinkSelenium2Driver/pull/233

😲🤯 What a mess! I'm sorry you have to bring clarity in this chaos 😔

Since the native drag and drop events aren't supported by the chromedriver and these solutions are all using javascript to simulate those events, it might not be so bad to just "fake" the drag and drop in the test.
[…]
seemed preferable to […] skipping the tests entirely.

Indeed, apparently we don't have any other choice. 😕

Patch review

1. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -100,6 +100,48 @@
   +  Drupal.layoutBuilderBlockUpdate = function(item, from, to) {
   
   @@ -110,45 +152,13 @@
   -      const update = event => {
   ...
   -          onEnd: update,
   +          onEnd: event => Drupal.layoutBuilderBlockUpdate(event.item, event.from, event.to),
   

   👍

   This is changing an anonymous function into a named function.

   This anonymous function used to be called upon completing a drag event.

   Because our JS testing infrastructure does not allow us to trigger drag events, this enables us to still have test coverage: in our JS tests we just call this named function directly, instead of performing the drag event.

2. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/ContentPreviewToggleTest.php
   @@ -91,9 +92,13 @@ public function testContentPreviewToggle() {
   +    $driver->sortableDragAfter($links_block_selector, $body_block_selector);
   +    $driver->sortableCallback($links_block_selector, ".layout__region--content");
   

   🤔 Why are we still simulating some drag things if we can't make it work anyway? I feel like I'm missing something! 🤔

3. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
   @@ -162,12 +163,15 @@ public function testLayoutBuilderUi() {
   +    $driver->sortableDragTo('.layout__region--content .block-system-powered-by-block', '.layout__region--second');
   ...
   +    $driver->sortableCallback('.layout__region--second .block-system-powered-by-block', '.layout__region--content', '.layout__region--second');
   

   🤔 Same question here, really. The way this is written, it now seems like some things do work?

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/DrupalSelenium2Driver.php
   @@ -108,4 +108,65 @@ public function uploadFileAndGetRemoteFilePath($path) {
   +  public function sortableCallback($sourceSelector, $fromSelector, $toSelector = NULL) {
   ...
   +  Drupal.layoutBuilderBlockUpdate(sourceElement, fromElement, toElement)
   

   👎 We can't call a Layout Builder-specific callback in the generic DrupalSelenium2Driver.

Next step

I'd like @tedbow to sign off on this, AFAIK he's one of the people who has had the questionable privilege of having dug really deep in Drupal's JS testing and testing infrastructure while working on Layout Builder.

2, 3. These methods just manipulate the DOM directly, dragTo and dragAfter are not technically the correct "verbs" since the javascript in those methods is just appending an element elsewhere.

Would 2 - 4 be improved with the method naming? I used the "sortable" prefix to emphasize it was for this purpose, (as opposed to a generic "moveTo" which might look like it could be used in other tests in the future.

We can't call a Layout Builder-specific callback in the generic DrupalSelenium2Driver.

in this case- not only should these methods be named differently, they should also be moved elsewhere.

I talked with @tedbow this morning and he suggested these methods could be moved out of DrupalSelenium2Driver and into a new trait just for layout_builder functional javascript tests, since AFAIK this is the only place we need to make these overrides.

The methods I had added to DrupalSelenium2Driver are now moved into a trait specific to layout_builder.
The callback now gets fired by each of the sort methods directly instead of separately in the tests to avoid confusion.

This is now re-rolled on the latest 8.8.x which just had a large commit (3003cc6) to layout_builder, but there weren't any conflicts.

From my point of view Looking at the jquery have been created more issues in this year https://github.com/jquery/jquery/issues so it does not look like a project preparing a new major version. but I choose on the replacement library for jquery.ui.sortable library is https://github.com/SortableJS/ngx-sortablejs i thought it is simple method.

This patch is not work for me no updates are conflicted in layout_builder.

#27:

• Yes, I think a method name that explicitly contains the keyword "simulate" would help.
• But I still think it would be dangerous and inappropriate to add Layout Builder-specific work-arounds to a generically named helper method. Plus, it certainly would be unacceptable to call module-specific code in a generically named method that lives in a generic place. It would be acceptable for Layout Builder JS tests to override that generic method and also call this Layout Builder-specific JS though 😊

#28: This is much better! Patch review:

+++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/ContentPreviewToggleTest.php
@@ -91,9 +92,12 @@ public function testContentPreviewToggle() {
+    $assert_session->elementExists('css', "[data-layout-content-preview-placeholder-label='$links_field_placeholder_label'] div");
+    $assert_session->elementExists('css', "[data-layout-content-preview-placeholder-label='$body_field_placeholder_label'] div");
+
+    $links_block_selector = addslashes("[data-layout-content-preview-placeholder-label='$links_field_placeholder_label']");
+    $body_block_selector = addslashes("[data-layout-content-preview-placeholder-label='$body_field_placeholder_label']");

1. These are very long selectors. That's why I think in this case we should assign the selector to variables, which are then used by both the elementExists() assertions and the sortAfter() call.
2. Rather than calling addslashes() here, that's something sortAfter() could and should do.

Leaving assigned to @tedbow because I think he should be the one to RTBC this given his Layout Builder and JS test experience.

Thank you for the review @Wim Leers, I updated the patch.

1. Assigned long selectors to variables for re-use
2. Moved addslashes into the sortTo and sortAfter methods

I'm not sure why the test bot didn't run in #28. The results showed warnings about several ignored files and a few spacing issues, these are now fixed in this patch.

@j.gayathri Did you test this patch against the latest 8.8.x ? There was a core commit to layout_builder that was released the same day, maybe that was the cause of your conflict. If you have time would you test that this latest patch works for you? I'd appreciate your feedback since you agree that SortableJS is a good choice as the replacement.

1. #31 is making a lot more changes to the Layout Builder JS tests. Why is that? Ideally, we'd make the minimal test coverage changes necessary.
2. Still leaving assigned to @tedbow for review.
3. We still need to vet this new dependency. See #3036210: Add justinrainbow/json-schema as a composer dependency so JSON:API can use it to validate its responses and #3074267-22: Replace jQuery UI position() with PopperJS (and related https://github.com/FezVrasta/popper.js/issues/823) for examples. Could you get the dependency assessment going already? :)

1. I think we should create a follow up to create a Javascript Test for ckeditor sortable functionality. We don't have these tests now so I don't think it should block this issue but since we are changing how it actually works we could use a test for it.
2. In the follow-up or preferably in this issue it would be great to move sortTo() and sortAfter() out of the Drupal\Tests\layout_builder\FunctionalJavascript namespace all together. If not ckeditor would duplicate a lot of the code and they could diverge slightly so they aren't actually simulating the same thing.

   The tricky part as @zrpnr has described it to me is that we can't really know anything about what should be done in the sortUpdate().

   One way to solve this would be make a generic trait SortableTestTrait instead of LayoutBuilderSortTrait and move the current sortTo() and sortAfter() methods to this trait. Also add a abstract sortUpdate() method to this trait with the current signature.

   Then any test that wanted to use this trait would have to implement sortUpdate() to actually execute the JS script that needs to be called after a sort but it wouldn't have reimplement the actually simulation of the moving of the element.

   In the case of Layout Builder we could still have LayoutBuilderSortTrait but now it would only have to use SortableTestTrait and implement sortUpdate()(it could use the current method it has for this).

#33: Thanks for your feedback!

1. I searched the ckeditor.module component to check whether there was an existing issue, but there isn't yet. So +1. Tagging Needs followup. @zrpnr, can you create an issue for this? Thanks!

   I've done thorough manual testing here to ensure this is still working exactly the same as before.

2. I agree that that we may want to extract a generic test trait from LayoutBuilderSortTrait in that to-be-created issue.

That leaves the following things to be addressed:

1. #32.1
2. #32.3
3. The follow-up for CKEditor configuration UI test coverage.

#32.1 - The changes to LayoutBuilderTest were just assigning selector strings to variable names, as recommended for ContentPreviewToggleTest in #30.

For example:

• +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderTest.php
  @@ -158,33 +159,39 @@ public function testLayoutBuilderUi() {
  -    $assert_session->assertNoElementAfterWait('css', '.layout__region--second .block-system-powered-by-block');
  -    $assert_session->elementTextNotContains('css', '.layout__region--second', 'Powered by Drupal');
  +    $region_content = '.layout__region--content';
  +    $region_second = '.layout__region--second';
  +    $block_selector = '.block-system-powered-by-block';
  +    $block_text = 'Powered by Drupal';
  +
  +    $assert_session->assertNoElementAfterWait('css', $region_second . ' ' . $block_selector);
  +    $assert_session->elementTextNotContains('css', $region_second, $block_text);
  

But to avoid unnecessary changes to the test code that is now reverted in this patch.

#32.3 - I posted an issue for the dependency assessment at https://github.com/SortableJS/Sortable/issues/1614 and updated the IS.
That issue already has a very thorough and encouraging reply from one of the main contributors, owen-m1!

#34.1 I'll post this followup issue and link it here.

#34.2 @tedbow provided me with some example code and guidance to complement the comments in #33, this patch has the the DOM manipulating methods in a generic trait, that declares an abstract function for the callback. Then the methods can be reused when the functional test for ckeditor is updated. Now the LayoutBuilder specific trait only defines its own callback. I also updated the method names to make their purpose less ambiguous.

#31 I applied this patch and it making more changes in layout builder,its working fine.
Thank you @zrpnr for giving reply for my comment, if u have any available time i sent this https://github.com/SortableJS/ngx-sortablejs test this library.

The SortableJS maintainer responded: https://github.com/SortableJS/Sortable/issues/1614#issuecomment-525984712 — that all sounds very encouraging! 🥳 I asked a few follow-up questions.

+++ b/core/tests/Drupal/FunctionalJavascriptTests/SortableTestTrait.php
@@ -0,0 +1,92 @@
+ * Functions for simulating layout changes.
+ */
+trait SortableTestTrait {

Probably in this class comment we should explain why we have to do this simulation.

Can we provide a link the chromedriver or other issue that prevents the testing of native drag and drop? Is it correct that this issue was resolved we could remove this work around?

It would good to know in the future why we are using this versus the dragTo() method in the driver.

Other than that I think it is looking good

Can we provide a link the chromedriver or other issue that prevents the testing of native drag and drop? Is it correct that this issue was resolved we could remove this work around?

That's how I understand it, the dragTo methods in selenium don't work because ChromeDriver doesn't recognize/trigger those events.

https://bugs.chromium.org/p/chromium/issues/detail?id=850071

I'm not sure if the old Selenium test would automatically work once ChromeDriver updates, since it is firing some of its own javascript and the Syn library also contributes some simulated events. Also, I don't know how the testbot gets updated, I believe it currently using an older version of the ChromeDriver anyway since #3073342: JavaScript tests don't work with Chromedriver 75 and higher has not been a problem for it yet.

It might be a good idea to make a followup issue to check on this in the future and revert these tests if possible.

I updated the comment to include more information, maybe it should have the follow-up link too?

diff --git a/core/tests/Drupal/FunctionalJavascriptTests/SortableTestTrait.php b/core/tests/Drupal/FunctionalJavascriptTests/SortableTestTrait.php
index 6688bd9b74..58623686f6 100644
--- a/core/tests/Drupal/FunctionalJavascriptTests/SortableTestTrait.php
+++ b/core/tests/Drupal/FunctionalJavascriptTests/SortableTestTrait.php
@@ -4,6 +4,13 @@
 
 /**
  * Functions for simulating layout changes.
+ *
+ * The ChromeDriver does not currently support HTML5 drag and drop.
+ * These methods manipulate the DOM directly for automated testing.
+ * When the chromium bug is resolved, any tests using these methods
+ * should be able to revert to the selenium dragTo method.
+ *
+ * @link https://bugs.chromium.org/p/chromium/issues/detail?id=850071 Issue 850071: Drag and drop not working through chrome debug protocol.
  */
 trait SortableTestTrait {

@zrpnr thanks for the explanation.

The comment looks good but maybe we should explicitly say the current method should be deprecated when the bug is fixed.

And then yes we could create the follow-up now to deprecate the current trait when the bug is fixed. that issue could be postponed right now.

we should explicitly say the current method should be deprecated when the bug is fixed.
And then yes we could create the follow-up now to deprecate the current trait when the bug is fixed

I updated the comment with a link to #3078152: Follow-up to #3064049 Deprecate sortable Trait

The last requirement of this issue is to replace jQuery UI Sortable in media_library.
It is only used in the "widget".

Steps to test:

1. Add a multivalue media field to a content type
2. Add more than 1 media item to the field
3. Drag an item to a different position, save the node
4. On refresh item should be in the same position.

This patch also adds a deprecation message to core/jquery.ui.sortable similar to the ones in #3067251: Deprecate unused jQuery UI components in favour of a suite of contrib modules

I noticed a change in functionality. I tested this with layout builder

The dropzone(or placeholder?) without this patch is just an empty yellow box. With this patch it is still yellow but it actually has the contents of the of the block being patched.

For small blocks this doesn't seem so bad but when I moved the body field with a lot of test it is pretty big change.

in 8.8.x

with patch

Thanks for pointing that out @tedbow! We can get a review on that separately, but based on just some quick testing, it seems as this approach leads to better UX. Especially in the case of long fields, with the previous approach, the page jumped around quite a lot when dragging. With this approach, that effect is gone.

• RE: documenting the rationale for the work-around and linking to a follow-up to remove the work-around when the infrastructure has improved: 👍
• #42 is adding a deprecation message for core/jquery.ui.sortable, but not yet for core/jquery.ui.touch-punch
• #45 is an interesting twist on #44's feedback! Curious what the feedback will be from the Layout Builder maintainers! :D

1. +++ b/core/core.libraries.yml
   @@ -808,6 +808,7 @@ jquery.ui.sortable:
   +  deprecated: The "%library_id%" asset library is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. See https://www.drupal.org/project/drupal/issues/3064049
   
   +++ b/core/modules/ckeditor/ckeditor.libraries.yml
   @@ -52,13 +52,11 @@ drupal.ckeditor.admin:
   -    - core/jquery.ui.touch-punch
   

   👍 I think we should do the same for core/jquery.ui.touch-punch, this also removed the only use of that :)

2. +++ b/core/core.libraries.yml
   @@ -923,6 +924,15 @@ picturefill:
   +  version: "1.10.0-rc3"
   

   Can we also get a follow-up issue filed to update this to version 1.10.0? Per https://github.com/SortableJS/Sortable/issues/1614#issuecomment-526137463, we can expect that to happen before Drupal 8.8.0 ships.

3. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -100,6 +100,48 @@
   +  Drupal.layoutBuilderBlockUpdate = function(item, from, to) {
   

   🤔 This is callable by arbitrary JS code. Either we need to make that impossible or mark it @internal.

4. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderSortTrait.php
   @@ -0,0 +1,43 @@
   + * Trait LayoutBuilderSortTrait, calls Drupal.layoutBuilderBlockUpdate.
   

   🤔 This is not a very descriptive description.

5. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderSortTrait.php
   @@ -0,0 +1,43 @@
   + * @package Drupal\Tests\layout_builder\FunctionalJavascript
   

   🤓 Nit: This is auto-generated, should be removed.

6. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -1,7 +1,23 @@
   +  Drupal.MediaLibraryWidgetUpdate = widget => {
   

   🤔 Can't we make this name more descriptive?

   🤔 This is callable by arbitrary JS code. Either we need to make that impossible or mark it @internal.

7. +++ b/core/tests/Drupal/FunctionalJavascriptTests/SortableTestTrait.php
   @@ -0,0 +1,98 @@
   + * The ChromeDriver does not currently support HTML5 drag and drop.
   + * These methods manipulate the DOM directly for automated testing,
   + * and should be deprecated when the chromium bug is resolved.
   ...
   + * @link https://www.drupal.org/project/drupal/issues/3078152 Deprecate sortable Trait
   

   Nits:

   • s/The ChromeDriver/ChromeDriver/
   • Does not use the full 80 available columns.
   • s/chromium/Chromium/
   • Is it a Chromium bug or a ChromeDriver bug?
   • Use @see, not @link, and don't include the title of the page that the URL points to.

8. +++ b/core/tests/Drupal/FunctionalJavascriptTests/SortableTestTrait.php
   @@ -0,0 +1,98 @@
   +   * Callback for ajax update.
   

   🤔

   s/ajax/AJAX/

   Also, why is this about AJAX at all, that' snot what the method name suggests?

9. +++ b/core/tests/Drupal/FunctionalJavascriptTests/SortableTestTrait.php
   @@ -0,0 +1,98 @@
   +  protected function sortableTo($item, $from, $to) {
   +
   +    $item = addslashes($item);
   ...
   +  protected function sortableAfter($item, $target, $from) {
   +
   +    $item   = addslashes($item);
   

   Nit: extraneous empty lines.

Oh, this is interesting. We've started using this library in paragraphs a while ago for our drag and drop mode that allows to drag and drop paragraphs hierachically across nested fields (No, you don't want to know how that works, I still have nightmares about it).

However, we're still stuck on v 1.6 + a custom patch because we're seeing weird problems without that patch and with more recent versions. I'll try to provide some more information asap. The relevant PR is https://github.com/SortableJS/Sortable/pull/1154, that was closed as fixed in 1.8, but we're still seeing issues but haven't been able to fix that.

Thanks for the detailed review @Wim Leers!

Curious what the feedback will be from the Layout Builder maintainers! :D

Me too! 😅 The difference pointed out in #44 is because jQuery UI Sortable generates a placeholder element then moves the actual element with the pointer, while the HTML5 drag and drop used by Sortable leaves the element in place and has a "ghost" of that element moving with the pointer.
There are pros and cons with each, I tend to agree with #45 that the HTML5 approach looks better and causes fewer changes to the layout. However it can create challenges with styling. With jQuery UI the "placeholder" and the dragged element can each have unique styles, while the "ghost" from Sortable inherits the styles of the element before the drag starts.

1. Good catch! That's a nice bonus for this issue. Added the deprecation message to jquery.ui.touch-punch.
2. Created #3079015: Follow-up to #3064049 Update Sortable to 1.10.0
3. This was previously only callable within Drupal.behaviors.layoutBuilderBlockDrag but was exposed in #25 so it could be called by LayoutBuilderSortTrait. Marked @internal with a comment.
4. Updated the comment to be more descriptive: Trait LayoutBuilderSortTrait, provides callback for simulated layout change.
5. Wups, removed.
6. This callback updates the "weights" of all the MediaLibrary Items in the field widget. I changed the method name to MediaLibraryUpdateWeights and added more to the docblock. This was also pulled to a separate function so it could be called by the automated tests, assuming the follow-up from #34 for ckeditor testing happens. Marked @internal like layoutBuilderBlockUpdate
7. Updated the comment. As I understand it, it's a Chromium bug, specifically with the Chrome Debug Protocol. From https://bugs.chromium.org/p/chromium/issues/detail?id=850071 "ChromeDriver uses the Chrome Debug Protocol ..."
8. Removed reference to ajax, this was specific to layout builder. Updated the docblock, hopefully it is more clear.
9. Removed extra lines.

#47 I didn't realize Sortable was used in Paragraphs experimental widget.
Do you have concerns about this plan to add Sortable to core @Berdir? Since this is using 1.10 it does seem like it would conflict with the directions in the Paragraphs README to use 1.6 plus the patch from the PR you referenced.

There were some edits to media_library in 9d9fd0a since I had posted the previous patch.

core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php

testConfigurationValidation now uses dragTo

This is an interesting problem because media_library is testing the ckeditor admin page. That means there is some cross dependency- it's not the media "widget" that's being "dragged" but a ckeditor button.
Following the plan so far there should be a CKEditor use of the SortableTestTrait, but implemented in core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php.

Unfortunately the "callback" for ckeditor is not as straightforward to trigger separately due to it being inside a Backbone view.

(No, you don't want to know how that works, I still have nightmares about it).

😅

Unfortunately the "callback" for ckeditor is not as straightforward to trigger separately due to it being inside a Backbone view.

I think you can achieve this by manipulating the relevant Backbone models. Backbone models are the source of truth, Backbone views just render that truth. At least, that's the theory, if I remember correctly :) Happy to talk meet/chat about this to give pointers if needed.

Added a trait for CKEditor Admin sorting with a callback that updates the Backbone model directly. Thanks @Wim Leers for pointing me in the right direction!

1. +++ b/core/core.libraries.yml
   @@ -923,6 +925,15 @@ picturefill:
   +  version: "1.10.0-rc3"
   

   Can we add a

   # @todo Update to 1.10 in https://www.drupal.org/project/drupal/issues/3079015
   

   here?

2. +++ b/core/core.libraries.yml
   @@ -923,6 +925,15 @@ picturefill:
   +  license:
   +    name: MIT
   +    url: https://github.com/SortableJS/Sortable/tree/master#mit-license
   +  js:
   

   This is missing a gpl-compatible line.

3. #54's interdiff makes perfect sense — and thanks to @zrpnr for walking me through it :)

The IS indicates we still need to decide whether to provide a BC layer between this (very promising!) library and jquery.ui.sortable. Is that still the case or have we decided?

#58 This replacement has similar API options and methods but it is implemented differently than jQuery UI sortable. I don't think there has been any discussion on what a BC layer would look like, or where it would be needed.
Any contrib modules/themes using jQuery UI sortable would still work since the old asset library isn't being removed yet, and these 3 changes in core are fairly isolated.

Should there be a contrib module to provide the old jquery.ui.sortable asset library for any sites using jQuery UI Sortable in D9?

#55 Added the @todo and the gpl-compatible line to the core.libraries.yml

• #58
  

  The IS indicates we still need to decide whether to provide a BC layer between this (very promising!) library and jquery.ui.sortable. Is that still the case or have we decided?

  Good call! I do not think providing a BC layer here is realistic nor reasonable.

  Look at the vast API surface that jQuery UI Sortable provides: https://api.jqueryui.com/sortable/. Automatically translating all those possible API calls into API calls to https://github.com/SortableJS/Sortable is clearly unrealistic in my opinion.

  This means the issue summary needs an update.

• #60
  

  Should there be a contrib module to provide the old jquery.ui.sortable asset library for any sites using jQuery UI Sortable in D9?

  Yes, definitely! The only question is if we leave that for #3067261: [Plan] Remove jQuery UI components used by Drupal core and replace with a set of supported solutions or do it here. But it needs to happen either way.

• In #34, I asked for a follow-up. That follow-up doesn't exist yet. Could you create that one, @zrpnr. (I overlooked that in #55.)
• jQuery UI Sortable is used in 3 places in HEAD: the CKEditor configuration UI, the Media Library widget (\Drupal\media_library\Plugin\Field\FieldWidget\MediaLibraryWidget) and Layout Builder.
  1. I tested the CKEditor configuration UI in detail, including its accessibility. It behaves exactly the same: it still has the same keyboard navigation support, it still triggers the same Voice Over speech, and it still behaves the same for mouse users.
  2. I also tested the Media Library widget in detail. This too behaves exactly the same for mouse users. But this is not keyboard-accessible in HEAD, nor is it keyboard-accessible with this patch applied (due to tabindex=-1) — I think the relevant issue for this is #3041111: Items selected from the media library cannot be reordered by weight. In other words: changing the sorting cannot be done with the keyboard. In any case, fixing that is out of scope here.
  3. Finally, I also tested the Layout Builder's use of sorting in detail. Again it behaves exactly the same for mouse users. But this also is not keyboard-accessible in HEAD nor with this patch applies. In other words: changing the sorting cannot be done with the keyboard.

  Conclusion: accessibility & usability stay exactly the same.

NW for #61's first bullet and #62's first bullet.

Some clarification on #62.3 as it could potentially give the impression that there is no means to sort blocks in a keyboard-accessible manner in Layout Builder.

Finally, I also tested the Layout Builder's use of sorting in detail. Again it behaves exactly the same for mouse users. But this also is not keyboard-accessible in HEAD nor with this patch applies. In other words: changing the sorting cannot be done with the keyboard.

It is possible to sort blocks via keyboard navigation, but it not accomplished by a keyboard-enabled version of the drag and drop sort that leverages sortable. Instead, each block has a "move" contextual link that takes the user to a keyboard accessible means of moving blocks. That was implemented here #2995689: Allow reordering blocks without a pointer device (and for those curious, the discussion in the issue provides insight as to why this approach was chosen vs keyboard-enabling the drag/drop functionality)

Also worth noting that Layout Builder's keyboard-enabled block moving functionality does not use Sortable, so it not impacted by the changes in this issue. Rather, it uses tabledrag, which does not use sortable.
As an additional assurance that accessibility has not been compromised, the test coverage for Layout Builder's move block functionality: \Drupal\Tests\layout_builder\FunctionalJavascript\MoveBlockFormTest includes keyboard navigation.

#61.1 : Updated issue summary to reflect that a BC layer will not be provided.
#62.1 : Added followup issue #3081210: Add tests for CKEditor sortable functionality

It seems like Sortable 1.10 has been released. Let's update the patch to include that.

Updated to Sortable 1.10 and closed the no-longer-necessary followup issue.

YAY! And #67 closed #3079015: Follow-up to #3064049 Update Sortable to 1.10.0, 🥳

Looks like we still need JavaScript maintainer sign-off.

#Wim Leers, Hi i created one custom module i want to contribute in drupal.org i created project in drupal.org but how to add my custom module can u help me. i want like this

Development version: 8.x-4.x-dev updated 23 Apr 2019 at 18:13 UTC
Testing result: PHP 5.5 & MySQL 5.5, D8.7 23 pass all results

My project link:
https://www.drupal.org/project/ckeditor_rm

Marking this as critical just like other subissues deprecating jQuery UI components.

At first, I'm impressed by the UX improvements this library brings. I tested layout builder and I noticed immediately how much smoother and more accurate the sorting experience was. There's a video demonstrating this made by the author of the library as well.

The library seems well maintained and seems maintainable. It does have some integration tests, as well as browser compatibility tests. We might want to test manually browser compatibility of supported browsers that haven't been tested automatically. I tested this quickly on mobile but all I did was I confirmed that the library supports touch events.

1. +++ b/core/modules/ckeditor/js/views/VisualView.es6.js
   @@ -150,37 +150,23 @@
   -          if (!success) {
   -            // Cancel any sorting in the configuration area.
   -            view.$el
   -              .find('.ckeditor-toolbar-configuration')
   -              .find('.ui-sortable')
   -              .sortable('cancel');
   -          }
   

   This indeed looks like dead code. Any thoughts on moving removing this to a follow-up?

2. +++ b/core/modules/ckeditor/js/views/VisualView.es6.js
   @@ -188,66 +174,63 @@
   +        Array.prototype.forEach.call(
   +          this.el.querySelectorAll('.ckeditor-buttons:not(.js-sortable)'),
   ...
   +        Array.prototype.forEach.call(
   +          this.el.querySelectorAll('.ckeditor-toolbar-groups:not(.js-sortable)'),
   ...
   +        Array.prototype.forEach.call(
   +          this.el.querySelectorAll('.ckeditor-multiple-buttons:not(.js-sortable)'),
   

   Couldn't we just use the return value of the querySelectorAll to access the array prototype?

#70.1 There is a callback for Drupal.ckeditor.registerButtonMove but no callback for Drupal.ckeditor.registerGroupMove, so you're correct that this code was already dead before it was removed in this patch. I don't think there is a Sortable equivalent to sortable.cancel() which is meant to reset the element to its former position.

What would be need to be done in the follow-up?

#70.2 NodeLists do have a forEach method, which would make this section less verbose.
However it's not supported across all browsers. The example from the MDN docs recommends this pattern for IE compatibility, which calls a function on each item in the list.

var list = document.querySelectorAll('input[type=checkbox]');
Array.prototype.forEach.call(list, function (checkbox) {
  checkbox.checked = true;
});

#71.1 👍Thanks for explaining that. In that case we probably don't need a follow-up.
#71.2 👍Could we add a comment to mention that this is done for browser compatibility?

Added a comment explaining the use of Array.prototype.forEach() vs NodeList.forEach().

This library seems v nice 👍

+++ b/core/modules/ckeditor/js/views/VisualView.es6.js
@@ -188,66 +174,65 @@
+        Array.prototype.forEach.call(
+          this.el.querySelectorAll('.ckeditor-buttons:not(.js-sortable)'),

oof that's rough. We could use the small polyfill for IE11 instead of having to do these workarounds everywhere.
https://developer.mozilla.org/en-US/docs/Web/API/NodeList/forEach#Polyfill

@justafish Does that mean you're +1 to this library being added, it's just that forEach polyfill that you'd like to see used instead? 🤓 If so, could you please remove the Needs subsystem maintainer review tag? 🙏😀

#74I agree this isn't as nice looking as it could be if NodeList.forEach was supported in IE11. This same pattern is also used in message.es6.js, or sometimes just as a simple for loop like in misc/displace.es6.js or misc/active-link.es6.js.

Could improving the use of loops over NodeLists using a polyfill be a separate issue?

Given that this is critical, this:

Could improving the use of loops over NodeLists using a polyfill be a separate issue?

seems totally reasonable. It really is a tiny detail in the grand scheme of things.

The criticality of this issue is the removal of a dependency that is no longer actively maintained. That aspect of this patch was approved by JavaScript subsystem maintainer @justafish in #74. The only thing that remains is the potential use of a polyfill. I think that can easily be done in a non-critical follow-up.

Note: This isn't on this issue, but I know that @lauriii asked @justafish in chat whether she recommended a particular polyfill, and she was going to evaluate the available polyfills. That is why she had not yet removed the Needs subsystem maintainer review issue tag.

Sweet!

Thanks all for some great work here!

1. +++ b/core/core.libraries.yml
   @@ -816,6 +816,7 @@ jquery.ui.sortable:
   +  deprecated: The "%library_id%" asset library is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. See https://www.drupal.org/project/drupal/issues/3064049
   
   @@ -868,6 +869,7 @@ jquery.ui.touch-punch:
   +  deprecated: The "%library_id%" asset library is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. See https://www.drupal.org/project/drupal/issues/3064049
   

   This needs to link to a change notice, not the removal/deprecation issue

2. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -110,52 +154,15 @@
   +      context.querySelectorAll(regionSelector).forEach(region => {
   +        Sortable.create(region, {
   

   we're calling forEach on a NodeList here, which IE11 doesn't support, shouldn't we be doing as we do above - calling it on the from the Array class prototype?

3. +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderSortTrait.php
   @@ -0,0 +1,41 @@
   +    $to = $to ?? $from;
   

   nit: I think ?: is the correct operator here

Thanks for the review @larowlan!

#79.1 Created a change notice and updated these urls.
#79.2 Added this for IE11, this was one I missed when discussing #70 and #74, and I'll be sure to mention this in the the follow-up issue @Wim Leers recommended in #77.
#79.3 I think you're right! Made this change.

Created the followup issue requested by @Wim Leers in #77 to add the polyfill recommended by @justafish in #74

#3084805: Add JavaScript polyfill for NodeList.forEach

👍

I realized I never replied on in this:

#47 I didn't realize Sortable was used in Paragraphs experimental widget.
Do you have concerns about this plan to add Sortable to core @Berdir? Since this is using 1.10 it does seem like it would conflict with the directions in the Paragraphs README to use 1.6 plus the patch from the PR you referenced.

I'm.. conflicted. I assume that if something else tries to load the core library on the same page as we do then it will conflict, yes. i don't want to block this getting into core and my JS skills are very limited, so I can't help that much with it. It would be amazing if someone could help us figure out the problem that's affecting us so we could also use the core version. I'm happy to show how to reproduce it if someone is interested.

See #2903543: Make drag and drop compatible with SortableJS 1.10 for the paragraphs issue on how to reproduce the problems with a recent version, see README.txt on how it needs to be installed.

I'm.. conflicted. I assume that if something else tries to load the core library on the same page as we do then it will conflict, yes.

Well … this is why we use file-level closures in all JS in core :) So that you are in fact able to load multiple versions of the same library. With just a little bit of extra work. So worst case, you could still continue to load your patched version of this library.

Unrelated failures caused by a commit to HEAD yesterday.

It looks like this failed just once on Oct 5 and then passed 3 times since then.

I re-rolled this against current 8.8.x to keep it up to date, there were a few line changes in core/modules/media_library/media_library.libraries.yml and core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php.

Setting to needs review to make sure it all still passes against current HEAD.

Rebased, green again … back to RTBC.

Thanks for tagging this @mgifford.

The issue summary mentions accessibility review, but that doesn't help accessibility maintainers/contributors notice the issue. I'm surprised this wasn't at least tagged "accessibility" before RTBC, considering the library is used in several places, and the issue is ranked critical.

Scanning through the issue, I think this still needs testing. Wherever accessibility was mentioned, it's always in the context of keyboard. I haven't seen any mention of assistive tech or screen readers. I suggest at minimum this needs testing with screen readers on different OS.

I agree this can use more testing, however I'd recommend limiting the reviews to finding any regressions caused by switching jQuery UI sortable to SortableJS.
Drag and Drop has known difficulties with accessibility, which is one reason that alternative (keyboard) UI was built for ckeditor and layout_builder for example.

If there are problems identified with any of the 3 modules this issue touches it is important to determine whether those problems existed already or if they are caused by this javascript change.

If they were pre-existing problems, or if there are new improvements that might be suggested, maybe those would be better candidates for follow-up issues.

The 3 UIs in core that use jQuery UI sortable are the CKEditor toolbar configuration UI, Layout Builder and Media Library.

Accessibility review with screen reader did happen

This is inaccurate;

Scanning through the issue, I think this still needs testing. Wherever accessibility was mentioned, it's always in the context of keyboard. I haven't seen any mention of assistive tech or screen readers. I suggest at minimum this needs testing with screen readers on different OS.

See my comment #62 from a month ago:

1. I tested the CKEditor configuration UI in detail, including its accessibility. It behaves exactly the same: it still has the same keyboard navigation support, it still triggers the same Voice Over speech, and it still behaves the same for mouse users.
2. I also tested the Media Library widget in detail. This too behaves exactly the same for mouse users. But this is not keyboard-accessible in HEAD, nor is it keyboard-accessible with this patch applied (due to tabindex=-1) — I think the relevant issue for this is #3041111: Items selected from the media library cannot be reordered by weight. In other words: changing the sorting cannot be done with the keyboard. In any case, fixing that is out of scope here.
3. Finally, I also tested the Layout Builder's use of sorting in detail. Again it behaves exactly the same for mouse users. But this also is not keyboard-accessible in HEAD nor with this patch applies. In other words: changing the sorting cannot be done with the keyboard.

Conclusion: accessibility & usability stay exactly the same.

I did test it all with VoiceOver turned on. Neither Media Library nor Layout Builder's sorting are keyboard-accessible, and so neither is accessible today.

More detail & history about the CKEditor module's use of jQuery UI sortable/Sortable.js

The CKEditor toolbar configuration UI is the one that introduced this into Drupal core. I worked on that >5 years ago. The responsibility of jQuery UI Sortable there was solely the keyboard interaction and mouse drag-and-drop. But

Screen reader support is the responsibility of core/modules/ckeditor/js/views/AuralView.es6.js. The very first commit of the CKEditor module to Drupal core already included Drupal.announce()-like logic (#1890502: WYSIWYG: Add CKEditor module to core), it was then refactored to have a single implementation of that instead of half a dozen (#1915302: Provide an API for aria-live region update announcements that module developers may leverage), and the concept of "visual vs aural view" objects in JS was introduced to stress the importance of providing a great pure auditive-only UX (#2174589: Split up ckeditor.admin.js), which is a pattern that exists in several Drupal core modules. If you look at that JS, you'll see that it's doing announcements not just for a few things, but to truly guide the user around the user interface, to describe what is possible and to describe what is happening.

Yes, I know that screen reader support is more than just announcements. But I refer back to the testing above: the screen reader behavior was identical.

Conclusion

We've confirmed there are no keyboard regressions, we know for a fact that the announcements are still the same (both by logical reasoning and manual testing), and we also know for a fact that jQuery UI sortable is unmaintained.

A lot of work has gone into evaluation of JS libraries, updating core to use it, verifying the user experience is the same for all users (visual & aural, keyboard & mouse). It was indeed not tested by an accessibility maintainer, but it was thoroughly tested by one of the people who is a subsystem maintainer of the only subsystem in Drupal core that actually uses this in a way that works. I trust that this is adequate.

To add to the excellent summary in #93, Layout Builder and Media Library are relatively new in Drupal core, and there are active plans for both to improve accessibility.

The media library widget has: #2988431: [Meta] Accessibility plan for Media Library Widget

As @Wim Leers mentioned there is no accessible "sorting" for Layout Builder, however @bnjmnm explained in #64 that Layout Builder does not rely on sortable for accessible layout changes, and instead uses contextual links #2995689: Allow reordering blocks without a pointer device

for those curious, the discussion in the issue provides insight as to why this approach was chosen vs keyboard-enabling the drag/drop functionality

That UI had extensive discussion and revisions around accessibility, and there is a a follow-up to improve it further #3042107: Layout Builder actions are inconvenient to access via keyboard navigation

Replacing jQuery UI sortable with a more modern and well supported library won't affect these existing plans.

@Wim Leers pointed out that we don't have a contrib module for jQuery UI sortable. We should add one so that there's a smooth upgrade for anyone who doesn't have time to refactor their JS to use Sortable.js. NWing for that.

Also the release note should definitely not be "none"; please always fill out that IS section even if you don't think it needs one. This release manager will often disagree with that assessment. :) Thanks!

Forgot to link the reference: https://www.drupal.org/issue-summaries#release-notes You can also compare with what I wrote for Popper.js on the other issue.

I manually tested this and I didn't find it disruptive. I actually think it's an improvement, to be honest. It "feels" less janky than jQuery UI's Sortable. Speaking as a Media subsystem maintainer, +1 from me.

(Beware, the attached screenshot includes profanity.)

Sortable contrib module added
https://www.drupal.org/project/jquery_ui_sortable
Currently it's only available as a development version as I wanted a different contributor to review and deem release worthy. Added @zrpnr as a maintainer, so he's probably the most suitable person for that step.

Added release notes.

Sortable contrib module added

Thanks @bnjmnm, this matches the format of all the other contrib modules exactly, depends on the contrib jquery_ui module and has the same folder structure with the included sortable-min.js file. I tested this manually by changing layout_builder.libraries.yml to depend on this module instead of core/jquery.ui.sortable and it worked!

This was RTBC in #93 but needed release notes and the contrib module, which are provided in #101

I reviewed the Layout Builder changes in detail, and +1 to RTBC!

Did some more work on the change record: https://www.drupal.org/node/3084730/revisions/view/11608136/11608211

I tried to search for contrib projects affected by the events change but I couldn't find any instances where contrib would try to attach events on any of the core usages.

+++ b/core/modules/layout_builder/js/layout-builder.es6.js
@@ -100,6 +100,50 @@
+   * @internal This method should only be called by layoutBuilderBlockDrag.

Why is this marked as internal? Is there a specific reason we couldn't make this function local if it's only supposed to be used by one place?

Edit: I see this was discussed earlier. Could we document that it's internal, not local because our testing trait needs to be able to call this?

This needs a reroll because #3074267: Replace jQuery UI position() with PopperJS landed.

Reroll of #88. ⛑🚒

Release note was added in #102. Thank you @bnjmnm!

Oops, NW for #107.

I updated the @internal comments for Drupal.layoutBuilderBlockUpdate and Drupal.MediaLibraryUpdateWeights.
Also added a link to the CR which describes the test trait and has links to the chromedrive issues.

1. +++ b/core/modules/layout_builder/js/layout-builder.es6.js
   @@ -110,7 +110,9 @@
   +   * @internal This method is a callback for layoutBuilderBlockDrag and is used
   +   *  in FunctionalJavascript tests. It may be renamed if the test changes.
   +   *  @see https://www.drupal.org/node/3084730
   

   This is a nice improvement!

   To make this even more clear, I would add @see reference to Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderSortTrait. Maybe we should say This may be moved to a local variable after Layout Builder tests no longer need this.

2. +++ b/core/modules/media_library/js/media_library.widget.es6.js
   @@ -9,7 +9,9 @@
   +   * @internal This method is a callback for MediaLibraryWidgetSortable and is
   +   * used in FunctionalJavascript tests. It may be renamed if the test changes.
   +   * @see https://www.drupal.org/node/3084730
       */
      Drupal.MediaLibraryUpdateWeights = widget => {
   

   I couldn't find any usages of this from the tests. Does this need to be exposed publicly like this?

3. +++ b/core/modules/ckeditor/tests/src/Traits/CKEditorAdminSortTrait.php
   @@ -0,0 +1,34 @@
   + * Trait CKEditorAdminSortTrait, provides callback for simulated layout change.
   
   +++ b/core/modules/layout_builder/tests/src/FunctionalJavascript/LayoutBuilderSortTrait.php
   @@ -0,0 +1,41 @@
   + * Trait LayoutBuilderSortTrait, provides callback for simulated layout change.
   
   +++ b/core/tests/Drupal/FunctionalJavascriptTests/SortableTestTrait.php
   @@ -0,0 +1,96 @@
   + * Trait SortableTestTrait, provides functions for simulating layout changes.
   

   Let's remove the trait name from the beginning of the comment. It doesn't add any extra information and it adds more work when class names are being changed.

Could we also document in the change record how someone should rewrite their code if they have attached events to jQuery UI Sortable by binding events to an element. An example of before code would be something like:

$('.selector').on('sortchange', (event, ui) => {});

All my concerns here have been addressed (#114 helps with them also). Untagging; once @lauriii's feedback and any other points are addressed, any committer could potentially review this for commit.

Does Paragraphs require an update to avoid errors with this fix? If so, we should probably mention that in the release note and CR.

rerolled off latest 8.9.x as of 36d0e5c

#113.1 Updated the comment, thanks for the suggestion
#113.2 You're right, it does not. I think it was made like that assuming there would be a similar callback. The media_library test coincidentally uses the CKEditorAdminSortTrait since it is the CKEditorIntegrationTest.
Since this doesn't need to be exposed now, I moved the function inline with the onEnd.
#113.3 Removed the word Trait from the comments.

Could we also document in the change record how someone should rewrite their code

I'll work on this now.

Does Paragraphs require an update to avoid errors with this fix?

In #85, @Wim Leers wrote:

you are in fact able to load multiple versions of the same library. With just a little bit of extra work. So worst case, you could still continue to load your patched version of this library.

There shouldn't be any conflicts with having both, and particularly because it is a problem just with paragraphs experimental.
The only way I can think of that both libraries could even be loaded would be having a media library widget inside a nested paragraph. And, again in #85:

we use file-level closures in all JS in core

Release notes were added in #102 but the tag was added back in #116 for the concern about Paragraphs, I think it's safe to remove that tag again.

#114 Added an example to explain example js changes with links to API docs for both projects.

What Wim said was:

With just a little bit of extra work

...which implies that Paragraphs would need to be changed, and therefore, updating Paragraphs to a new release of that module might be required to avoid JS errors on the page, if the library is attached with too global a scope in Paragraphs currently. If so, it'll still need to be mentioned. :) So let's keep the tag for now until @Berdir can confirm.

@zrpnr, do you know why the patch got 20K bigger with the last attachment?

--- /dev/null
+++ b/core/core.libraries.yml.orig

Oh, this is why.

Let's remove please. :)

Removed the extra file, must have missed it after a rebase 😳
Back to ~80kb

let's keep the tag for now until @Berdir can confirm

Totally makes sense, thanks

I made some minor documentation improvements. I also updated the change record to specifically show examples of adding events to Sortable outside of the options that are given on the initialization of the library because that seems most relevant to contrib.

I walked through the Paragraphs problem with @Wim Leers. Paragraphs is only using Sortable in an experimental widget. The problem that we have is that both core and Paragraphs are loading the Sortable library to window.Sortable. Luckily this by itself doesn't cause any errors. However, Paragraphs recommends installing an older version of Sortable which we haven't tested with core. It seems like Paragraphs is always loading the library with ajax, which means that it will be always loaded after the Drupal core shipped version of Sortable. This is good for Paragraphs because they have more specific version requirements in this case.

I'm not sure how many sites are actually using the older version of Sortable because the Paragraphs’ README has contradictory statements, and Paragraphs code does not check the version of Sortable.

I confirmed manually that Media Library works with the Sortable 1.6.0 version which is mentioned in the README to make sure that there are no regressions.

The conclusion is that there's no disruption and there's no need for a new release of Paragraphs. However, it would be great to have the Paragraphs maintainers verify whether Sortable 1.10 (which ships with core) fixes it, and if not, help fix it upstream, so that Paragraphs can start using the Sortable version shipped in core.

I think all of my feedback and concerns have been addressed so moving this to the RTBC queue.

I'm not sure exactly what I need to confirm/say now.

The reason why we use the old version + patch is in #47, as well as #83/#84 has the reference to the paragraphs issue that has steps on how to reproduce this.

I did test the latest version of the library and we still have problems (Note the original problem for which we needed the patch, that was apparently fixed and the related issue closed), see that issue.

My JS knowledge is very limited, but someone from our team will investigate that loading stuff and see if we can change how we load it so that it doesn't conflict as a minimal fix.

> Paragraphs is only using Sortable in an experimental widget.

Worth pointing out that this "experimental" widget is just a patch away from becoming the recommended/default widget, there's an issue to do that already :)

As I said earlier, I'm fine with this going into core, but I'd still appreciate any help we can get on this. Maybe it's as "simple" as updating our JS code to work with the new versions, maybe it's a bug there.

Tweaking the release note a little. Release notes should usually end with an action, i.e., what should I do if I encounter this problem?

I checked with @Berdir and he is OK with the release note we have now.

@bnjmnm pointed out that "Sortable" needs to be added to the list of globals in .eslintrc

#129👍

🦅👁 — go @bnjmnm!

So, good news from the Paragraphs front, looks like @sasanikolic found a fix in #2903543: Make drag and drop compatible with SortableJS 1.10 that allows us to use the most recent version as well, which means we will eventually be able to deprecate our own library and just use the one from core.

🥳 Wonderful news! Thanks, @Berdir and @sasanikolic! Crediting you both :)

deleted, wrong issue

Updated issue credits

Ran the fixers so the patch complies with coding standards.

This issue is 100% approved for backport to 8.8.x prior to 8.8.0-beta1.

Committed 4fb98eb and pushed to 9.0.x, 8.9.x, and 8.8.x. Thanks!