[#47689] - [7.0] Fix sorting for data-nested="false" in draggable list #47675
- Pending
- Medium
- Build: 5.4-dev
- # 47689
- Diff
- RiteshGite:fix/isNested-boolean-conversion
User tests: Successful: Unsuccessful:
Pull Request resolves #47675
- I read the Generative AI policy and my contribution is either not created with the help of AI or is compatible with the policy and GNU/GPL 2 or later.
Summary of Changes
Fixed the isNested variable to properly handle data-nested="false" attribute by converting the string value to a boolean.
Testing Instructions
- Set
data-nested="false"on a draggable table - Try to drag and drop rows
- Verify that sorting works correctly
- Test with
data-nested="true"to ensure nesting still works
Actual result BEFORE applying this Pull Request
When data-nested="false" is set, drag-and-drop sorting does not work because the string "false" is treated as truthy in JavaScript, causing the condition if (isNested) to incorrectly evaluate to true.
Expected result AFTER applying this Pull Request
Both data-nested="true" and data-nested="false" work correctly. The fix explicitly converts the dataset string value to a boolean using isNested = container.dataset.nested === 'true';
Link to documentations
- No documentation changes for guide.joomla.org needed
- No documentation changes for manual.joomla.org needed
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | JavaScript Repository NPM Change |
| Labels |
Added:
NPM Resource Changed
bug
PR-5.4-dev
|
||
muhme
- | Title |
|
||||||
Thanks for your Contribution @RiteshGite.
Could you please give us more explained( a bit in detail) Testing Instruction to test this PR?
@RiteshGite Technically, your fix is correct. However, changing this behavior now could cause many views in Joomla core (such as Categories, Menu Items, Modules, etc.) to suddenly stop working as expected, because they currently use data-nested="false" in places where it should be data-nested="true".
A few examples:
https://github.com/joomla/joomla-cms/blob/5.4-dev/administrator/components/com_categories/tmpl/categories/default.php#L130
https://github.com/joomla/joomla-cms/blob/5.4-dev/administrator/components/com_menus/tmpl/items/default.php#L105
So at the moment, this JS bug actually allows buggy core code (and probably some third-party extensions as well) to continue working as expected :)
I think the safer approach would be:
- First identify and fix all existing code which has data-nested="false" by mistake (Modules, Categories....)
- Then change the JS behavior afterward, ideally in Joomla 7 (or possibly 6.2 I'm unsure).
Thanks for the guidance!
I've created PR #47780 which addresses the first step you mentioned.
This PR fixes the data-nested attributes in Categories and Menu Items
components that have parent-child hierarchies.
Once this PR is merged, the JavaScript fix in this PR (#47689) can be
safely applied without breaking any existing functionality.
The approach:
- PR #47780 - Fix data-nested attributes in core components ✓
- PR #47689 - Fix JavaScript boolean conversion (this PR)
Looking forward to your review!
Thanks for the feedback!
I've added more detailed testing instructions in the related PR #47780, covering step-by-step testing for nested drag-and-drop behavior in Categories, Menu Items, and Modules.
Please let me know if you think more scenarios should be covered.
Hi @RiteshGite
Change on this PR is counted backward incompatible, so the earliest version we can make this change is Joomla 7. For the time being, I would suggest we fix wrong usages of data-nested in our core code (as you did in PR #47780), and worry about this later (I saw somewhere we might switch to use a different library for drag and drop in J7)
@joomdonation Understood.
We'll keep the core data-nested fixes in #47780 for now, and revisit the JS behavior change later if needed.
I have tested this item ✅ successfully on 5a3d1e7
Works as described.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/47689.
I have tested this item ✅ successfully on 5a3d1e7
Works as described.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/47689.
I have not tested this item.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/47689.
I have not tested this item.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/47689.
I have tested this item ✅ successfully on 5a3d1e7
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/47689.
I have tested this item ✅ successfully on 5a3d1e7
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/47689.
Not setting RTC (ready to commit) despite of the 2 successful human tests because as clearly pointed out above by @joomdonation , it is a b/c break and so has to be moved (or rebased) to 7.0-dev.
We had discussed that also in the CMS Maintenance Team a while ago and came to the same conclusion.
@RiteshGite Please rebase this PR to 7.0-dev (or if rebase doesn't work wekk, just redo it). Thanks in advance.
| Labels |
Added:
PBF
|
||
| Category | JavaScript Repository NPM Change | ⇒ | Repository Administration com_admin SQL |
![avatar github-actions[bot]](/images/avatars/github-actions[bot].png)
This pull request has conflicts, please resolve those before we can evaluate the pull request.
| Title |
|
||||||
| Category | Repository Administration com_admin SQL | ⇒ | JavaScript NPM Change |
| Labels |
Added:
PR-7.0-dev
Removed: PBF PR-5.4-dev |
||
Please use the Pull Request-Template.