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

Remove webkitConvertPointFromNodeToPage #796

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
6 participants
@markelog
Copy link
Member

commented May 25, 2012

Yeah, i know, i know, you guys voted on it. But hear me out for a minute, from ticket description –

"For older versions of Safari (or older mobile safari) it seems simpler to calculate offset using window.webkitConvertPointFromNodeToPage()."

In what browsers, exactly, we have webkitConvertPointFromNodeToPage and do not have getBoundingClientRect? For desktop browsers it's simple – for Safari, support for webkitConvertPointFromNodeToPage begins from Safari 4, just like for getBoundingClientRect, Chrome always had it.

Mobile browsers? There is no way to reliable know if all mobile webkit-based browsers lists here is have webkitConvertPointFromNodeToPage and do not have getBoundingClientRect. Only way to know that, is to check lowest versions of them all –

  • Apple iOS 3.2
  • Android 2.1
  • Blackberry 6
  • Blackberry Playbook 1
  • Palm WebOS 1.4
  • Meego 1.2
  • Dolphin 3
  • Kindle Fire

And i did – they all have getBoundingClientRect, which means webkitConvertPointFromNodeToPage code path will never be chosen, which means webkitConvertPointFromNodeToPage is dead code and should be removed.

cc @mikesherov

@rwaldron

This comment has been minimized.

Copy link
Member

commented May 25, 2012

@markelog

This comment has been minimized.

Copy link
Member Author

commented May 25, 2012

@rwldrn, hey, I already did that :)

@rwaldron

This comment has been minimized.

Copy link
Member

commented May 25, 2012

Sorry, I missed that. (it's a habit now, for all things box related)

@mikesherov

This comment has been minimized.

Copy link
Member

commented May 25, 2012

Yeah, I'm with you on this one @Orkel. I kept it in simply because I couldn't really get a great answer from the jQm team on whether or not we needed to support this. I know that the original iPhone is affected by this, but that's about it :-.

I say good riddance. Just let someone complain!

@rwaldron

This comment has been minimized.

Copy link
Member

commented May 25, 2012

That's a relief...

@jaubourg

This comment has been minimized.

Copy link
Member

commented May 25, 2012

\o/

@mikesherov

This comment has been minimized.

Copy link
Member

commented May 25, 2012

@Orkel, epic amounts of research there!

@markelog

This comment has been minimized.

Copy link
Member Author

commented May 25, 2012

@mikesherov thanks, it did take a lot of patience, but, please, dont trust me, dont land this, until somebody else check this at least some of them browsers i listed above

@jaubourg

This comment has been minimized.

Copy link
Member

commented May 25, 2012

@Orkel did you collect devices or did you use emulators ? or both ?

@mikesherov

This comment has been minimized.

Copy link
Member

commented May 25, 2012

From my research, all I could find was the original iPhone that this impacted. One other note, you should remove the object from the grunt JSHint expected globals array.

@mikesherov

This comment has been minimized.

Copy link
Member

commented May 25, 2012

And by object, I mean WebkitPoint.

@markelog

This comment has been minimized.

Copy link
Member Author

commented May 25, 2012

@jaubourg both, mostly emulators, except for android and old iphone, do you have a reason not to trust those emulators?
@mikesherov done, thanks!

@jaubourg

This comment has been minimized.

Copy link
Member

commented May 26, 2012

@Orkel I do trust emulators, I was just curious about the means ;)

@dmethvin

This comment has been minimized.

Copy link
Member

commented May 27, 2012

Sizes - compared to master 252683 (-397) dist/jquery.js 92521 (-221) dist/jquery.min.js 33162 (-77) dist/jquery.min.js.gz

@dmethvin dmethvin closed this in d0763a3 May 27, 2012

@dmethvin

This comment has been minimized.

Copy link
Member

commented May 27, 2012

Excellent detective work @Orkel, thanks!

@dmethvin

This comment has been minimized.

Copy link
Member

commented May 27, 2012

Looks like we have some test failures from this? http://swarm.jquery.org/job/163

Related discussion on http://bugs.jquery.com/ticket/4996 and cf672a2, I still think asking for the offset of a disconnected node is a bad idea. 💣

@rwaldron

This comment has been minimized.

Copy link
Member

commented May 27, 2012

I think expecting anything about display/style from a detached node is a bad idea...

@mikesherov

This comment has been minimized.

Copy link
Member

commented May 27, 2012

Webkit doesn't not return style information on disconnected node unless they are inline styles. Any test for style info on disconnected nodes is a bad test IMHO.

@mikesherov

This comment has been minimized.

Copy link
Member

commented May 27, 2012

Oh, yeah, just saw the failures. We should still be normalizing to 0 for disconnected nodes. It's the only sane response besides an exception.

markelog added a commit to markelog/jquery that referenced this pull request Jun 29, 2012

markelog added a commit to markelog/jquery that referenced this pull request Jun 29, 2012

@Yaffle

This comment has been minimized.

Copy link

commented May 17, 2013

webkitConvertPointFromNodeToPage works even if css transformation was applied unlike getBoundingClientRect

@lock lock bot locked as resolved and limited conversation to collaborators Jan 22, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.