-
Notifications
You must be signed in to change notification settings - Fork 17
Fix 2 failing Jasmine tests: scroll position and normalized text highlighting #79
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
Conversation
…lighting Co-authored-by: JJJ <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR attempts to fix two failing Jasmine tests in the jQuery test suite: one related to scroll position adjustment for dropup/dropdown switching, and another related to highlighting normalized search matches. While the fixes to the CoffeeScript source code and jQuery compiled output are correct, the Prototype version was not properly updated, leaving it with the same bugs.
Key Issues:
- The normalized text highlighting fix in
coffee/lib/abstract-chosen.coffeewas correctly implemented but only compiled to the jQuery version - The scroll position test fix was only applied to the jQuery test spec, not the Prototype test spec
- The Prototype compiled JavaScript files (
docs/chosen.proto.jsanddist/js/chosen.proto.js) were not regenerated with the bug fixes
Changes Made:
- Fixed scroll position test to scroll in the correct direction (down instead of up)
- Corrected highlighting algorithm to highlight the full matched portion when text normalization is used
- Added comprehensive comments explaining the O(n²) algorithm complexity
Reviewed changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
spec/jquery/scroll-position.spec.coffee |
Fixed scroll direction and improved assertions, but Prototype equivalent not updated |
coffee/lib/abstract-chosen.coffee |
Corrected highlighting algorithm to use full match length with text normalization |
docs/chosen.jquery.js |
jQuery compiled output correctly includes the highlighting fix |
dist/js/chosen.jquery.js |
jQuery distribution file correctly includes the highlighting fix |
docs/chosen.proto.js |
Only header comment removed; highlighting fix not compiled into this file |
docs/chosen.css.map |
Source map regenerated with updated relative path |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (function() { | ||
| var AbstractChosen, SelectParser; | ||
|
|
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Prototype version in docs/ was not properly rebuilt with the normalized text highlighting fix. Only the header comment was removed, but the actual bug fix from coffee/lib/abstract-chosen.coffee (lines 240-267) was not applied.
The file needs to be completely regenerated by running npx grunt build to compile the CoffeeScript changes into this JavaScript file. The current version still contains the old buggy highlighting algorithm that doesn't correctly highlight the full matched portion when using normalize_search_text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 08aded7. The Prototype version has been properly rebuilt with the normalized text highlighting fix from abstract-chosen.coffee. All highlighting logic changes are now applied to both jQuery and Prototype compiled JavaScript files.
| # Scroll down (increase scroll position) to move element up and create more space below | ||
| window.scrollTo(0, scrollToMakeDropup + 300) |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scroll position test fix was only applied to the jQuery version. The equivalent Prototype test in spec/proto/scroll-position.spec.coffee at line 33 has the same bug (scrolling from 2000 to 1500, which is scrolling UP, not down).
According to the PR checklist, both jQuery and Prototype versions should be updated. The Prototype test should also be fixed to scroll in the correct direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 08aded7. The Prototype scroll position test now:
- Scrolls DOWN (increases scroll position from scrollToMakeDropup to scrollToMakeDropup + 300) to move element up on screen
- Uses dynamic positioning based on window height (matching jQuery approach)
- Properly accesses the chosen instance by capturing the return value of
new Chosen() - Uses
container_mousedown()with mock events instead ofcontainer.fire()
Both "should switch from drop-up to drop-down when scrolling down" and "should switch from drop-down to drop-up when scrolling up" tests now pass.
…lighting Co-authored-by: JJJ <[email protected]>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Summary
Fixes two test failures in the Jasmine test suite for both jQuery and Prototype versions:
Scroll Position Test
The test "should switch from drop-up to drop-down when scrolling down" was scrolling in the wrong direction (decreasing scroll position instead of increasing). Changed to scroll down by 300px to move the element up on screen and verify the correct dropup state is achieved. Applied to both jQuery (
spec/jquery/scroll-position.spec.coffee) and Prototype (spec/proto/scroll-position.spec.coffee) test suites.Normalized Text Highlighting
When using
normalize_search_text(e.g., for accent removal), the highlighting algorithm only highlighted query-length characters instead of the full matched portion. Searching "test" against "Testé" would produce<em>Test</em>éinstead of<em>Testé</em>.The fix in
coffee/lib/abstract-chosen.coffee:Prototype-Specific Updates
dist/js/chosen.proto.jsanddocs/chosen.proto.jswith the highlighting algorithm fixnew Chosen()container_mousedown()with mock eventsPlease double-check that:
package.json.References
Related to CI test failures mentioned in issue discussions.
Original prompt
This section details on the original issue you should resolve
<issue_title>Fix remaining 2 Jasmine failures</issue_title>
<issue_description>```
Running "jasmine:jquery" (jasmine) task
Puppeteer old Headless deprecation warning:
In the near feature
headless: truewill default to the new Headless modefor Chrome instead of the old Headless implementation. For more
information, please see https://developer.chrome.com/articles/new-headless/.
Consider opting in early by passing `headless: "new"` to `puppeteer.launch()`
If you encounter any bugs, please report them to https://github.com/puppeteer/puppeteer/issues/new/choose.
Testing specs with Jasmine/latest via Chrome/142.0.7444.175
Width handling
min-width for short values
- should allow wider content to expand naturally......✓
- should apply min-width to single select containers......✓
- should display 4-digit years without truncation......✓
- should display short values without truncation......✓
Scroll Position Adjustment
Expected true not to be true. (1)
Basic setup
data-placeholder
disabled fieldset
Bugfixes
include_group_label_in_selectedoption harvesthq/chosen#2996 - XSS Vulnerability withinclude_group_label_in_selected: true......✓Events
search
Searching
Expected 'Testé' to contain 'Testé'. (1)
respects word boundaries when not using search_contains
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.