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

Resizable: Fix content shrink on resize #2281

Merged
merged 9 commits into from
Sep 9, 2024

Conversation

Daniel-Garmig
Copy link
Contributor

Fixes: #2277.
This end up being a bug on "resizable", and as I have shown on the issue it's quite a weird behavior and it's not consistent across browsers.

The same bug applies to the resizable element and "alsoResize" elements, so I fixed it on both places.

Dialog widget uses "alsoResize" property to update dialog contents size (as seen on the initial issue). This PR fixes that as well.

I have added some tests for both "box-sizing: content-box" and "box-sizing: border-box".

Resizable element shrinks on resize when it has scrollbars and "box-sizing: content-box".

Fixes: jquerygh-2277
Copy link

@melloware melloware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome!

@melloware
Copy link

@Daniel-Garmig do you know a way I can "MonkeyPatch" your fix on top of existing jQuery UI? We do stuff like this in our app to patch other jquery UI functions but the magic is never quite the same for all patches.

https://github.com/primefaces/primefaces/blob/master/primefaces/src/main/resources/META-INF/resources/primefaces/jquery/jquery.ui.pfextensions.js

I would like to be able to apply this patch until its accepted by jQueryUI team or (gulp) rejected.

tests/unit/resizable/core.js Outdated Show resolved Hide resolved
tests/unit/resizable/core.js Outdated Show resolved Hide resolved
tests/unit/resizable/core.js Outdated Show resolved Hide resolved
ui/widgets/resizable.js Outdated Show resolved Hide resolved
ui/widgets/resizable.js Outdated Show resolved Hide resolved
@Daniel-Garmig
Copy link
Contributor Author

@mgol I have changed the method to avoid using getBoundingClientRect().
Instead I get the value using parseFloat( element.css( "width" ) );, but without the scrollbars. So I disable scrollbars, get the correct computed size and set scrollbars back. (This seems kind of expensive and slow to me, but idk).

I tried to optimize it, so this calculation is only done when necessary. After a resize, width and height properties will be added as inline css to the element, so I will use those when available to avoid the expensive calculation.

I added some test for resizable elements with transform properties set. It doesn't matter after this changes but why not.

Let me know what do you think.

tests/unit/resizable/core.js Outdated Show resolved Hide resolved
ui/widgets/resizable.js Outdated Show resolved Hide resolved
Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more comments.

tests/unit/resizable/core.js Outdated Show resolved Hide resolved
ui/widgets/resizable.js Outdated Show resolved Hide resolved
ui/widgets/resizable.js Outdated Show resolved Hide resolved
ui/widgets/resizable.js Outdated Show resolved Hide resolved
tests/unit/resizable/core.js Show resolved Hide resolved
Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost ready! Just two more comments from me.

tests/unit/resizable/core.js Outdated Show resolved Hide resolved
tests/unit/resizable/core.js Outdated Show resolved Hide resolved
@Daniel-Garmig
Copy link
Contributor Author

Changes done!
@mgol Thank you for all your suggestions and comments, they were great!

Copy link
Member

@mgol mgol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good!

@mgol mgol requested a review from fnagel September 9, 2024 15:49
Copy link
Member

@fnagel fnagel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 by reading and by running the unit tests (can confirm the new test will fail in Chrome if the fix is not present)

@mgol mgol merged commit c934995 into jquery:main Sep 9, 2024
9 checks passed
@mgol mgol added this to the 1.14.1 milestone Sep 9, 2024
@mgol
Copy link
Member

mgol commented Sep 9, 2024

Landed, thanks!

@mgol
Copy link
Member

mgol commented Oct 30, 2024

jQuery UI 1.14.1 with this change has been released.

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

Successfully merging this pull request may close these issues.

Dialog: Content shrinks on resizing when box-sizing: content-box
4 participants