Fix tooltip positioning with regards to viewports #14226

ericmartel · 2014-07-24T01:26:51Z

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.

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.

ericmartel · 2014-07-24T01:31:38Z

Hi,

This is my first contribution, hopefully it's a decent one. It fixes #14195

The Tooltip Viewport Example is still working and all unit tests pass.

I "hardcoded" some rules concerning the css position for the viewport example to work as it was before, as theoretically the "bottom" tooltip would have room to expand towards the bottom... now I restrict it in screen space like it was before.

It's my first JavaScript / web PR, so it's possible that I messed something up... feel free to give me feedback :)

If the style / direction I used to fix the problem is "ok", I think I would go further in simplifying / refactoring the getPosition function which is used for many other things than actually getting a position.

Thanks!

ericmartel · 2014-07-24T01:37:02Z

And obviously I had to play with whitespaces after running my final 'grunt test'... fixing the file.

Remove trailing whitespace

hnrch02 · 2014-07-24T07:40:32Z

js/tooltip.js

@@ -340,16 +340,40 @@

  }

+  Tooltip.prototype.getViewportBounds = function ($viewport) {
+    if ($viewport.selector == 'body') {


Use $viewport.is('body') here.

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’

ericmartel · 2014-07-24T16:28:07Z

Thank you for your feedback!

hnrch02 · 2014-07-24T16:31:03Z

js/tooltip.js

+  }
+
+  Tooltip.prototype.getScreenSpaceBounds = function () {
+    return $.extend({}, {


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

Bring back the initial extend Remove the extend that only merges 1 element with an empty one

cvrebert · 2014-07-24T16:35:49Z

js/tooltip.js

+
+  Tooltip.prototype.getScreenSpaceBounds = function () {
+    return $.extend({}, {
+      top: $('body').scrollTop(),


Perhaps use document.body here

@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?

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.

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.

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!

Move away from a switch to a regular expression

fat · 2014-07-30T17:32:34Z

js/tooltip.js

@@ -340,16 +340,36 @@

  }

+  Tooltip.prototype.getViewportBounds = function ($viewport) {
+    if ($viewport.is('body')) {


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

fat · 2014-07-30T17:36:40Z

this is looking pretty good (thanks @hnrch02 for the help) – any chance you could write some new tests @ericmartel for the issues you are fixing? thanks!

ericmartel · 2014-07-30T18:05:14Z

Thanks, will do the changes later tonight! As for the tests, I'll see to add more, but I'm not sure how to handle the scolling in the tests. I'll try to find how to when I get home, but if you have some pointers I'll be more than happy to learn from you guys!

Reduce access to the DOM and merge conditions on one line

ericmartel · 2014-07-31T12:08:15Z

Updated the code and I should be able to write new unit tests for fixed and absolute elements later this week or during the week end.

Thanks! :)

Added a unit test to validate that the computed position didn’t take scrolling into account

ericmartel · 2014-08-02T19:54:24Z

I added a new test to handle scrolling, the rest of the code being already covered by existing unit tests

ericmartel · 2014-08-02T20:05:42Z

If that's the case, I believe a lot of tests will have to be updated as a lot of tests about placement and viewport are not using callbacks at the moment.

From the results I was getting though, it SEEMED like my tooltip was already created when I was getting the offset values.

gharlan · 2014-08-04T14:40:15Z

Is this issue about this problem: http://jsbin.com/risupocu/1/ ?
I would expect that the tooltip is shown on bottom and not on top..

ericmartel · 2014-08-04T18:36:29Z

My pull request doesn't cover auto placement.

I suggest you open a new issue, once this pull request is accepted and merge, it should be easy to use the same bounds code to handle auto placement.

ericmartel · 2014-11-21T05:24:46Z

Humm I suspect my Git noobness will show in this commit... I think it should be fine but let me know if I did something wrong. (I almost only use Perforce)

cvrebert · 2014-11-21T05:31:26Z

@ericmartel Much appreciated!

@fat Ping.

cvrebert · 2014-11-21T07:05:16Z

Running Sauce cross-browser test: https://travis-ci.org/twbs/bootstrap/jobs/41687000
(Savage rightfully refused because 44735c8 is in this PR's commit list for some reason.)

cvrebert · 2014-11-21T07:10:11Z

js/tests/unit/tooltip.js

@@ -711,6 +711,42 @@ $(function () {
    $styles.remove()
  })

+  test('should not be adjust position when scrolling', function () {


grammar problem: "should not be adjust position" [sic]

ericmartel · 2014-11-21T17:04:11Z

@cvrebert thanks Chris for the feedback! I'll most definitely fix the typos. How can I run the Sauce cross-browser tests? Launching grunt was successful but I guess it only runs the unit tests?

As for the PR containing wrong commits, it's the first time I try to integrate a branch to update a PR (I'm used to perforce), I'll try to rework the whole thing this week end

Thanks

cvrebert · 2014-11-21T17:47:05Z

Using Sauce requires a Sauce account, so there's not an easy way to run the Sauce tests yourself.
However, if you're able to remove 44735c8 from this PR's changelist, our bot (@twbs-savage) will automatically run the Sauce tests for you and report the results.

ericmartel · 2014-11-21T17:48:49Z

@cvrebert Thanks, I'll read my git book and see what I did wrong :)

ericmartel · 2014-11-24T20:54:09Z

Ok, I read about how to fix this PR... can you guys advise the best way to do so? I believe my best home would be to git revert to my change before merging from master, and then redo the merge (though i'm not sure where I messed during the merge to cause this problem)?

Or simply since there's very little change... create a new branch, redo the changes and open a new PR?

just-boris · 2014-12-01T23:42:58Z

Looking forward to see it merged

cvrebert · 2015-01-05T09:29:27Z

@twbs-savage Retry

twbs-savage · 2015-01-05T09:33:06Z

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

Commit: bc9a3ad
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/45925623

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

cvrebert · 2015-01-05T09:36:42Z

@ericmartel I'll just squash the history before merging this. Could you add a commit to fix the rounding problem and address the other minor comments I made?

ericmartel · 2015-01-05T13:45:33Z

@cvrebert sorry I think I missed this comment, let me know if I can do anything else to help!

twbs-savage · 2015-01-05T13:46:40Z

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

Commit: 388064a
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/45946437

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

ericmartel · 2015-01-05T13:50:19Z

@cvrebert there seems to be problems with the tests, I doubt it has anything to do with my changelist though :/

Could not connect to Sauce Labs API: Error: read ECONNRESET

cvrebert · 2015-01-05T15:40:01Z

@twbs-savage Retry

twbs-savage · 2015-01-05T15:45:13Z

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

Commit: 388064a
Build details: https://travis-ci.org/twbs-savage/bootstrap/builds/45960000

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

ericmartel · 2015-01-05T16:08:39Z

@cvrebert thanks for the retry, is there a way to get more details than:

13972015-01-05 15:53:17:344 - info: [IOS_SYSLOG_ROW ] Jan 5 07:53:17 itako36176 assertiond[631]: assertion failed: 13F34 12B411: assertiond + 11523 [3F572A0B-7E12-378D-AFEE-EA491BAF2C36]: 0x1

is there a way to extract which assert failed?

Thank you very much, I'm sorry if I'm being a burden

cvrebert · 2015-01-05T16:26:35Z

Should be in the screencast on https://saucelabs.com/jobs/fe963a60d91d48a999a3a8e750010755

ericmartel · 2015-01-05T17:45:57Z

@cvrebert the error seems to come from affix.js, possibly from my bad merge?

cvrebert · 2015-01-05T20:20:23Z

Ah, that might be because this PR is against an older version of master. I'll test a rebased version.

cvrebert · 2015-01-05T20:39:07Z

Superseded by #15494, which is just a rebased version of this PR.

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. Fixes #14195 Closes #14226

Fix style

5d29ec7

Remove trailing whitespace

hnrch02 reviewed Jul 24, 2014
View reviewed changes

Fix comments from @hnrch02

3214513

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’

hnrch02 reviewed Jul 24, 2014
View reviewed changes

Fix extend

4d4808a

Bring back the initial extend Remove the extend that only merges 1 element with an empty one

cvrebert reviewed Jul 24, 2014
View reviewed changes

cvrebert added the js label Jul 24, 2014

Fix string testing

fc873cf

Move away from a switch to a regular expression

hnrch02 mentioned this pull request Jul 25, 2014

Popover placed incorrectly #14247

Closed

fat reviewed Jul 30, 2014
View reviewed changes

cvrebert added this to the v3.2.1 milestone Jul 30, 2014

Merge condition and pass viewport

8c69579

Reduce access to the DOM and merge conditions on one line

Add scrolling unit test

a9bb129

Added a unit test to validate that the computed position didn’t take scrolling into account

cvrebert reviewed Nov 21, 2014
View reviewed changes

Round unit tests using offset

388064a

cvrebert mentioned this pull request Jan 5, 2015

Fix tooltip positioning with regards to viewports #15494

Closed

cvrebert removed this from the v3.3.2 milestone Jan 5, 2015

cvrebert closed this Jan 5, 2015

Fix tooltip positioning with regards to viewports #14226

Fix tooltip positioning with regards to viewports #14226

Conversation

ericmartel commented Jul 24, 2014

ericmartel commented Jul 24, 2014

ericmartel commented Jul 24, 2014

Choose a reason for hiding this comment

ericmartel commented Jul 24, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fat commented Jul 30, 2014

ericmartel commented Jul 30, 2014

ericmartel commented Jul 31, 2014

ericmartel commented Aug 2, 2014

ericmartel commented Aug 2, 2014

gharlan commented Aug 4, 2014

ericmartel commented Aug 4, 2014

ericmartel commented Nov 21, 2014

cvrebert commented Nov 21, 2014

cvrebert commented Nov 21, 2014

Choose a reason for hiding this comment

ericmartel commented Nov 21, 2014

cvrebert commented Nov 21, 2014

ericmartel commented Nov 21, 2014

ericmartel commented Nov 24, 2014

just-boris commented Dec 1, 2014

cvrebert commented Jan 5, 2015

twbs-savage commented Jan 5, 2015

cvrebert commented Jan 5, 2015

ericmartel commented Jan 5, 2015

twbs-savage commented Jan 5, 2015

ericmartel commented Jan 5, 2015

cvrebert commented Jan 5, 2015

twbs-savage commented Jan 5, 2015

ericmartel commented Jan 5, 2015

cvrebert commented Jan 5, 2015

ericmartel commented Jan 5, 2015

cvrebert commented Jan 5, 2015

cvrebert commented Jan 5, 2015