search

backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

Update jQuery UI to 1.13.0 #5401

Closed indigoxela closed 2 years ago

indigoxela commented 2 years ago

There's already a meta issue for updating both, jQuery and jQuery UI, but this one deserves its own. There are some problems to solve.

"Recently" jQuery UI released a new version:

https://github.com/jquery/jquery-ui/releases/tag/1.13.0

The problem is: how to get it into Backdrop?

Backdrop has individual minified js and css files under core/misc/ui. All filenames are prefixed with jquery.ui.

The official downloads only provide a single minified js file (concatenated). The sources have a different structure and the files are not minified.

The only solution that comes to my mind: get the sources (not minified), rename and reorder all files manually.

Is that the right approach?

I also tried to get info about how this has been done before, but unfortunately got no answer. Possibly nobody remembers, how this has to be done?

jquery UI functions in Backdrop

Here's a checklist of functions in core and contrib that use jquery UI

Core

Blocks

Field UI

Layout UI

Modules

Better Exposed Filters

CKEditor

Coffee

Draggable Views

Elements

Rules

Views UI

Weight

advocate: Jen Lampton.

quicksketch commented 2 years ago

The only solution that comes to my mind: get the sources (not minified), rename and reorder all files manually.

Yep, I believe that is the correct and only approach we have available. I think we need to run them through a compresssor like uglify as well. I think this has been the case for several releases of jQuery UI, since they rearranged their packaged distribution several times since Drupal/Backdrop started including it.

indigoxela commented 2 years ago

Yep, I believe that is the correct and only approach we have available.

I was afraid, this would be the answer :grin:

So these were my steps: I set up a VM for that job, as I didn't want to bloat my dev box

apt-get install npm
export NODE_PATH=/usr/local/lib/node_modules # not sure about that
npm install --global uglify-js
npm -g list

wget https://github.com/jquery/jquery-ui/archive/refs/tags/1.13.0.zip
unzip -q 1.13.0.zip

Handling files manually seemed too error-prone, so I created a build script:

build-jquery-ui.sh (click to see the full build script) ```sh #!/bin/bash # General check, without uglifyjs nothing is possible. if ! command -v uglifyjs &> /dev/null then echo 'Fatal: uglifyjs not found in PATH' echo 'Hint: npm -g install uglifyjs' exit 1 fi # Default value, can be overridden via command line. TARGET=ui # We need at least one argument, the source directory. if [ $# -lt 1 ] then echo 'Usage: build-jquery-ui.sh -s path/to/source [ -t target/dir ]' exit 1 fi while getopts ":s:t:" opt do case $opt in s) if [ ! -d $OPTARG ] then echo 'Source directory not found:' $OPTARG exit 1 fi SRC="$OPTARG" ;; t) TARGET="$OPTARG" ;; \?) echo "Invalid option -$OPTARG" >&2 exit 1 ;; esac case $OPTARG in -*) echo "Option $opt needs a valid argument" exit 1 ;; esac done # Create target directories if they don't exist yet. if [ ! -d $TARGET -o ! -d $TARGET/i18n ] then echo 'Creating direcory structure' mkdir -p $TARGET/i18n fi # Main directory. Uglify and rename files. ls -1 $SRC/ui/*js | while read LINE do BASENAME="${LINE##*/}" OUT=$TARGET/jquery.ui."${BASENAME%.js}".min.js echo Creating $OUT uglifyjs $LINE --compress --mangle --comments '/^\/*!/' -o $OUT done # Effects are in a subdirectory, but go to the same directory. ls -1 $SRC/ui/effects/*js | while read LINE do BASENAME="${LINE##*/}" OUT=$TARGET/jquery.ui."${BASENAME%.js}".min.js echo Creating $OUT uglifyjs $LINE --compress --mangle --comments '/^\/*!/' -o $OUT done # Widgets also go to the same directory. ls -1 $SRC/ui/widgets/*js | while read LINE do BASENAME="${LINE##*/}" OUT=$TARGET/jquery.ui."${BASENAME%.js}".min.js echo Creating $OUT uglifyjs $LINE --compress --mangle --comments '/^\/*!/' -o $OUT done # Translations are in a subdirectory, file names stay the same. ls -1 $SRC/ui/i18n/*js | while read LINE do BASENAME="${LINE##*/}" OUT=$TARGET/i18n/$BASENAME echo Creating $OUT uglifyjs $LINE --compress --mangle -o $OUT done # Images just get copied. echo "Copy image directory" cp -r $SRC/themes/base/images/ $TARGET # Base theme css files. ls -1 $SRC/themes/base/*css | while read LINE do BASENAME="${LINE##*/}" OUT=$TARGET/jquery.ui.$BASENAME echo Copy $OUT cp $LINE $OUT done # This file seems to be new in 1.13.0. # Possibly effects depend on it? # @todo Check if and where it's in use. if [ ! -d $TARGET/vendor/jquery-color ] then mkdir -p $TARGET/vendor/jquery-color fi SUBPATH=vendor/jquery-color/jquery.color.js echo Creating $TARGET/$SUBPATH uglifyjs $SRC/ui/$SUBPATH --compress --mangle --comments '/^\/*!/' -o $TARGET/$SUBPATH exit 0 ```

Then ran that to create the desired structure and file names.

build-jquery-ui.sh -s jquery-ui-1.13.0

UPDATE: this build script is outdated, as a different file structure has been requested.

indigoxela commented 2 years ago

There are some new files:

    ui/i18n/datepicker-de-AT.js
    ui/jquery.ui.jquery-patch.min.js
    ui/jquery.ui.jquery-var-for-color.min.js
    ui/vendor/jquery-color/jquery.color.js

The color plugin seems new, and possibly some of the effects depend on it?

This will be very difficult to test and IMO impossible to review. All minified. :disappointed: How can we handle this? It might be a breaking change or maybe not. :shrug:

indigoxela commented 2 years ago

First problem: all css files need to get updated with the appropriate filenames in @import. Done.

indigoxela commented 2 years ago

Nice... the datepicker widget and layout admin drag-and-drop still work. :relieved:

herbdool commented 2 years ago

Nice work! That's a good list of steps and scripts to keep around for next time.

We could compile a list of widgets to manually check that rely on this. Including major contrib modules.

Might need to just review on a high level, based on your documents steps.

indigoxela commented 2 years ago

Re the new files mentioned before:

    ui/jquery.ui.jquery-patch.min.js

That seems to be the successor to jquery.ui.escape-selector.min.js (escape-selector.js). It contains compatibility stuff for jQuery versions before 2.2 (we have 1.12.4), so possibly it should be included instead of core/misc/ui/jquery.ui.escape-selector.min.js in system_library_info(). Need to check for that.

Assumptions are based on info from this commit: https://github.com/jquery/jquery-ui/commit/7c6a9f01281a9739f54ef57d7deecb41a873ef38

Note that escape-selector.js doesn't ship with jQuery UI anymore, so it probably has to go in B.

    ui/jquery.ui.jquery-var-for-color.min.js
    ui/vendor/jquery-color/jquery.color.js

I'm still investigating, but jquery.color.js seems to address compatibility with more recent jQuery versions (3+?). It seems related to effect.js (our jquery.ui.effect.min.js), but I'm not yet sure, in how far.

Both are currently not considered in system_library_info(). Should we add it?

indigoxela commented 2 years ago

Re ui/vendor/jquery-color/jquery.color.js that code used to be part of effect.js, but has been moved out of it.

Assumption based on https://github.com/jquery/jquery-ui/commit/512cbbf4d9d039519ef9c94716d7048ca2ff582f

We probably do not need jquery.ui.jquery-var-for-color.min.js, as we do include jQuery before that, so that would end up as the noop described in the comment: https://github.com/jquery/jquery-ui/blob/main/ui/jquery-var-for-color.js

indigoxela commented 2 years ago

We could compile a list of widgets to manually check that rely on this. Including major contrib modules.

I found rules/ui/ui.theme.inc, which includes ui, ui.autocomplete and ui.button. Need to check what they actually use.

@herbdool are you aware of any other contrib module that uses jQuery UI libraries from core?

indigoxela commented 2 years ago

So I checked Rules in the sandbox. The autocomplete of data-selectors uses jQuery UI, and that still works. :+1: I've no idea how to trigger a rules debug message, though - never used that with Rules.

indigoxela commented 2 years ago

CKEditor uses sortable and draggable in its admin form for toolbar configuration. Still works :+1:

argiepiano commented 2 years ago

@herbdool are you aware of any other contrib module that uses jQuery UI libraries from core?

These are a few that I've used:

herbdool commented 2 years ago

We might be able to use GitHub API to search all contrib modules. I haven't tried it.

indigoxela commented 2 years ago

I just tested Coffee (autocomplete) - it still works :+1:

And also checked DaggableViews (draggable/droppable) - and that also works :+1:

Wylbur commented 2 years ago

I compiled checklist from comments in this thread, and the original meta-issue. Feel free to update and improve!

klonos commented 2 years ago

Thanks for all your work on this @indigoxela 🙏🏼

I think that we should merge this in as soon as possible. It will allow us plenty of time to test things in the rest of the PR sandboxes till Jan 15.

jenlampton commented 2 years ago

I did a quick code review and (correct me if I'm wrong) it looks like the only changes in the PR are to the jQuery UI library itself - no changes to integration code. 👍 I'm working on hands-on testing now, but this looks great to me so far. I'm adding it to the milestone, and removing the label.

[edit:] Also adding myself as advocate because I'd like to see this in 1.21 (and my other issue is now in near-ready state)

indigoxela commented 2 years ago

I corrected the checklist a bit:

Node Edit

author name autocomplete
post date pop-up calendar

None of these uses jQuery UI, the autocomplete is a Backdrop autocomplete, the pop-up calendar for post date is a html5 form element (input date).

indigoxela commented 2 years ago

Another slight correction: module CKEditor Accordion uses normal jQuery (slideUp/slideDown...) and does not use jQuery UI at all (as maintainer I should know :wink:).

indigoxela commented 2 years ago

@quicksketch jquery.color is now a separate library as requested, and effects depends on it. Ready for another round of review.

indigoxela commented 2 years ago

Directory structure changed as requested. The build script is now outdated and needs some update.

I'm not sure if it would be safe to use a different jquery.color version than jQuery UI ships with (in their vendor subdir). Should we document, where this file actually comes from? And to not update it independently? Not sure.

quicksketch commented 2 years ago

Thanks @indigoxela! I tested this locally on all the core situations listed in the summary and found everything to be working the same as before.

Touch support seems to be poor when using touch mode in desktop Firefox and Chrome: it is not possible to move blocks in the Layout UI. However, this is an existing issue and probably something that needs to be corrected in Touch Punch or a future version of jQuery UI. I confirmed drag and drop still works on my Android phone in Chrome and Firefox.

Should we document, where this file actually comes from? And to not update it independently?

I think that wouldn't hurt. My guess is that the two libraries will likely be released mostly in tandem with each other anyway. I pushed up a small commit with additional inline code documentation to your PR: https://github.com/backdrop/backdrop/pull/3868/commits/53fe496d1c464ee5c712b1f80933e5cb46ebfd25

I'm comfortable with merging this work as it is now. We need to get this in before 1.21.0, and merging it today will include it in the 1.21.0-preview release and hopefully expose us to a bit more testing.

laryn commented 2 years ago

Thank you @indigoxela for filing and fixing! Thanks to @quicksketch for major review and testing, and for pitching in @klonos, @jenlampton, @Wylbur, @herbdool and @argiepiano. I've merged https://github.com/backdrop/backdrop/pull/3868 into 1.x for 1.21.0 -- let's kick the tires a little more in 1.21.0-preview before the formal release.

quicksketch commented 2 years ago

Yay! A big thanks to @indigoxela!

stpaultim commented 2 years ago

Thanks @indigoxela !!!!!