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
Offset: Operate "relative to document" correctly #3096
Conversation
Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA). 📝 Please visit http://contribute.jquery.org/CLA/ to sign. After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know. If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check. |
I don't know why "CLA:Error"... 😢 |
Related to: jquery#3080 Remove unnecessary code that subtracts `document.documentElement.clientTop/clientLeft` (i.e. `border-width` of `<html>`) from coordinates. In Firefox, those are not subtracted because `clientTop/clientLeft` are `0` always by accidentally bug.
@anseki See http://contribute.jquery.org/CLA/status/?owner=jquery&repo=jquery&sha=359da5dd3b080c4c6367ec904fa9325a1d4f0305 . We ask for your real email address and real name (or consistently-used pseudonym), and require that the git commits match the CLA. Is "AnSeki" the best name for you to use for assigning contributions? |
Thank you @gibson042.
What should I do? |
The current exception is about the name field. Do you have a more "traditional" name, or is "AnSeki" the best descriptor for us? |
I've been using this name anytime, and I hope use this. |
Fix: incorrect indention
|
||
for ( flags = 0; | ||
flags <= ( FLAG_DOC_MARGIN | FLAG_DOC_BORDER | FLAG_DOC_PADDING | FLAG_BODY_MARGIN | FLAG_BODY_BORDER | FLAG_BODY_PADDING ); | ||
flags++ ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very good start, but is a bit hard to read and hard to reason about. I now think every interesting case here can operate in a page with nonzero, non-pixel (to catch inappropriate parseFloat
s) values for all of margin, border, and padding on both <html>
and <body>
, with the only variation being position
values for those two elements. You can achieve that by means of a single iframe fixture (setting position
styles before making any other calls), but I would like them in distinct testIframe
invocations (see effects.js for an example of such process-constructed QUnit.test
input). The cases, and my observations of them, are as follows:
<html> position |
<body> position |
.offset origin |
---|---|---|
static | static/relative/absolute/fixed | document (i.e., outer edge of <html> margin) |
relative/absolute/fixed | static/relative/absolute/fixed | inside <html> border |
That's a total of 16 cases, each case with its own testIframe
. I'm aware that <body>
position doesn't actually seem to matter, but it's probably worth covering anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I try to rewrite tests.
You said that situations()
should be called in each case of that 16 cases. Is my interpretation correct?
Also, that 16 cases include two cases in current code "CASE 1" and "CASE 2".
Is "CASE 3" in current code unnecessary?
Or, should two cases marker1.style.position = "absolute"
and marker1.style.position = "static"
be tested in each case of that 16 cases? That is, situations()
is called in each case of 2x16 cases.
It seems "CLA:Error" was solved by second commit. Thank you. |
- Add more test cases of `position` variation of `<html>` and `<body>`. - Do `testIframe()` in each case.
I added test cases, then
Total: 8192 cases Also, I tried to change messages. |
Add comment as information for tester who see the test code also when the test failed.
FLAG_DOC_PADDING = 4, | ||
FLAG_BODY_MARGIN = 8, | ||
FLAG_BODY_BORDER = 16, | ||
FLAG_BODY_PADDING = 32, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
andborder
. 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 iniframe
have constructor in different namespaces (i.e. window), thereforea.constructor === b.constructor
isfalse
.
Return values from.offset()
have to be copied to be checked byassert.deepEqual()
.
supportjQuery.extend({}, $( "#static" ).offset())
- This test failed when
html{position}
isstatic
andbody{position}
isnon-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 notstatic
orrelative
.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
andborder
. 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 byassert.deepEqual()
…supportjQuery.extend({}, $( "#static" ).offset())
👍
If
body
hasposition:non-static
, the element is positioned relative tobody
without being affected byposition
of<html>
.body
was already positioned withmargin
,border
andpadding
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>
hasposition:non-static
, it is positioned withmargin
even ifpotision
is other thanrelative
.
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
andborder
.
But I didn't think that it should changetop
andleft
of<html>
andbody
.
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.
There was a problem hiding this comment.
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
ofhtml/body
?
I think that these properties affectgetBoundingClientRect()
method of browser, and.offset()
method of jQuery calculates with a return value of thegetBoundingClientRect()
without consideringtop/left
.
That is, if the result become incorrect by changingtop/left
, it is bug ofgetBoundingClientRect()
method (i.e. browser), not jQuery.
Also,top/left
ofhtml/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, forsetOffset()
.
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 hasposition:static
are important becausesetOffset()
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>
.
There was a problem hiding this comment.
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
ofhtml/body
?
I think that these properties affectgetBoundingClientRect()
method of browser, and.offset()
method of jQuery calculates with a return value of thegetBoundingClientRect()
without consideringtop/left
.
That is, if the result become incorrect by changingtop/left
, it is bug ofgetBoundingClientRect()
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
ofhtml/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 hasposition:static
are important becausesetOffset()
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.
I reconsidered about setter. Therefore if getter works correctly, things that affect setter are:
I think that the test doesn't need to change I will rewrite the test later. |
- Remove the code that changes box-properties of `<html>` and `body`. - Add `testIframe` to test the setter.
BTW, I found another issue. I found that I send this as "new issue" later. |
We don't need to change |
html { | ||
margin: 1px; | ||
border-width: 2px; | ||
padding: 4px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is important to me that all of these units are not pixels.
I see. |
- Copy the code from jquery#3096 (comment) - Wrap `.offset()` by `supportjQuery.extend`. - Fix `origin` calucuration. - Fix `originAdjust` calucuration.
"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 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$
-> jQuery
As discussed at today's meeting, we'll hold off on changing the |
Summary
Fix:
.offset()
doesn't operate "relative to document" correctlyChecklist
Mark an
[x]
for completed items, if you're not sure leave them unchecked and we can assist.Thanks! Bots and humans will be around shortly to check it out.
Related to: #3080
Remove unnecessary code that subtracts
document.documentElement.clientTop/clientLeft
(i.e.border-width
of<html>
) from coordinates.In Firefox, those are not subtracted because
clientTop/clientLeft
are0
always by accidentally bug.