From 7228c8071369b405d9426f66cb47733fcd8981b4 Mon Sep 17 00:00:00 2001 From: Eric Martel Date: Wed, 23 Jul 2014 21:23:39 -0400 Subject: [PATCH 01/15] Fix tooltip positioning with regards to viewports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The getPosition function was simplified with a new function, getViewportBounds, was added to specifically tackle the cases where we’re looking for the viewport bounds. ‘absolute’ and ‘fixed’ tooltips are now matched against the screen where other ones are made to fit inside the viewport element. --- js/tooltip.js | 36 ++++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/js/tooltip.js b/js/tooltip.js index 0c3a79d0b975..1e6698f4c5ce 100644 --- a/js/tooltip.js +++ b/js/tooltip.js @@ -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) @@ -340,16 +340,40 @@ } + Tooltip.prototype.getViewportBounds = function ($viewport) { + if ($viewport.selector == 'body') { + var elementPositionAttribute = $(this.$element).css('position') + + // fixed and absolute elements should be tested against the window + switch (elementPositionAttribute) { + case 'absolute': + case 'fixed': + return this.getScreenSpaceBounds(); + } + } + + return $.extend({}, $viewport.offset(), { width: $viewport.outerWidth(), height: $viewport.outerHeight() }) + } + + Tooltip.prototype.getScreenSpaceBounds = function () { + return $.extend({}, { + top: $('body').scrollTop(), + 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 @@ -367,7 +391,7 @@ return delta } - + Tooltip.prototype.getTitle = function () { var title var $e = this.$element From 5d29ec70b532d4e66bac1443825d897967a038c7 Mon Sep 17 00:00:00 2001 From: Eric Martel Date: Wed, 23 Jul 2014 21:38:04 -0400 Subject: [PATCH 02/15] Fix style Remove trailing whitespace --- js/tooltip.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/tooltip.js b/js/tooltip.js index 1e6698f4c5ce..655eff0efe61 100644 --- a/js/tooltip.js +++ b/js/tooltip.js @@ -391,7 +391,7 @@ return delta } - + Tooltip.prototype.getTitle = function () { var title var $e = this.$element From 32145138588f724ac300eb228ab0a74a404cd309 Mon Sep 17 00:00:00 2001 From: Eric Martel Date: Thu, 24 Jul 2014 12:25:39 -0400 Subject: [PATCH 03/15] Fix comments from @hnrch02 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove semi colon Remove temporary variable to hold CSS position Remove extend and explicitly construct top and left elements Use “is” to know if the viewport is a ‘body’ --- js/tooltip.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/js/tooltip.js b/js/tooltip.js index 655eff0efe61..f75d7bb24245 100644 --- a/js/tooltip.js +++ b/js/tooltip.js @@ -341,18 +341,18 @@ } Tooltip.prototype.getViewportBounds = function ($viewport) { - if ($viewport.selector == 'body') { - var elementPositionAttribute = $(this.$element).css('position') - + if ($viewport.is('body')) { // fixed and absolute elements should be tested against the window - switch (elementPositionAttribute) { + switch (this.$element.css('position')) { case 'absolute': case 'fixed': - return this.getScreenSpaceBounds(); + { + return this.getScreenSpaceBounds() + } } } - return $.extend({}, $viewport.offset(), { width: $viewport.outerWidth(), height: $viewport.outerHeight() }) + return { top: $viewport.offset().top, left: $viewport.offset().left, width: $viewport.outerWidth(), height: $viewport.outerHeight() } } Tooltip.prototype.getScreenSpaceBounds = function () { From 4d4808a0a66083d522b3749541a11f3d92865dd1 Mon Sep 17 00:00:00 2001 From: Eric Martel Date: Thu, 24 Jul 2014 12:35:00 -0400 Subject: [PATCH 04/15] Fix extend Bring back the initial extend Remove the extend that only merges 1 element with an empty one --- js/tooltip.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/js/tooltip.js b/js/tooltip.js index f75d7bb24245..7b7c43f53c26 100644 --- a/js/tooltip.js +++ b/js/tooltip.js @@ -352,16 +352,16 @@ } } - return { top: $viewport.offset().top, left: $viewport.offset().left, width: $viewport.outerWidth(), height: $viewport.outerHeight() } + return $.extend({}, $viewport.offset(), { width: $viewport.outerWidth(), height: $viewport.outerHeight() }) } Tooltip.prototype.getScreenSpaceBounds = function () { - return $.extend({}, { + return { top: $('body').scrollTop(), left: $('body').scrollLeft(), width: $(window).width(), height: $(window).height() - }) + } } Tooltip.prototype.getViewportAdjustedDelta = function (placement, pos, actualWidth, actualHeight) { From fc873cf7fc9196ab64020ace144c9ca6230f1d24 Mon Sep 17 00:00:00 2001 From: Eric Martel Date: Thu, 24 Jul 2014 18:10:03 -0400 Subject: [PATCH 05/15] Fix string testing Move away from a switch to a regular expression --- js/tooltip.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/js/tooltip.js b/js/tooltip.js index 7b7c43f53c26..e24549edc558 100644 --- a/js/tooltip.js +++ b/js/tooltip.js @@ -343,12 +343,8 @@ Tooltip.prototype.getViewportBounds = function ($viewport) { if ($viewport.is('body')) { // fixed and absolute elements should be tested against the window - switch (this.$element.css('position')) { - case 'absolute': - case 'fixed': - { - return this.getScreenSpaceBounds() - } + if ((/fixed|absolute/).test(this.$element.css('position'))) { + return this.getScreenSpaceBounds() } } From 8c695791e7da65f2c9dfb075ff68dedbed3e6322 Mon Sep 17 00:00:00 2001 From: Eric Martel Date: Thu, 31 Jul 2014 08:04:28 -0400 Subject: [PATCH 06/15] Merge condition and pass viewport Reduce access to the DOM and merge conditions on one line --- js/tooltip.js | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/js/tooltip.js b/js/tooltip.js index e24549edc558..ec9bb7bf76d7 100644 --- a/js/tooltip.js +++ b/js/tooltip.js @@ -341,20 +341,18 @@ } Tooltip.prototype.getViewportBounds = function ($viewport) { - if ($viewport.is('body')) { + if ($viewport.is('body') && (/fixed|absolute/).test(this.$element.css('position'))) { // fixed and absolute elements should be tested against the window - if ((/fixed|absolute/).test(this.$element.css('position'))) { - return this.getScreenSpaceBounds() - } + return this.getScreenSpaceBounds($viewport) } return $.extend({}, $viewport.offset(), { width: $viewport.outerWidth(), height: $viewport.outerHeight() }) } - Tooltip.prototype.getScreenSpaceBounds = function () { + Tooltip.prototype.getScreenSpaceBounds = function ($viewport) { return { - top: $('body').scrollTop(), - left: $('body').scrollLeft(), + top: $viewport.scrollTop(), + left: $viewport.scrollLeft(), width: $(window).width(), height: $(window).height() } From a9bb1297eb988ec178cba915683b98e2eb10a596 Mon Sep 17 00:00:00 2001 From: Eric Martel Date: Sat, 2 Aug 2014 15:52:55 -0400 Subject: [PATCH 07/15] Add scrolling unit test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Added a unit test to validate that the computed position didn’t take scrolling into account --- js/tests/unit/tooltip.js | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/js/tests/unit/tooltip.js b/js/tests/unit/tooltip.js index fb1940b2d98d..32332bd00406 100644 --- a/js/tests/unit/tooltip.js +++ b/js/tests/unit/tooltip.js @@ -522,6 +522,42 @@ $(function () { $styles.remove() }) + test('should not be adjust position when scrolling', function () { + var styles = '' + var $styles = $(styles).appendTo('head') + + var $container = $('
').appendTo(document.body) + var $target = $('') + .appendTo($container) + .bootstrapTooltip({ + placement: 'right', + viewport: 'body' + }) + $('
').appendTo($container) + + $target.bootstrapTooltip('show') + var $tooltip = $container.find('.tooltip') + var $initialTop = $tooltip.offset().top + + $target.bootstrapTooltip('hide') + + window.scrollTo(0, 2000) + + $target.bootstrapTooltip('show') + $tooltip = $container.find('.tooltip') + + equal($tooltip.offset().top, $initialTop, 'position is the same after scrolling') + equal($(document).scrollTop(), 2000, 'document scrolled') + + $target.bootstrapTooltip('hide') + + $container.remove() + $styles.remove() + window.scrollTo(0, 0) + }) + test('should not error when trying to show an auto-placed tooltip that has been removed from the dom', function () { var passed = true var $tooltip = $('') From caaad18c1c31ca14b4308e44291c83ddb4616c45 Mon Sep 17 00:00:00 2001 From: Eric Martel Date: Wed, 23 Jul 2014 21:23:39 -0400 Subject: [PATCH 08/15] Fix tooltip positioning with regards to viewports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The getPosition function was simplified with a new function, getViewportBounds, was added to specifically tackle the cases where we’re looking for the viewport bounds. ‘absolute’ and ‘fixed’ tooltips are now matched against the screen where other ones are made to fit inside the viewport element. --- js/tooltip.js | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/js/tooltip.js b/js/tooltip.js index cda147d9c701..c1922a8da7b6 100644 --- a/js/tooltip.js +++ b/js/tooltip.js @@ -340,7 +340,7 @@ } var elOffset = isBody ? { top: 0, left: 0 } : $element.offset() var scroll = { scroll: isBody ? document.documentElement.scrollTop || document.body.scrollTop : $element.scrollTop() } - var outerDims = isBody ? { width: $(window).width(), height: $(window).height() } : null + var outerDims = isBody ? { width: $element.outerWidth(), height: $element.outerHeight() } : null return $.extend({}, elRect, scroll, outerDims, elOffset) } @@ -353,16 +353,40 @@ } + Tooltip.prototype.getViewportBounds = function ($viewport) { + if ($viewport.selector == 'body') { + var elementPositionAttribute = $(this.$element).css('position') + + // fixed and absolute elements should be tested against the window + switch (elementPositionAttribute) { + case 'absolute': + case 'fixed': + return this.getScreenSpaceBounds(); + } + } + + return $.extend({}, $viewport.offset(), { width: $viewport.outerWidth(), height: $viewport.outerHeight() }) + } + + Tooltip.prototype.getScreenSpaceBounds = function () { + return $.extend({}, { + top: $('body').scrollTop(), + 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 @@ -380,7 +404,7 @@ return delta } - + Tooltip.prototype.getTitle = function () { var title var $e = this.$element From b906e11aea0f600222f04925c3c488e6a7c54cd5 Mon Sep 17 00:00:00 2001 From: Eric Martel Date: Wed, 23 Jul 2014 21:38:04 -0400 Subject: [PATCH 09/15] Fix style Remove trailing whitespace --- js/tooltip.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/tooltip.js b/js/tooltip.js index c1922a8da7b6..3a814eece102 100644 --- a/js/tooltip.js +++ b/js/tooltip.js @@ -404,7 +404,7 @@ return delta } - + Tooltip.prototype.getTitle = function () { var title var $e = this.$element From c438fedafea10fa0ffa9f06b96856ea13d04a3a9 Mon Sep 17 00:00:00 2001 From: Eric Martel Date: Thu, 24 Jul 2014 12:25:39 -0400 Subject: [PATCH 10/15] Fix comments from @hnrch02 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove semi colon Remove temporary variable to hold CSS position Remove extend and explicitly construct top and left elements Use “is” to know if the viewport is a ‘body’ --- js/tooltip.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/js/tooltip.js b/js/tooltip.js index 3a814eece102..3c0744b053b5 100644 --- a/js/tooltip.js +++ b/js/tooltip.js @@ -354,18 +354,18 @@ } Tooltip.prototype.getViewportBounds = function ($viewport) { - if ($viewport.selector == 'body') { - var elementPositionAttribute = $(this.$element).css('position') - + if ($viewport.is('body')) { // fixed and absolute elements should be tested against the window - switch (elementPositionAttribute) { + switch (this.$element.css('position')) { case 'absolute': case 'fixed': - return this.getScreenSpaceBounds(); + { + return this.getScreenSpaceBounds() + } } } - return $.extend({}, $viewport.offset(), { width: $viewport.outerWidth(), height: $viewport.outerHeight() }) + return { top: $viewport.offset().top, left: $viewport.offset().left, width: $viewport.outerWidth(), height: $viewport.outerHeight() } } Tooltip.prototype.getScreenSpaceBounds = function () { From 4fad947a73870555b1f711acc70ca0eb8a0a6df1 Mon Sep 17 00:00:00 2001 From: Eric Martel Date: Thu, 24 Jul 2014 12:35:00 -0400 Subject: [PATCH 11/15] Fix extend Bring back the initial extend Remove the extend that only merges 1 element with an empty one --- js/tooltip.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/js/tooltip.js b/js/tooltip.js index 3c0744b053b5..7db32a4e323e 100644 --- a/js/tooltip.js +++ b/js/tooltip.js @@ -365,16 +365,16 @@ } } - return { top: $viewport.offset().top, left: $viewport.offset().left, width: $viewport.outerWidth(), height: $viewport.outerHeight() } + return $.extend({}, $viewport.offset(), { width: $viewport.outerWidth(), height: $viewport.outerHeight() }) } Tooltip.prototype.getScreenSpaceBounds = function () { - return $.extend({}, { + return { top: $('body').scrollTop(), left: $('body').scrollLeft(), width: $(window).width(), height: $(window).height() - }) + } } Tooltip.prototype.getViewportAdjustedDelta = function (placement, pos, actualWidth, actualHeight) { From 2a8b6f98fa91a99b0c39cd5ef81a45a73d980c84 Mon Sep 17 00:00:00 2001 From: Eric Martel Date: Thu, 24 Jul 2014 18:10:03 -0400 Subject: [PATCH 12/15] Fix string testing Move away from a switch to a regular expression --- js/tooltip.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/js/tooltip.js b/js/tooltip.js index 7db32a4e323e..f2776c07ea99 100644 --- a/js/tooltip.js +++ b/js/tooltip.js @@ -356,12 +356,8 @@ Tooltip.prototype.getViewportBounds = function ($viewport) { if ($viewport.is('body')) { // fixed and absolute elements should be tested against the window - switch (this.$element.css('position')) { - case 'absolute': - case 'fixed': - { - return this.getScreenSpaceBounds() - } + if ((/fixed|absolute/).test(this.$element.css('position'))) { + return this.getScreenSpaceBounds() } } From c2edabf82393b7d68f36fe7933d20a95b2bf9fa5 Mon Sep 17 00:00:00 2001 From: Eric Martel Date: Thu, 31 Jul 2014 08:04:28 -0400 Subject: [PATCH 13/15] Merge condition and pass viewport Reduce access to the DOM and merge conditions on one line --- js/tooltip.js | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/js/tooltip.js b/js/tooltip.js index f2776c07ea99..b6a5f700347b 100644 --- a/js/tooltip.js +++ b/js/tooltip.js @@ -354,20 +354,18 @@ } Tooltip.prototype.getViewportBounds = function ($viewport) { - if ($viewport.is('body')) { + if ($viewport.is('body') && (/fixed|absolute/).test(this.$element.css('position'))) { // fixed and absolute elements should be tested against the window - if ((/fixed|absolute/).test(this.$element.css('position'))) { - return this.getScreenSpaceBounds() - } + return this.getScreenSpaceBounds($viewport) } return $.extend({}, $viewport.offset(), { width: $viewport.outerWidth(), height: $viewport.outerHeight() }) } - Tooltip.prototype.getScreenSpaceBounds = function () { + Tooltip.prototype.getScreenSpaceBounds = function ($viewport) { return { - top: $('body').scrollTop(), - left: $('body').scrollLeft(), + top: $viewport.scrollTop(), + left: $viewport.scrollLeft(), width: $(window).width(), height: $(window).height() } From 6e329001686d18815fcbb7a6b1b43cc8cd1013c8 Mon Sep 17 00:00:00 2001 From: Eric Martel Date: Sat, 2 Aug 2014 15:52:55 -0400 Subject: [PATCH 14/15] Add scrolling unit test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Added a unit test to validate that the computed position didn’t take scrolling into account --- js/tests/unit/tooltip.js | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/js/tests/unit/tooltip.js b/js/tests/unit/tooltip.js index 4fbd91dc3705..417497644046 100644 --- a/js/tests/unit/tooltip.js +++ b/js/tests/unit/tooltip.js @@ -711,6 +711,42 @@ $(function () { $styles.remove() }) + test('should not be adjust position when scrolling', function () { + var styles = '' + var $styles = $(styles).appendTo('head') + + var $container = $('
').appendTo(document.body) + var $target = $('') + .appendTo($container) + .bootstrapTooltip({ + placement: 'right', + viewport: 'body' + }) + $('
').appendTo($container) + + $target.bootstrapTooltip('show') + var $tooltip = $container.find('.tooltip') + var $initialTop = $tooltip.offset().top + + $target.bootstrapTooltip('hide') + + window.scrollTo(0, 2000) + + $target.bootstrapTooltip('show') + $tooltip = $container.find('.tooltip') + + equal($tooltip.offset().top, $initialTop, 'position is the same after scrolling') + equal($(document).scrollTop(), 2000, 'document scrolled') + + $target.bootstrapTooltip('hide') + + $container.remove() + $styles.remove() + window.scrollTo(0, 0) + }) + test('should not error when trying to show an auto-placed tooltip that has been removed from the dom', function () { var passed = true var $tooltip = $('') From 388064a84ed771e47959cfc77b2f47472ab4c56f Mon Sep 17 00:00:00 2001 From: Eric Martel Date: Mon, 5 Jan 2015 08:44:05 -0500 Subject: [PATCH 15/15] Round unit tests using offset --- js/tests/unit/tooltip.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/js/tests/unit/tooltip.js b/js/tests/unit/tooltip.js index 417497644046..01b167ba6c26 100644 --- a/js/tests/unit/tooltip.js +++ b/js/tests/unit/tooltip.js @@ -728,7 +728,7 @@ $(function () { $target.bootstrapTooltip('show') var $tooltip = $container.find('.tooltip') - var $initialTop = $tooltip.offset().top + var $initialTop = Math.round($tooltip.offset().top) $target.bootstrapTooltip('hide') @@ -737,7 +737,7 @@ $(function () { $target.bootstrapTooltip('show') $tooltip = $container.find('.tooltip') - equal($tooltip.offset().top, $initialTop, 'position is the same after scrolling') + equal(Math.round($tooltip.offset().top), $initialTop, 'position is the same after scrolling') equal($(document).scrollTop(), 2000, 'document scrolled') $target.bootstrapTooltip('hide')