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

Jump to bottom

Tooltip: Don't crash on empty content #1994

Merged
merged 7 commits into from Nov 8, 2021
Merged

Tooltip: Don't crash on empty content #1994

merged 7 commits into from Nov 8, 2021

Conversation

josepsanzcamp
Copy link
Contributor

The 1.13.0 release remove the try-catch that prevents spurious errors

Fixes: 1f2011e
Closes: gh-1990

@josepsanzcamp
Widget & Tooltip: Regression from 1.12.1
c5aef9b 
The 1.13.0 release remove the try-catch that prevents spurious errors

Fixes: 1f2011e
Closes: gh-1990
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 8, 2021 •
edited

CLA Signed

The committers are authorized under a signed CLA.

caugner
caugner approved these changes Oct 11, 2021
View reviewed changes
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM

mgol
mgol requested changes Oct 11, 2021
View reviewed changes
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 for the PR! Please see my comment.

ui/widget.js Outdated Show resolved Hide resolved
@josepsanzcamp
Widget & Tooltip: Regression from 1.12.1
1a9947f 
Removed the previous sanctioning crashing code in favor to control the real problem inside the tooltip widget, thanks to the @mgol clarification.

Fixes: c5aef9b
Closes: gh-1990
caugner
caugner reviewed Oct 12, 2021
View reviewed changes
ui/widgets/tooltip.js Outdated Show resolved Hide resolved
@josepsanzcamp @caugner
Update ui/widgets/tooltip.js
dea398b 
Co-authored-by: Claas Augner <github@caugner.de>
caugner
caugner approved these changes Oct 13, 2021
View reviewed changes
Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

mgol
mgol requested changes Oct 14, 2021
View reviewed changes
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.

One remark to source; also, let's still add a test as I mentioned in my original comment.

ui/widgets/tooltip.js Outdated Show resolved Hide resolved
@josepsanzcamp @mgol
Update ui/widgets/tooltip.js
8a07975 
Co-authored-by: Michał Gołębiowski-Owczarek <m.goleb@gmail.com>
@mgol
Copy link
Member

mgol commented Oct 15, 2021

@josepsanzcamp Can you add a unit test for this change? This could be something similar to the issue reported in #1990, written in tests/unit/tooltip/core.js.

@mgol
Copy link
Member

mgol commented Oct 27, 2021

@josepsanzcamp Hey, are you willing to finish this PR, i.e. write a unit test? It'd be good to get this released soon, especially now that security advisories against <1.13.0 are out.

@josepsanzcamp
Widget & Tooltip: Regression from 1.12.1
f8f320a 
Add a unit test for the #1990 and the PR#1994
@josepsanzcamp
Copy link
Contributor Author

josepsanzcamp commented Oct 30, 2021 •
edited

Hi @mgol.

Sorry by my delay, I have committed an unit test proposal.

Do you have some comment???

Thanks in advance.

Josep.

mgol
mgol approved these changes Nov 7, 2021
View reviewed changes
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.

Looks good to me now, thanks! I'll leave it open for a few days to see if we can get another review and I'll merge it afterwards.

@fnagel would you mind having a look?

tests/unit/tooltip/core.js Outdated Show resolved Hide resolved
@mgol mgol requested a review from fnagel November 7, 2021 12:13
fnagel
fnagel approved these changes Nov 8, 2021
View reviewed changes
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

mgol
mgol reviewed Nov 8, 2021
View reviewed changes
tests/unit/tooltip/core.js Outdated Show resolved Hide resolved
mgol
mgol reviewed Nov 8, 2021
View reviewed changes
tests/unit/tooltip/core.js Outdated Show resolved Hide resolved
@mgol mgol changed the title Widget & Tooltip: Regression from 1.12.1 Tooltip: Don't crash on empty content Nov 8, 2021
@mgol mgol merged commit 85fba3f into jquery:main Nov 8, 2021
@mgol
Copy link
Member

mgol commented Nov 8, 2021

Landed, thanks!

@mgol mgol added this to the 1.13.1 milestone Nov 8, 2021
@mgol
Copy link
Member

mgol commented Jan 20, 2022

jQuery UI 1.13.1 including this fix has been released: https://blog.jqueryui.com/2022/01/jquery-ui-1-13-1-released/.

@sgiehl sgiehl mentioned this pull request Mar 16, 2022
Merged
11 tasks
@peterhashair peterhashair mentioned this pull request Mar 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgol mgol mgol approved these changes

@fnagel fnagel fnagel approved these changes

@caugner caugner caugner approved these changes

@melloware melloware melloware approved these changes

Assignees
No one assigned
Labels
Bug Comp: Tooltip Comp: Widget
Milestone
1.13.1
Development

Successfully merging this pull request may close these issues.

jQuery-UI 1.13.0 tooltip bug
5 participants
@josepsanzcamp @mgol @fnagel @caugner @melloware