Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update jQuery UI to 1.13.0 #5401

Closed
23 of 26 tasks
indigoxela opened this issue Dec 16, 2021 · 25 comments · Fixed by backdrop/backdrop#3868
Closed
23 of 26 tasks

Update jQuery UI to 1.13.0 #5401

indigoxela opened this issue Dec 16, 2021 · 25 comments · Fixed by backdrop/backdrop#3868

Comments

@indigoxela
Copy link
Member

indigoxela commented Dec 16, 2021

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

  • Added a custom block
  • Moved another block

Field UI

  • Changed the order of fields on one of the Manage Display pages

Layout UI

  • for path node/% does context appear automatically?
  • drag and drop blocks
  • Added a custom block
  • Moved another block

Modules

Better Exposed Filters

  • datepicker
  • slider

CKEditor

  • link dialog
  • image dialog
  • Configuration: Added buttons to the editor toolbar
  • Sortable
  • Draggable

Coffee

  • autocomplete

Draggable Views

  • draggable
  • droppable

Elements

  • autocomplete

Rules

  • autocomplete
  • button

Views UI

  • Added a view
  • Changed to the display of fields
  • Modal windows
  • Rearranged the fields

Weight

  • draggable
  • droppable

advocate: Jen Lampton.

@quicksketch
Copy link
Member

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
Copy link
Member Author

indigoxela commented Dec 17, 2021

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

I was afraid, this would be the answer 😁

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)
#!/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
Copy link
Member Author

indigoxela commented Dec 17, 2021

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. 😞
How can we handle this? It might be a breaking change or maybe not. 🤷

@indigoxela
Copy link
Member Author

indigoxela commented Dec 17, 2021

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

@indigoxela
Copy link
Member Author

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

@herbdool
Copy link

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
Copy link
Member Author

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: jquery/jquery-ui@7c6a9f0

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
Copy link
Member Author

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 jquery/jquery-ui@512cbbf

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
Copy link
Member Author

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
Copy link
Member Author

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

@indigoxela
Copy link
Member Author

CKEditor uses sortable and draggable in its admin form for toolbar configuration. Still works 👍

@Wylbur
Copy link
Member

Wylbur commented Dec 23, 2021

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

@klonos
Copy link
Member

klonos commented Dec 25, 2021

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
Copy link
Member

jenlampton commented Dec 30, 2021

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
Copy link
Member Author

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
Copy link
Member Author

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

@indigoxela
Copy link
Member Author

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

@indigoxela
Copy link
Member Author

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
Copy link
Member

quicksketch commented Jan 3, 2022

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: backdrop/backdrop@53fe496

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
Copy link
Contributor

laryn commented Jan 3, 2022

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 backdrop/backdrop#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
Copy link
Member

Yay! A big thanks to @indigoxela!

@stpaultim
Copy link
Member

Thanks @indigoxela !!!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants