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.

Sign up for GitHub

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

Fix tooltip positioning with regards to viewports #14226

Closed
wants to merge 16 commits into from
Closed

Fix tooltip positioning with regards to viewports #14226

Changes from 3 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
7228c80
Fix tooltip positioning with regards to viewports
ericmartel Jul 24, 2014
5d29ec7
Fix style
ericmartel Jul 24, 2014
3214513
Fix comments from @hnrch02
ericmartel Jul 24, 2014
4d4808a
Fix extend
ericmartel Jul 24, 2014
fc873cf
Fix string testing
ericmartel Jul 24, 2014
8c69579
Merge condition and pass viewport
ericmartel Jul 31, 2014
a9bb129
Add scrolling unit test
ericmartel Aug 2, 2014
caaad18
Fix tooltip positioning with regards to viewports
ericmartel Jul 24, 2014
b906e11
Fix style
ericmartel Jul 24, 2014
c438fed
Fix comments from @hnrch02
ericmartel Jul 24, 2014
4fad947
Fix extend
ericmartel Jul 24, 2014
2a8b6f9
Fix string testing
ericmartel Jul 24, 2014
c2edabf
Merge condition and pass viewport
ericmartel Jul 31, 2014
6e32900
Add scrolling unit test
ericmartel Aug 2, 2014
bc9a3ad
Merge 3.3.1
ericmartel Nov 21, 2014
388064a
Round unit tests using offset
ericmartel Jan 5, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 29 additions & 5 deletions js/tooltip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,8 +325,8 @@
var elOffset = isBody ? { top: 0, left: 0 } : $element.offset()
var scroll = { scroll: isBody ? document.documentElement.scrollTop || document.body.scrollTop : $element.scrollTop() }
var outerDims = isSvg ? {} : {
width: isBody ? $(window).width() : $element.outerWidth(),
height: isBody ? $(window).height() : $element.outerHeight()
width: $element.outerWidth(),
height: $element.outerHeight()
}

return $.extend({}, elRect, scroll, outerDims, elOffset)
Expand All @@ -340,16 +340,40 @@

}

Tooltip.prototype.getViewportBounds = function ($viewport) {
if ($viewport.is('body')) {
Copy link
Member

Choose a reason for hiding this comment

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

you can combine these conditions
if ($viewport.is('body') && /fixed|absolute/.test(this.$element.css('position'))

// fixed and absolute elements should be tested against the window
switch (this.$element.css('position')) {
case 'absolute':
case 'fixed':
{
return this.getScreenSpaceBounds()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The curly braces here aren't necessary.

Copy link
Author

Choose a reason for hiding this comment

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

I didn't have them at first, but it made the 'test' script complain that the number of whitespaces should be 8 and not 6 (because it was going to the next curly brace)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting. Seems like a bug in JSCS. I'll look more into it and file an upstream issue.

Copy link
Author

Choose a reason for hiding this comment

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

FYI, the issue wasn't popping while the semi-colon was there

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, we had something like that before: #13512 and jscs-dev/node-jscs#357.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To fix it for now, you can remove the curly braces and add a break in the next line.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Never mind my previous comment, it's probably better (in order to be more consistent with the rest of the codebase) to switch to this instead:

if (/absolute|fixed/.test(this.$element.css('position'))) {
  return this.getScreenSpaceBounds()
}

Copy link
Author

Choose a reason for hiding this comment

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

The regular expression is indeed more elegant, I made the change! Thanks

}
}

return { top: $viewport.offset().top, left: $viewport.offset().left, width: $viewport.outerWidth(), height: $viewport.outerHeight() }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps add a local var to avoid calling .offset() twice

Copy link
Collaborator

Choose a reason for hiding this comment

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

ericmartel@7228c80#diff-f3e99e0bb007ace7a370f0492b9cb5abR355 That's how it was before, @ericmartel removed the $.extend per my suggestion, but I actually was talking about the one farther down this file.

Copy link
Author

Choose a reason for hiding this comment

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

@cvrebert @hnrch02 thanks for your feedback guys, I really didn't like calling it twice, but coming from C++ I was hoping the call could get cached / optimized away? I guess it's not the case in JS?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't seem to be cached, or if it is, then other overhead is outweighing it; http://jsperf.com/caching-of-offset-value

Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, resources posted below. I don't think its worth talking about optimization and speed on such a scale. (Talking about scale again, hopefully that doesn't work out like last time.)

}

Tooltip.prototype.getScreenSpaceBounds = function () {
return $.extend({}, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The extend here seems unnecessary, or am I missing something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was talking about this $.extend, the other one was fine, IMO.

top: $('body').scrollTop(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps use document.body here

Copy link
Author

Choose a reason for hiding this comment

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

@cvrebert I wasn't too sure about that one, but based on http://jsperf.com/jquery-body-vs-document-body-selector this seemed like a good option to use $('body'). Is there a good way to profile performance of jquery / bootstrap or in a case like this it's more a case of project standards?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No standard that I know of. It's admittedly a micro-optimization (http://jsperf.com/jquery-body-vs-document-body-selector/2 ). It's ultimately up to @fat to decide.

Copy link
Collaborator

Choose a reason for hiding this comment

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

mathiasbynens/jsperf.com#158 and http://mrale.ph/blog/2012/12/15/microbenchmarks-fairy-tale.html

I personally prefer $(document.body) because it's more explicit.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for sharing! Since I'm new to all this that's priceless when you guys share information like this!

I have a bunch of meetings in the afternoon, but I definitely wouldn't mind changing it to $(document.body) later tonight!

left: $('body').scrollLeft(),
width: $(window).width(),
height: $(window).height()
})
}

Tooltip.prototype.getViewportAdjustedDelta = function (placement, pos, actualWidth, actualHeight) {
var delta = { top: 0, left: 0 }
if (!this.$viewport) return delta

var viewportPadding = this.options.viewport && this.options.viewport.padding || 0
var viewportDimensions = this.getPosition(this.$viewport)
var viewportDimensions = this.getViewportBounds(this.$viewport)

if (/right|left/.test(placement)) {
var topEdgeOffset = pos.top - viewportPadding - viewportDimensions.scroll
var bottomEdgeOffset = pos.top + viewportPadding - viewportDimensions.scroll + actualHeight
var topEdgeOffset = pos.top - viewportPadding
var bottomEdgeOffset = pos.top + viewportPadding + actualHeight
if (topEdgeOffset < viewportDimensions.top) { // top overflow
delta.top = viewportDimensions.top - topEdgeOffset
} else if (bottomEdgeOffset > viewportDimensions.top + viewportDimensions.height) { // bottom overflow
Expand Down