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

tooltip.js: getBoundingClientRect method existence check is unnecessary? #14093

Closed
cvrebert opened this issue Jul 8, 2014 · 17 comments · Fixed by #14492
Closed

tooltip.js: getBoundingClientRect method existence check is unnecessary? #14093

cvrebert opened this issue Jul 8, 2014 · 17 comments · Fixed by #14492
Labels
Milestone

Comments

@cvrebert
Copy link
Collaborator

cvrebert commented Jul 8, 2014

Element.getBoundingClientRect() seems extremely well-supported:
https://developer.mozilla.org/en-US/docs/Web/API/Element.getBoundingClientRect#Browser_compatibility
So then, is there a reason that there's a check in tooltip.js for whether this method exists?:

var elRect = typeof el.getBoundingClientRect == 'function' ? el.getBoundingClientRect() : null

To: @fat
X-Ref: #14090

@cvrebert cvrebert added this to the v3.2.1 milestone Jul 8, 2014
@cvrebert cvrebert added the js label Jul 8, 2014
@fat
Copy link
Member

fat commented Jul 12, 2014

i feel like that was there for svg elements or something weird

@cvrebert
Copy link
Collaborator Author

cvrebert commented Aug 4, 2014

Might've been due to iOS v3 not supporting it?: thebird/Swipe#71

cvrebert added a commit that referenced this issue Aug 4, 2014
@cvrebert
Copy link
Collaborator Author

cvrebert commented Aug 4, 2014

Running Sauce cross-browser test with the existence check removed:
https://travis-ci.org/twbs/bootstrap/jobs/32660379

@hnrch02
Copy link
Collaborator

hnrch02 commented Aug 4, 2014

@cvrebert Sauce timed out, mind rerunning?

@fat
Copy link
Member

fat commented Aug 15, 2014

ah yeah that might have been it. feel free to remove if tests pass

cvrebert added a commit that referenced this issue Aug 15, 2014
@cvrebert
Copy link
Collaborator Author

Sauce tests still pass with the check removed (see above).

@gsmet
Copy link
Contributor

gsmet commented Aug 19, 2014

Hi,

I am the one who introduced this test: see #6737 : there are unrelated discussions in the PR but the point is it won't work with IE8 if you don't have this test.

IE8 does have getBoundingClientRect but it's broken, it's just an empty placeholder.

It would be better to revert this IMHO.

@gsmet
Copy link
Contributor

gsmet commented Aug 19, 2014

...and add the function test I added in openwide-java@b7764d8 .

el.getBoundingClientRect is defined but is not a function in IE8.

@cvrebert
Copy link
Collaborator Author

So what we really need to do is backtrace to see when the is-a-function test was changed to a simple truthiness test, and why.

@cvrebert cvrebert reopened this Aug 19, 2014
@cvrebert
Copy link
Collaborator Author

Hmm, looks like it was part of @hnrch02's changes in #14090.

@cvrebert
Copy link
Collaborator Author

Whatever the root IE8 quirk here is, it's in dire need of being documented on the MDN page (https://developer.mozilla.org/en-US/docs/Web/API/Element.getBoundingClientRect).

@gsmet
Copy link
Contributor

gsmet commented Aug 19, 2014

I can assure you the code didn't work in 2.3.0 and we fixed the behavior by adding the test. Calling getBoundingClientRect returned an empty object which didn't contain anything useful.

As you can see in zeroclipboard/zeroclipboard@be45735 (it's referenced by @hnrch02 in the PR) , they removed the test BUT they added a workaround as the "method" might return something empty. This was our case and that's why we fixed it like that.

Considering the way tooltip was coded at the time, adding the function test was the right thing to do as it triggers the workaround.

@hnrch02
Copy link
Collaborator

hnrch02 commented Aug 19, 2014

I mentioned why I changed it to a simple truthiness check here: #14090 (comment). It's documented on the MDN page for the typeof operator: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/typeof#IE_host_objects_are_objects_not_functions

@hnrch02
Copy link
Collaborator

hnrch02 commented Aug 19, 2014

Also, calling getBoundingClientRect doesn't return an empty object for me:

@cvrebert
Copy link
Collaborator Author

@cvrebert
Copy link
Collaborator Author

Okay, I propose we fix this by doing the same thing as ZeroClipboard: #14492

@fat
Copy link
Member

fat commented Sep 7, 2014

sgtm

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 a pull request may close this issue.

4 participants