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

Avoid using $.offset() on SVGs since it gives incorrect results in jQuery 3 #20313

Merged
merged 1 commit into from
Jul 21, 2016

Conversation

cvrebert
Copy link
Collaborator

@cvrebert cvrebert added this to the v3.3.7 milestone Jul 17, 2016
@twbs-savage
Copy link

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 6b1e2bc
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/145303749
Docs preview: http://preview.twbsapps.com/c/${commitSha.sha}

(Please note that this is a fully automated comment.)

@cvrebert
Copy link
Collaborator Author

The Savage failure is Safari-specific, seemingly unrelated, and probably transient: https://saucelabs.com/jobs/c8bda0f8f4db468491e30a985319aabc
Will retry shortly.

@twbs-savage
Copy link

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: c4d1cdd
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/145305276
Docs preview: http://preview.twbsapps.com/c/${commitSha.sha}

(Please note that this is a fully automated comment.)

@hnrch02
Copy link
Collaborator

hnrch02 commented Jul 17, 2016

So for SVGs the position is now determined based on getBoundingClientRect alone, right? Does that mean tooltips are now be properly positioned when used in SVGs? I remember reading an issue about that problem some time ago.

@XhmikosR
Copy link
Member

LGTM as far as I can tell.

@cvrebert
Copy link
Collaborator Author

So for SVGs the position is now determined based on getBoundingClientRect alone, right?

Correct.

Does that mean tooltips are now be properly positioned when used in SVGs?

That's already the case and should remain the case. We have a unit test specifically for SVG handling:

QUnit.test('should correctly position tooltips on SVG elements', function (assert) {

@cvrebert
Copy link
Collaborator Author

For extra assurance, I upgraded to jQuery 3 in an experimental commit (b9ac9e5) and the Sauce tests still passed on all browsers:
https://travis-ci.org/twbs/bootstrap/jobs/146142422

@hnrch02
Copy link
Collaborator

hnrch02 commented Jul 21, 2016

LGTM then.

@cvrebert cvrebert merged commit c2404d3 into master Jul 21, 2016
@cvrebert cvrebert deleted the jq3-svg-fail branch July 21, 2016 00:22
@mdo mdo mentioned this pull request Jul 25, 2016
krissihall pushed a commit to krissihall/bootstrap that referenced this pull request Sep 17, 2016
* tag 'v3.3.7' of https://github.com/twbs/bootstrap: (195 commits)
  Regenerate Customizer JavaScript
  _config.yml: Update SRI hashes for v3.3.7
  grunt
  grunt dist
  Update remaining version numbers in source files
  Fix grunt/change-version.js to update NuGet-related files too
  Fix grunt/change-version.js to update .less files too
  NuGet: Adjust version range to allow jQuery 3
  Update NuGet version numbers is preparation for v3.3.7 release
  Update version numbers in preparation for v3.3.7 release (twbs#20357)
  Discourage usage of <a>s for disabled pagination items
  Revert "Add pointer-events:none to disabled pager/pagination"
  Update iOS temporal input types support comment (twbs#20343)
  iOS 9.3 is still affected by https://webkit.org/b/153856
  Docs: Update Safari versions affected by twbs#9774
  Update jQuery version check and jQuery dependency version range (twbs#20338)
  Avoid using $.offset() on SVGs since it gives incorrect results in jQuery 3; fixes twbs#20280 (twbs#20313)
  Port twbs#20315 to v3
  Clarify valid values of Carousel's `pause` option
  Docs CSS: Removed border from GitHub buttons (twbs#19606)
  ...
chiraggmodi pushed a commit to chiraggmodi/bootstrap that referenced this pull request Apr 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants