Conversation
reiern70
force-pushed
the
reiern70/issue-1618-10x-broken-palette
branch
from
September 26, 2026 14:17
eb415ef to
2aeef1b
Compare
Wicket.Palette.moveUpHelper called box.trigger('focus'), but box is a
plain DOM <select> element returned by document.getElementById, not a
jQuery object. Clicking the "move up" button on an ordered Palette
threw a TypeError before reaching updateRecorder, so the hidden
recorder input never picked up the new order and the reordering was
discarded on the next render. The "move down" button was unaffected
since moveDownHelper has no equivalent call.
This was introduced by the jQuery 4.0.0 migration, which rewrote
box.focus() as box.trigger('focus'). master is unaffected because it
later dropped jQuery from this file entirely and regained
box.focus() incidentally. This fix restores box.focus() on
wicket-10.x, matching what master does today.
Adds a QUnit harness for wicket-extensions' client-side JavaScript
(none existed; only wicket-core's was wired into the grunt/js-test
build) and a regression test asserting that moveUp reorders the
selection and updates the recorder without throwing.
GitHub issue #1618
Wicket.trapFocus's keydown handler resolves the first and last
focusable elements with $focusable.get(0) and
$focusable.get($focusable.length - 1), which are plain DOM elements,
then called .trigger('focus') on them. Tabbing past the last
focusable element (or Shift+Tabbing past the first) inside a
focus-trapped container - e.g. a ModalDialog - threw a TypeError
instead of wrapping focus around, so keyboard users got stuck instead
of cycling back to the other end of the trap.
The other two calls in the same file already went through
findFocusable(...).first(), which returns a jQuery object, so they
were unaffected. This is the same class of bug as the Palette "move
up" issue: both were introduced by the jQuery 4.0.0 migration, which
mechanically rewrote box.focus() as box.trigger('focus') without
checking whether the receiver was a jQuery object. master does not
have this bug; it later dropped jQuery from this file and uses
.focus() directly.
Adds a QUnit regression test asserting that Tab/Shift+Tab at the
trap's boundaries move focus to the other end without throwing.
GitHub issue #1620
reiern70
force-pushed
the
reiern70/issue-1618-10x-broken-palette
branch
from
September 26, 2026 14:27
2aeef1b to
cc29182
Compare
2 tasks
papegaaij
approved these changes
Sep 26, 2026
papegaaij
left a comment
papegaaij
left a comment
Contributor
There was a problem hiding this comment.
Looks good to me. Used GPT-6 to review this.
bitstorm
approved these changes
Sep 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The jQuery 4.0.0 migration (WICKET-7179) mechanically rewrote several
box.focus()calls asbox.trigger('focus'), without checking whether the receiver was actually a jQuery object. A follow-up fix already corrected two spots (wicket-ajax-jquery.js,trap-focus.js's focus restore), but missed two others, both of which throw aTypeErroronwicket-10.x(10.9.0+):Wicket.Palette.moveUpHelperinpalette.jscallsbox.trigger('focus')whereboxis a plain DOM<select>fromdocument.getElementById. Clicking the "move up" button on an orderedPalettethrows before the hidden recorder input is updated, so the reorder is silently discarded on the next render. "move down" is unaffected. Fixes GitHub issue Palette "move up" broken since 10.9.0: moveUpHelper calls box.trigger('focus') on a plain DOM element #1618.Wicket.trapFocus's keydown handler intrap-focus.jscalls.trigger('focus')on$focusable.get(0)/.get(length - 1), which are plain DOM elements (unlike the file's other two calls, which go through.first()and are fine). Tabbing/Shift-tabbing past the boundary of a focus-trapped container (e.g. aModalDialog) throws instead of wrapping focus around.masteris unaffected by either — it later dropped jQuery from these files entirely and uses.focus()directly.wicket-9.xandwicket-8.xpredate the jQuery 4 migration and also use.focus(). So both fixes arewicket-10.x-only.Changes
wicket-extensions/src/test/js/), wired into the existinggrunt/-Pjs-testbuild via a secondconnectserver target (port 38888) — previously only wicket-core's JS was exercised at runtime; wicket-extensions JS was only linted.Test plan
grunt jshint connect qunitintesting/wicket-js-tests— 230 tests, 0 failed (jQuery 3.7.1 and 4.0.0).TypeError) and pass after the fix.