Fixes Ticket 12749, position() on positon:fixed elements is wrong #991

Closed
wants to merge 7 commits into
from

Projects

None yet

2 participants

@fracmak
Contributor
fracmak commented Oct 16, 2012

adds a new path for position() when it detects a position: fixed element to use getBoundingClientRect() to measure the exact position of the element in the window and bypassing the dom position checks. Unit tests updated

Merrifield, Jay added some commits Oct 16, 2012
Merrifield, Jay Fixes the offset unit tests by moving support tests into a setup func…
…tion so they're available for all tests. Removing said functionality from the absolute and fixed position tests
1fe88a4
Merrifield, Jay Fixes #12749, adds a new path for position() when it detects a positi…
…on: fixed element to use getBoundingClientRect() to measure the exact position of the element in the window and bypassing the dom position checks. Unit tests updated
f52e1b6
Merrifield, Jay size optimization, remove copy of the boundingClientRect, moved the m…
…argin manipulation to the resulting return object
3d4cd1a
@mikesherov mikesherov commented on an outdated diff Oct 16, 2012
src/offset.js
@@ -111,29 +111,34 @@ jQuery.fn.extend({
return;
}
- var elem = this[0],
+ var offsetParent, offset, parentOffset = { top: 0, left: 0 },
@mikesherov
mikesherov Oct 16, 2012 Member

parentOffset belongs on the next line.

Merrifield, Jay style fix cbf3e06
@mikesherov mikesherov commented on an outdated diff Oct 16, 2012
test/unit/offset.js
@@ -1,6 +1,26 @@
if ( jQuery.fn.offset ) {
-module("offset", { teardown: moduleTeardown });
+module("offset", { setup: function(){
+ // force a scroll value on the main window
+ // this insures that the results will be wrong
+ // if the offset method is using the scroll offset
+ // of the parent window
+ var forceScroll = jQuery("<div>").css({ "width": 2000, "height": 2000 });
+ forceScroll.appendTo("body");
@mikesherov
mikesherov Oct 16, 2012 Member

append to ("#qunit-fixture") instead.

@mikesherov mikesherov commented on an outdated diff Oct 16, 2012
test/unit/offset.js
@@ -1,6 +1,26 @@
if ( jQuery.fn.offset ) {
-module("offset", { teardown: moduleTeardown });
+module("offset", { setup: function(){
+ // force a scroll value on the main window
+ // this insures that the results will be wrong
+ // if the offset method is using the scroll offset
+ // of the parent window
+ var forceScroll = jQuery("<div>").css({ "width": 2000, "height": 2000 });
+ forceScroll.appendTo("body");
+ var checkDiv = jQuery("<div>").appendTo("body")[0];
@mikesherov
mikesherov Oct 16, 2012 Member

append to ("#qunit-fixture") instead.

@mikesherov mikesherov commented on an outdated diff Oct 16, 2012
test/unit/offset.js
@@ -1,6 +1,26 @@
if ( jQuery.fn.offset ) {
-module("offset", { teardown: moduleTeardown });
+module("offset", { setup: function(){
+ // force a scroll value on the main window
+ // this insures that the results will be wrong
+ // if the offset method is using the scroll offset
+ // of the parent window
+ var forceScroll = jQuery("<div>").css({ "width": 2000, "height": 2000 });
+ forceScroll.appendTo("body");
+ var checkDiv = jQuery("<div>").appendTo("body")[0];
+
+ window.scrollTo(200, 200);
@mikesherov
mikesherov Oct 16, 2012 Member

style nits: window.scrollTo( 200, 200 );

@mikesherov mikesherov commented on an outdated diff Oct 16, 2012
test/unit/offset.js
@@ -1,6 +1,26 @@
if ( jQuery.fn.offset ) {
-module("offset", { teardown: moduleTeardown });
+module("offset", { setup: function(){
+ // force a scroll value on the main window
+ // this insures that the results will be wrong
+ // if the offset method is using the scroll offset
+ // of the parent window
+ var forceScroll = jQuery("<div>").css({ "width": 2000, "height": 2000 });
+ forceScroll.appendTo("body");
+ var checkDiv = jQuery("<div>").appendTo("body")[0];
+
+ window.scrollTo(200, 200);
+ window.supportsScroll = ( document.documentElement.scrollTop || document.body.scrollTop );
+ window.scrollTo(1, 1);
@mikesherov
mikesherov Oct 16, 2012 Member

style nits: window.scrollTo( 1, 1 );

@mikesherov mikesherov commented on an outdated diff Oct 16, 2012
test/unit/offset.js
+ // force a scroll value on the main window
+ // this insures that the results will be wrong
+ // if the offset method is using the scroll offset
+ // of the parent window
+ var forceScroll = jQuery("<div>").css({ "width": 2000, "height": 2000 });
+ forceScroll.appendTo("body");
+ var checkDiv = jQuery("<div>").appendTo("body")[0];
+
+ window.scrollTo(200, 200);
+ window.supportsScroll = ( document.documentElement.scrollTop || document.body.scrollTop );
+ window.scrollTo(1, 1);
+
+ checkDiv.style.position = "fixed";
+ checkDiv.style.top = "20px";
+ // safari subtracts parent border width here which is 5px
+ window.supportsFixedPosition = (checkDiv.offsetTop === 20 || checkDiv.offsetTop === 15);
@mikesherov
mikesherov Oct 16, 2012 Member

why not use jQuery.offset.supportsFixedPosition?

@mikesherov
mikesherov Oct 16, 2012 Member

style nits here too: window.supportsFixedPosition = ( checkDiv.offsetTop === 20 || checkDiv.offsetTop === 15 );

Merrifield, Jay more style fixes da0355e
@mikesherov mikesherov commented on an outdated diff Oct 16, 2012
test/unit/offset.js
+ // if the offset method is using the scroll offset
+ // of the parent window
+ var forceScroll = jQuery("<div>").css({ "width": 2000, "height": 2000 });
+ forceScroll.appendTo("body");
+ var checkDiv = jQuery("<div>").appendTo("body")[0];
+
+ window.scrollTo(200, 200);
+ window.supportsScroll = ( document.documentElement.scrollTop || document.body.scrollTop );
+ window.scrollTo(1, 1);
+
+ checkDiv.style.position = "fixed";
+ checkDiv.style.top = "20px";
+ // safari subtracts parent border width here which is 5px
+ window.supportsFixedPosition = (checkDiv.offsetTop === 20 || checkDiv.offsetTop === 15);
+ checkDiv.style.position = checkDiv.style.top = "";
+ jQuery(checkDiv).remove();
@mikesherov
mikesherov Oct 16, 2012 Member

jQuery( checkDiv ).remove();

@mikesherov mikesherov commented on the diff Oct 16, 2012
test/unit/offset.js
-
- // force a scroll value on the main window
- // this insures that the results will be wrong
- // if the offset method is using the scroll offset
- // of the parent window
- forceScroll = jQuery("<div>").css({ "width": 2000, "height": 2000 });
- forceScroll.appendTo("body");
-
- window.scrollTo(200, 200);
-
- if ( document.documentElement.scrollTop || document.body.scrollTop ) {
- supportsScroll = true;
- }
-
- window.scrollTo(1, 1);
+ tests;
@mikesherov
mikesherov Oct 16, 2012 Member

why declare without assign?

Merrifield, Jay more style fixes caec360
@mikesherov
Member

@fracmak, awesome stuff today. I'm going to be testing this now, and I'll let you know if any other tweaks are needed.

@fracmak
Contributor
fracmak commented Oct 17, 2012

Thanks! It was a ton of fun and a once in a lifetime experience that I enjoyed immensely.

@mikesherov mikesherov added a commit that closed this pull request Oct 17, 2012
@mikesherov Merrifield, Jay + mikesherov Fixes #12749, correctly detect position() for position:fixed elements…
…, closes gh-991
425272a
@mikesherov
Member

@fracmak Congrats on landing your first pull request! I'd love for you to continue contributing more! The only feeback I have was that I made some more edits before landing to the style. Even if you are moving lines around, if you touch a line, you should change the line to match the style guidelines.

@mescoda mescoda pushed a commit to mescoda/jquery that referenced this pull request Nov 4, 2014
@mikesherov Merrifield, Jay + mikesherov Fixes #12749, correctly detect position() for position:fixed elements…
…, closes gh-991
78c3250
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment