Problem/Motivation
We are in the process of deprecating jQuery UI in core. The jQueryUI sortable library is among the components that need to be removed/replaced.
As mentioned in the parent issue: #3067261: [Plan] Remove jQuery UI components used by Drupal core and replace with a set of supported solutions
The OpenJS Foundation lists jQuery UI as an Emeritus project in https://openjsf.org/projects/ which is described as:
Emeritus projects are those which the maintainers feel have reached or are nearing end-of-life
3 of Drupal core's modules currently depend on jquery.ui.sortable
- ckeditor
- layout_builder
- media_library
Proposed resolution
- Sortable https://github.com/SortableJS/Sortable was chosen because it seems popular (in terms of github stars and forks), is actively maintained (looking at recent issue fixes and releases), and the comparison video looks impressive.
- Switch ckeditor, media_library, and layout_builder to use the replacement library.
- Deprecate jquery.ui.sortable (e.g., emit a deprecation notice for modules/themes that depend on it).
- Deprecate jquery.ui.touch-punch which is only used by jquery.ui.sortable in media_library.
SortableJS/Sortable
-
- Code quality: documented, comprehensive tests
- Maintainership of the package: recent releases (latest release was in 26 Jun 2019)
- https://github.com/SortableJS/Sortable/releases
- 119 contributors https://github.com/SortableJS/Sortable/graphs/contributors
- License: MIT
- Security policies of the package: Not formally documented. The maintainer prefers a private email about any security issues with the package and is comfortable following coordinated disclosure with us. https://github.com/SortableJS/Sortable/issues/1614
- Expected release and support cycles: undocumented; sporadic releases, asked in https://github.com/SortableJS/Sortable/issues/1614. Maintainer is confident an updated minor version will be released before Drupal 8.8. The maintainer also intends a future 2.x major version, and will continue to support 1.x after it is released.
- Other dependencies it'd add, if any: none
Remaining tasks
Decide on the replacement library.Patch the affected core modules (ckeditor, media_library, layout_builder)Validate the prototype with JS maintainersSee #74.Finish patch-
Accessibility and usability review
See #62.
User interface changes
None.
API changes
Sortable has a different JS API than jquery.ui.sortable. We still need to decide whether we need to provide a bridge/façade, or whether it's ok to require contrib modules to change their JS code to the new API.The decision was no BC layer , rationale can be found at #61- A contrib module providing the jquery.ui.sortable asset library could also be created
Data model changes
None.
Release notes snippet
The CKEditor, Layout Builder, and Media Library modules previously depended on the jQuery UI sortable asset library. Since jQuery UI is end-of-life, we've removed the dependency on jQuery UI sortable and added a dependency on the SortableJS library instead. It's recommended that any modules or themes that use jQuery UI sortable also update their code to use SortableJS. Modules extending CKEditor, Media Library or Layout Builder may also require small changes.
The Paragraphs contributed project uses an older version of Sortable for its experimental widget. There is a chance that this could lead into version conflicts when both the Paragraphs experimental widget and the core library are used on the same page (for example, on a form with the Media Library). If you notice JavaScript issues when using the Paragraphs experimental widget in 8.8, report them in the Paragraphs issue queue.
Comment | File | Size | Author |
---|---|---|---|
#137 | 3064049-136.patch | 80.35 KB | alexpott |
#137 | 129-136-interdiff.txt | 2.99 KB | alexpott |
#129 | interdiff_123-129.txt | 230 bytes | zrpnr |
#129 | 3064049-129.patch | 80.27 KB | zrpnr |
#123 | interdiff.txt | 1.88 KB | lauriii |
Comments
Comment #2
xjmComment #3
finnsky CreditAttribution: finnsky at Skilld commentedAdded quick implementation. For ckeditor buttons.
First buttons managed with sortable.js and second with jqueryUI.
Comment #4
andypostComment #5
zrpnrSortable 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:
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.
Comment #6
andypostDid it pass a11y review?
Comment #8
lauriiiComment #9
zrpnrThe 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:
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.
Comment #10
zrpnrComment #11
Wim LeersPatch review
Is this class name arbitrarily chosen or does it have special meaning?
Why this change?
Comment #12
finnsky CreditAttribution: finnsky at Skilld commented@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:)
Comment #13
Wim Leerscore/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?Comment #14
finnsky CreditAttribution: finnsky at Skilld commented@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.
Comment #15
Wim LeersAll 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.
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.Comment #16
zrpnrThe 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
andcore/themes/stable/css/ckeditor/ckeditor.admin.css
Comment #17
zrpnrI 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.
Comment #19
Wim Leers#16:
#17:
Comment #20
zrpnrSortable 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?
Comment #21
andypostIs there method to mark tests skipped if some requirements are not met?
Comment #22
Wim Leers#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()
.Comment #23
zrpnrIf you are testing FunctionalJavascript tests and starting with a newer install of chromedriver make sure to add
{ "chromeOptions": { "w3c": false } }
to yourMINK_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
Comment #24
Wim LeersComment #25
zrpnrThe 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
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.
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.
Comment #26
Wim Leers😲🤯 What a mess! I'm sorry you have to bring clarity in this chaos 😔
Indeed, apparently we don't have any other choice. 😕
Patch review
👍
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 thedrag
event.🤔 Why are we still simulating some drag things if we can't make it work anyway? I feel like I'm missing something! 🤔
🤔 Same question here, really. The way this is written, it now seems like some things do work?
👎 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.
Comment #27
zrpnr2, 3. These methods just manipulate the DOM directly,
dragTo
anddragAfter
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.
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.
Comment #28
zrpnrThe 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.
Comment #29
Gayathri J CreditAttribution: Gayathri J commentedFrom 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.
Comment #30
Wim Leers#27:
#28: This is much better! Patch review:
elementExists()
assertions and thesortAfter()
call.addslashes()
here, that's somethingsortAfter()
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.
Comment #31
zrpnrThank you for the review @Wim Leers, I updated the patch.
addslashes
into thesortTo
andsortAfter
methodsI'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.
Comment #32
Wim LeersComment #33
tedbowsortTo()
andsortAfter()
out of theDrupal\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 ofLayoutBuilderSortTrait
and move the currentsortTo()
andsortAfter()
methods to this trait. Also add a abstractsortUpdate()
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 useSortableTestTrait
and implementsortUpdate()
(it could use the current method it has for this).Comment #34
Wim Leers#33: Thanks for your feedback!
ckeditor.module
component to check whether there was an existing issue, but there isn't yet. So +1. Tagging . @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.
LayoutBuilderSortTrait
in that to-be-created issue.That leaves the following things to be addressed:
Comment #35
zrpnrComment #36
zrpnr#32.1 - The changes to LayoutBuilderTest were just assigning selector strings to variable names, as recommended for ContentPreviewToggleTest in #30.
For example:
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.
Comment #37
Gayathri J CreditAttribution: Gayathri J commented#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.
Comment #38
Wim LeersThe SortableJS maintainer responded: https://github.com/SortableJS/Sortable/issues/1614#issuecomment-525984712 — that all sounds very encouraging! 🥳 I asked a few follow-up questions.
Comment #39
tedbowProbably 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
Comment #40
zrpnrThat'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?
Comment #41
tedbow@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.
Comment #42
zrpnrI 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:
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 modulesComment #43
zrpnrComment #44
tedbowI 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
Comment #45
lauriiiThanks 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.
Comment #46
Wim Leerscore/jquery.ui.sortable
, but not yet forcore/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 :)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.
🤔 This is callable by arbitrary JS code. Either we need to make that impossible or mark it
@internal
.🤔 This is not a very descriptive description.
🤓 Nit: This is auto-generated, should be removed.
🤔 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
.Nits:
@see
, not@link
, and don't include the title of the page that the URL points to.🤔
s/ajax/AJAX/
Also, why is this about AJAX at all, that' snot what the method name suggests?
Nit: extraneous empty lines.
Comment #47
BerdirOh, 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.
Comment #48
zrpnrThanks for the detailed review @Wim Leers!
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.
Drupal.behaviors.layoutBuilderBlockDrag
but was exposed in #25 so it could be called byLayoutBuilderSortTrait
. Marked @internal with a comment.Trait LayoutBuilderSortTrait, provides callback for simulated layout change.
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 likelayoutBuilderBlockUpdate
#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.
Comment #50
zrpnrThere were some edits to
media_library
in9d9fd0a
since I had posted the previous patch.core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
testConfigurationValidation
now usesdragTo
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 incore/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.
Comment #51
Wim Leers😅
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.
Comment #52
zrpnrComment #53
zrpnrComment #54
zrpnrAdded 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!
Comment #55
Wim LeersCan we add a
here?
This is missing a
gpl-compatible
line.Comment #56
xjmComment #57
xjmComment #58
xjmThe 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?Comment #59
xjmComment #60
zrpnr#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
Comment #61
Wim LeersGood 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.
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.
Comment #62
Wim Leers\Drupal\media_library\Plugin\Field\FieldWidget\MediaLibraryWidget
) and Layout Builder.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.Conclusion: accessibility & usability stay exactly the same.
Comment #63
Wim LeersNW for #61's first bullet and #62's first bullet.
Comment #64
bnjmnmSome 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.
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.Comment #65
bnjmnm#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
Comment #66
lauriiiIt seems like Sortable 1.10 has been released. Let's update the patch to include that.
Comment #67
bnjmnmUpdated to Sortable 1.10 and closed the no-longer-necessary followup issue.
Comment #68
Wim LeersYAY! And #67 closed #3079015: Follow-up to #3064049 Update Sortable to 1.10.0, 🥳
Looks like we still need JavaScript maintainer sign-off.
Comment #69
Gayathri J CreditAttribution: Gayathri J commented#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
Comment #70
lauriiiMarking 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.
This indeed looks like dead code. Any thoughts on moving removing this to a follow-up?
Couldn't we just use the return value of the querySelectorAll to access the array prototype?
Comment #71
zrpnr#70.1 There is a callback for
Drupal.ckeditor.registerButtonMove
but no callback forDrupal.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 aSortable
equivalent tosortable.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.
Comment #72
lauriii#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?
Comment #73
zrpnrAdded a comment explaining the use of
Array.prototype.forEach()
vsNodeList.forEach()
.Comment #74
justafishThis library seems v nice 👍
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
Comment #75
Wim Leers@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 tag? 🙏😀Comment #76
zrpnr#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 inmessage.es6.js
, or sometimes just as a simplefor
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?
Comment #77
Wim LeersGiven that this is critical, this:
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
issue tag.Comment #78
xjmSweet!
Comment #79
larowlanThanks all for some great work here!
This needs to link to a change notice, not the removal/deprecation issue
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?
nit: I think ?: is the correct operator here
Comment #80
zrpnrThanks 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.
Comment #81
zrpnrCreated 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
Comment #82
Wim Leers👍
Comment #83
BerdirI realized I never replied on in this:
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.
Comment #84
BerdirSee #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.
Comment #85
Wim LeersWell … 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.
Comment #87
Wim LeersUnrelated failures caused by a commit to HEAD yesterday.
Comment #88
zrpnrIt 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
andcore/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
.Setting to needs review to make sure it all still passes against current HEAD.
Comment #89
Wim LeersRebased, green again … back to RTBC.
Comment #90
mgiffordComment #91
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer commentedThanks 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.
Comment #92
zrpnrI 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.
Comment #93
Wim LeersThe 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;
See my comment #62 from a month ago:
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 includedDrupal.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.
Comment #94
zrpnrTo 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
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.
Comment #95
Gayathri J CreditAttribution: Gayathri J commentedComment #96
Gayathri J CreditAttribution: Gayathri J at UniMity Solutions Pvt Limited commentedComment #98
xjm@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!
Comment #99
xjmForgot 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.
Comment #100
phenaproximaI 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.)
Comment #101
bnjmnmSortable 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.
Comment #102
bnjmnmAdded release notes.
Comment #103
xjmComment #104
zrpnrThanks @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
Comment #105
tim.plunkettI reviewed the Layout Builder changes in detail, and +1 to RTBC!
Comment #106
xjmDid some more work on the change record: https://www.drupal.org/node/3084730/revisions/view/11608136/11608211
Comment #107
lauriiiI 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.
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?
Comment #108
lauriiiThis needs a reroll because #3074267: Replace jQuery UI position() with PopperJS landed.
Comment #109
lauriiiReroll of #88. ⛑🚒
Comment #110
lauriiiRelease note was added in #102. Thank you @bnjmnm!
Comment #111
lauriiiOops, NW for #107.
Comment #112
zrpnrI updated the @internal comments for
Drupal.layoutBuilderBlockUpdate
andDrupal.MediaLibraryUpdateWeights
.Also added a link to the CR which describes the test trait and has links to the chromedrive issues.
Comment #113
lauriiiThis 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 sayThis may be moved to a local variable after Layout Builder tests no longer need this.
I couldn't find any usages of this from the tests. Does this need to be exposed publicly like this?
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.
Comment #114
lauriiiCould 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:
Comment #115
xjmAll 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.
Comment #116
xjmDoes Paragraphs require an update to avoid errors with this fix? If so, we should probably mention that in the release note and CR.
Comment #117
zrpnrrerolled 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 theCKEditorIntegrationTest
.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.
I'll work on this now.
In #85, @Wim Leers wrote:
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:
Comment #118
zrpnrRelease 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.
Comment #119
xjmWhat Wim said was:
...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.
Comment #120
xjm@zrpnr, do you know why the patch got 20K bigger with the last attachment?
Comment #121
xjmOh, this is why.
Let's remove please. :)
Comment #122
zrpnrRemoved the extra file, must have missed it after a rebase 😳
Back to ~80kb
Totally makes sense, thanks
Comment #123
lauriiiI 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.
Comment #124
lauriiiComment #125
BerdirI'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.
Comment #126
xjmTweaking the release note a little. Release notes should usually end with an action, i.e., what should I do if I encounter this problem?
Comment #127
xjmI checked with @Berdir and he is OK with the release note we have now.
Comment #128
xjmComment #129
zrpnr@bnjmnm pointed out that "Sortable" needs to be added to the list of globals in .eslintrc
Comment #130
bnjmnm#129👍
Comment #131
Wim Leers🦅👁 — go @bnjmnm!
Comment #132
BerdirSo, 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.
Comment #134
Wim Leers🥳 Wonderful news! Thanks, @Berdir and @sasanikolic! Crediting you both :)
Comment #135
catchdeleted, wrong issue
Comment #136
lauriiiUpdated issue credits
Comment #137
alexpottRan the fixers so the patch complies with coding standards.
Comment #138
xjmThis issue is 100% approved for backport to 8.8.x prior to 8.8.0-beta1.
Comment #142
lauriiiCommitted 4fb98eb and pushed to 9.0.x, 8.9.x, and 8.8.x. Thanks!
Comment #143
xjm