Navigation Menu

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

Jump to bottom

Offset: Operate "relative to document" correctly #3096

Closed
wants to merge 6 commits into from
Closed

Offset: Operate "relative to document" correctly #3096

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8fba235
Offset: Operate "relative to document" correctly
anseki May 1, 2016
8dd15ec
Offset: Fix indention in test code
anseki May 1, 2016
3d72e54
Offset: Add test cases
anseki May 2, 2016
f40e903
Offset: Add comment in test code
anseki May 2, 2016
fa231be
Offset: rewrite the test
anseki May 5, 2016
d56c818
Offset: replace test code
anseki May 6, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 4 additions & 6 deletions src/offset.js
View file
Open in desktop
Expand Up @@ -86,7 +86,7 @@ jQuery.fn.extend( {
} );
}

var docElem, win, rect, doc,
var win, rect,
elem = this[ 0 ];

if ( !elem ) {
Expand All @@ -104,13 +104,11 @@ jQuery.fn.extend( {

// Make sure element is not hidden (display: none)
if ( rect.width || rect.height ) {
doc = elem.ownerDocument;
win = getWindow( doc );
docElem = doc.documentElement;
win = getWindow( elem.ownerDocument );

return {
top: rect.top + win.pageYOffset - docElem.clientTop,
left: rect.left + win.pageXOffset - docElem.clientLeft
top: rect.top + win.pageYOffset,
left: rect.left + win.pageXOffset
};
}

Expand Down
26 changes: 26 additions & 0 deletions test/data/offset/rel-doc.html
View file
Open in desktop
@@ -0,0 +1,26 @@
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"
"http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html>
<head>
<meta http-equiv="Content-type" content="text/html; charset=utf-8">
<title>body</title>
<style type="text/css" media="screen">
html, body { border: solid red; }
html { background-color: green; }
body { background-color: yellow; }
#marker1 { background-color: blue; }
#marker2 { background-color: orange; position: absolute; }
</style>
<script src="../../jquery.js"></script>
<script src="../iframeTest.js"></script>
<script type="text/javascript" charset="utf-8">
jQuery(function() {
startIframeTest();
});
</script>
</head>
<body>
<div id="marker1">marker1</div>
<div id="marker2">marker2</div>
</body>
</html>
138 changes: 138 additions & 0 deletions test/unit/offset.js
View file
Open in desktop
Expand Up @@ -568,4 +568,142 @@ QUnit.test( "iframe scrollTop/Left (see gh-1945)", function( assert ) {
}
} );

(function() {
var POSITION_VALUES = [ "static", "relative", "absolute", "fixed" ];

supportjQuery.each( POSITION_VALUES, function( i, docPosition ) {
supportjQuery.each( POSITION_VALUES, function( i, bodyPosition ) {
supportjQuery.each( [ "static", "absolute" ], function( i, markerPosition ) {

testIframe( "coordinates relative to document" +
" (html{position:" + docPosition + "} body{position:" + bodyPosition + "} div{position:" + markerPosition + "})",
"offset/rel-doc.html", function( assert, $, iframe, doc ) {
assert.expect( 256 );

var docElem = doc.documentElement, // `<html>` element
bodyElem = doc.body,
marker1 = doc.getElementById( "marker1" ),
marker2 = doc.getElementById( "marker2" ),
$marker1 = $( marker1 ),
$marker2 = $( marker2 ),
affectProps, markerTopLeft,

// Bit as Flag of box-properties
FLAG_DOC_MARGIN = 1,
FLAG_DOC_BORDER = 2,
FLAG_DOC_PADDING = 4,
FLAG_BODY_MARGIN = 8,
FLAG_BODY_BORDER = 16,
FLAG_BODY_PADDING = 32,
Copy link
Member

@gibson042 gibson042 May 2, 2016 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You said that situations() should be called in each case of that 16 cases. Is my interpretation correct?

Not exactly., I'm saying that situations and "CASE 1"/"CASE 2"/"CASE 3" should be removed in favor of a simpler approach. The testIframe callback shouldn't need to do too much:

// Assume initial conditions include html/document margin/border/padding:
// html { font-size: 10px; margin: 1em; border: 2em solid red; padding: 4em; }
// body { margin: 8em; border: 16em solid blue; padding: 32em; }

// …and a static element and an absolutely-positioned element:
// #static { position: static; }
// #absolute { position: absolute; top: 2em; left: 2em; }

// Establish document-relative origins for children of <body>
docElem.style.position = docPosition;
body.style.position = bodyPosition;
var bodyContentOrigin = 10 + 20 + 40 + 80 + 160 + 320;
var origin = 0 +
    (docPosition === "static" ? 0 : 10 + 20) +
    (bodyPosition === "static" ? 0 : 40 + 80 + 160);

// Check offsets
var absoluteOffset = $( "#absolute" ).offset();
assert.deepEqual(
    $( "#static" ).offset(),
    { top: bodyContentOrigin, left: bodyContentOrigin },
    "offset of position:static element includes <html> and <body> box styles" );
assert.deepEqual( absoluteOffset, { top: origin + 20, left: origin + 20 },
    "offset of position:absolute element ignores box styles of position:static ancestors" );
$( "#absolute, #static" ).offset( absoluteOffset );
assert.deepEqual( $( "#absolute" ).offset(), absoluteOffset, "offset() round-trips" );
assert.deepEqual( $( "#static" ).offset(), absoluteOffset, "offset() is transitive" );
$( "#static" ).css( "position", "static" );

// Reposition html and body, tracking origin adjustments given margin/border/padding
var originAdjust = 0;
$( docElem ).css( { top: "1.5em", left: "1.5em" } );
if ( docPosition !== "static" ) {
    originAdjust += 15 - (docPosition === "relative" ? 0 : 10);
}
$( body ).css( { top: "3em", left: "3em" } );
if ( bodyPosition === "fixed" || bodyPosition === "absolute" && docPosition === "static" ) {

    // html box styles no longer matter
    origin = 80 + 160;
    bodyContentOrigin = origin + 320;
    originAdjust = 30;
} else if ( bodyPosition !== "static" ) {
    originAdjust += 30 - (bodyPosition === "relative" ? 0 : 40);
}

// Recheck offsets
absoluteOffset = $( "#absolute" ).offset();
assert.deepEqual(
    $( "#static" ).offset(),
    { top: bodyContentOrigin + originAdjust, left: bodyContentOrigin + originAdjust },
    "offset of position:static element respects ancestor positioning" );
assert.deepEqual(
    absoluteOffset,
    { top: origin + originAdjust + 20, left: origin + originAdjust + 20 },
    "offset of position:absolute respects ancestor positioning" );
$( "#absolute, #static" ).offset( absoluteOffset );
assert.deepEqual( $( "#absolute" ).offset(), absoluteOffset, "offset() still round-trips" );
assert.deepEqual( $( "#static" ).offset(), absoluteOffset, "offset() is still transitive" );

And there could easily be ways to simplify even further.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I have to learn English...
So, please check following:

  • I interpreted your comment in that issue as that the code should be tested with changing margin and border. Did I mistake again?
  • All assert.deepEqual()s that are passed return value from .offset() failed.
    I found this:
    https://github.com/jquery/jquery/blob/master/external/qunit/qunit.js#L1628
    if ( a.constructor === b.constructor ) {
    Expected values that were made in the test code and return values from .offset() that were made in iframe have constructor in different namespaces (i.e. window), therefore a.constructor === b.constructor is false.
    Return values from .offset() have to be copied to be checked by assert.deepEqual().
    supportjQuery.extend({}, $( "#static" ).offset())
  • This test failed when html{position} is static and body{position} is non-static.
assert.deepEqual( absoluteOffset, { top: origin + 20, left: origin + 20 },
    "offset of position:absolute element ignores box styles of position:static ancestors" );

For example, in a case html{position:static} body{position:relative}:

Expected: 

{
  "left": 300,
  "top": 300
}

Result: 

{
  "left": 330,
  "top": 330
}

I think, this code is incorrect:

var origin = 0 +
    (docPosition === "static" ? 0 : 10 + 20) +
    (bodyPosition === "static" ? 0 : 40 + 80 + 160);

If body has position:non-static, the element is positioned relative to body without being affected by position of <html>. body was already positioned with margin, border and padding of <html>.
Therefore the code should be:

var origin =
    bodyPosition !== "static" ? 10 + 20 + 40 + 80 + 160 : // `10 + 20 + 40` must be included
    docPosition !== "static" ? 10 + 20 : // relative to inside `<html>`
    0; // relative to document (i.e. outside `<html>`)
  • This test failed when html{position} is not static or relative.
assert.deepEqual(
    supportjQuery.extend({}, $( "#static" ).offset()),
    { top: bodyContentOrigin + originAdjust, left: bodyContentOrigin + originAdjust },
    "offset of position:static element respects ancestor positioning" );

I think, this code is incorrect:

if ( docPosition !== "static" ) {
    originAdjust += 15 - (docPosition === "relative" ? 0 : 10);
}

If <html> has position:non-static, it is positioned with margin even if potision is other than relative.
Therefore the code should be:

if ( docPosition !== "static" ) {
    originAdjust += 15;
}
  • This test failed in some cases:
assert.deepEqual(
    absoluteOffset,
    { top: origin + originAdjust + 20, left: origin + originAdjust + 20 },
    "offset of position:absolute respects ancestor positioning" );

This is solved by fixing originAdjust and origin.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test code I wrote might do the almost same as your code, by removing code that changes margin and border.
But I didn't think that it should change top and left of <html> and body.
Therefore I think that I can't write PR. Maybe, I can't understand your request well by my very poor English.
Then, I hope that someone write PR.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, why the test doesn't need to change border?
I think that this is PR to fix bug with clientTop/clientLeft (i.e. border).
At least, it should test in cases of that <html> has border and no border.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I have to learn English...
So, please check following:

No problem. And it's not all your misunderstanding; my suggestions are changing as I learn new details of browser behavior around this issue.

I interpreted your comment in that issue as that the code should be tested with changing margin and border. Did I mistake again?

It was a partial mistake. I wasn't saying that margin and border need to change, just that test cases should cover both zero and nonzero values. Further, we already have tests for the no-margin no-border no-padding cases, and I have since concluded that the "only margin" and "only border" cases are unnecessary, so this PR can focus on situations in which all the box styles are nonzero.

Return values from .offset() have to be copied to be checked by assert.deepEqual()… supportjQuery.extend({}, $( "#static" ).offset())

👍

If body has position:non-static, the element is positioned relative to body without being affected by position of <html>. body was already positioned with margin, border and padding of <html>.
Therefore the code should be:

var origin =
    bodyPosition !== "static" ? 10 + 20 + 40 + 80 + 160 : // `10 + 20 + 40` must be included
    docPosition !== "static" ? 10 + 20 : // relative to inside `<html>`
    0; // relative to document (i.e. outside `<html>`)

I definitely mis-defined origin, and I agree that your fix is correct.

I think, this code is incorrect:

if ( docPosition !== "static" ) {
    originAdjust += 15 - (docPosition === "relative" ? 0 : 10);
}

If <html> has position:non-static, it is positioned with margin even if potision is other than relative.
Therefore the code should be:

if ( docPosition !== "static" ) {
    originAdjust += 15;
}

Again, a correct fix for my mistake.

The test code I wrote might do the almost same as your code, by removing code that changes margin and border.
But I didn't think that it should change top and left of <html> and body.
Therefore I think that I can't write PR. Maybe, I can't understand your request well by my very poor English.
Then, I hope that someone write PR.

I really think we're close. My complaint wan't about what you were testing, just how it was being tested. And as shown above, you clearly have the right insights.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for your very attentive care.
I understood that the test doesn't have to change border.

I have more questions.

  • Why does the code change the top/left of html/body?
    I think that these properties affect getBoundingClientRect() method of browser, and .offset() method of jQuery calculates with a return value of the getBoundingClientRect() without considering top/left.
    That is, if the result become incorrect by changing top/left, it is bug of getBoundingClientRect() method (i.e. browser), not jQuery.
    Also, top/left of html/body are usually not changed.
  • Why is the code that passes a return value of .offset() to another element unnecessary?
    https://github.com/anseki/jquery/blob/offset-html-border/test/unit/offset.js#L650
    This is not "round-trip". I thought that the test should compare an element that was positioned by .offset() and another element that was positioned by native CSS, for setOffset().
    setOffset() must position an element at the same position as another element that was positioned by various CSS properties (position, border, etc.).
    I think that a cases of an element that has position:static are important because setOffset() calculates distance from current coordinates. offset method returns incorrect value in Firefox #3080 (comment)
    That is, it compares coordinates that was got without .offset and coordinates that was changed by .offset that should not be affected by border of <html>.

Copy link
Member

@gibson042 gibson042 May 5, 2016 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the code change the top/left of html/body?
I think that these properties affect getBoundingClientRect() method of browser, and .offset() method of jQuery calculates with a return value of the getBoundingClientRect() without considering top/left.
That is, if the result become incorrect by changing top/left, it is bug of getBoundingClientRect() method (i.e. browser), not jQuery.

If there are such bugs, we need to know so we can work around them (or document our failure to do so).

Also, top/left of html/body are usually not changed.

Nor do html or body elements usually have borders. The point is verifying that our functions perform as claimed.

Why is the code that passes a return value of .offset() to another element unnecessary?
…
I think that a cases of an element that has position:static are important because setOffset() calculates distance from current coordinates. #3080 (comment)

I agree. We should do both $( "#absolute" ).offset( absoluteOffset ) and $( "#static" ).offset( absoluteOffset ) and expect the subsequent .offset() to match absoluteOffset. I updated my suggestions accordingly.


// Pixels
DOC_MARGIN = 1,
DOC_BORDER = 2,
DOC_PADDING = 4,
BODY_MARGIN = 8,
BODY_BORDER = 16,
BODY_PADDING = 32,
MARKER_TOP_LEFT = 64;

// Test it in each situation that is made by combined box-properties.
function testWithBoxProps( affectProps, markerTopLeft ) {
var props, offset1, offset2, sumLen, correctTopLeft, labels, labelsInMessage;

function switchProp( elem, enable, affect, styleProp, pixels, label ) {
if ( enable ) {
elem.style[ styleProp ] = pixels + "px";
if ( affect ) {
sumLen += pixels;
label = "[*]" + label;
} else {
label = "[v]" + label;
}
} else {
elem.style[ styleProp ] = "0";
label = "[-]" + label;
}
labels.push( label );
}

for ( props = 0;
props <= ( FLAG_DOC_MARGIN | FLAG_DOC_BORDER | FLAG_DOC_PADDING | FLAG_BODY_MARGIN | FLAG_BODY_BORDER | FLAG_BODY_PADDING );
props++ ) {

sumLen = 0;
labels = [];

switchProp( docElem, props & FLAG_DOC_MARGIN, affectProps & FLAG_DOC_MARGIN, "margin", DOC_MARGIN, "document-margin" );
switchProp( docElem, props & FLAG_DOC_BORDER, affectProps & FLAG_DOC_BORDER, "borderWidth", DOC_BORDER, "document-border" );
switchProp( docElem, props & FLAG_DOC_PADDING, affectProps & FLAG_DOC_PADDING, "padding", DOC_PADDING, "document-padding" );
switchProp( bodyElem, props & FLAG_BODY_MARGIN, affectProps & FLAG_BODY_MARGIN, "margin", BODY_MARGIN, "body-margin" );
switchProp( bodyElem, props & FLAG_BODY_BORDER, affectProps & FLAG_BODY_BORDER, "borderWidth", BODY_BORDER, "body-border" );
switchProp( bodyElem, props & FLAG_BODY_PADDING, affectProps & FLAG_BODY_PADDING, "padding", BODY_PADDING, "body-padding" );

offset1 = $marker1.offset();
// If getter works correctly, the returned values can be compared with result of setter.
offset2 = $marker2.offset( offset1 ).offset(); // Set and Get

correctTopLeft = markerTopLeft + sumLen;
// `*` means that the property should affect position
labelsInMessage = " - Props (v:enabled, *:affect): " + labels.join( " " );
assert.equal( offset1.top, correctTopLeft, "Get `top`" + labelsInMessage );
assert.equal( offset1.left, correctTopLeft, "Get `left`" + labelsInMessage );
assert.equal( offset2.top, correctTopLeft, "Set `top`" + labelsInMessage );
assert.equal( offset2.left, correctTopLeft, "Set `left`" + labelsInMessage );
}
}

docElem.style.position = docPosition;
bodyElem.style.position = bodyPosition;
marker1.style.position = markerPosition;
affectProps = 0;
markerTopLeft = 0;

if ( markerPosition === "absolute" ) {
marker1.style.top = MARKER_TOP_LEFT + "px";
marker1.style.left = MARKER_TOP_LEFT + "px";
markerTopLeft = MARKER_TOP_LEFT;

if ( bodyPosition !== "static" ) {
// When `<body>` has `position:(non-static)` and `marker1` has `position:absolute`,
// `marker1` is positioned relative to inside `<body>` border.
// Therefore, `.offset()` should return `top/left` of `marker1` plus
// * document-margin, document-border, document-padding
// * body-margin, body-border
affectProps |= FLAG_DOC_MARGIN | FLAG_DOC_BORDER | FLAG_DOC_PADDING | FLAG_BODY_MARGIN | FLAG_BODY_BORDER;

} else if ( docPosition !== "static" ) {
// When `<body>` has `position:static` and `<html>` has `position:(non-static)` and
// `marker1` has `position:absolute`, `marker1` is positioned relative to inside
// `<html>` border.
// Therefore, `.offset()` should return `top/left` of `marker1` plus
// * document-margin, document-border
affectProps |= FLAG_DOC_MARGIN | FLAG_DOC_BORDER;
}

// When `<body>` has `position:static` and `<html>` has `position:static` and `marker1`
// has `position:absolute`, `marker1` is positioned relative to the initial container
// (i.e. outer edge of `<html>` margin).
// https://developer.mozilla.org/en-US/docs/Web/CSS/position#Absolute_positioning
// Therefore, `.offset()` should return `top/left` of `marker1` without being
// affected by `margin`, `border` and `padding` of `<html>` and `<body>`.

} else {
// When `marker1` has `position:static`, `marker1` is laid out in its current position
// in the flow.
// Therefore, `.offset()` should return sum of
// * document-margin, document-border, document-padding
// * body-margin, body-border, body-padding
affectProps |= FLAG_DOC_MARGIN | FLAG_DOC_BORDER | FLAG_DOC_PADDING | FLAG_BODY_MARGIN | FLAG_BODY_BORDER | FLAG_BODY_PADDING;
}

testWithBoxProps( affectProps, markerTopLeft );
} );

} );
} );
} );

})();

} )();