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

`offset` method returns incorrect value in Firefox #3080

Closed
anseki opened this issue Apr 26, 2016 · 12 comments
Closed

`offset` method returns incorrect value in Firefox #3080

anseki opened this issue Apr 26, 2016 · 12 comments
Assignees
Milestone

Comments

@anseki
Copy link

@anseki anseki commented Apr 26, 2016

Hi,
I found a case that offset method returns incorrect value in Firefox.
offset method subtracts docElem.clientTop/clientLeft (i.e. border-width of html element), but Firefox seems to return 0 as clientTop/clientLeft of html element always.

For example:
https://jsfiddle.net/xwuc0g6q/

<html>
  <body>
    <div id="box1"></div>
  </body>
</html>
html {
  border: 10px solid red;
}

body {
  border: 10px solid blue;

  margin: 20px;
  padding: 30px;

  /*position: relative;*/
}

#box1 {
  position: absolute;
  left: 0;
  top: 0;
  width: 50px;
  height: 50px;
  background-color: green;
}
$(function() {

  var elem = document.getElementById('box1');

  // <html>.clientLeft/clientTop
  // from https://github.com/jquery/jquery/blob/305f193aa57014dc7d8fa0739a3fefd47166cd44/src/offset.js#L78
  var doc = elem.ownerDocument,
    docElem = doc.documentElement;
  console.log(docElem.tagName + ' > clientLeft: ' + docElem.clientLeft + ' clientTop: ' + docElem.clientTop);

  // document.body.clientLeft/clientTop
  var body = document.body;
  console.log(body.tagName + ' > clientLeft: ' + body.clientLeft + ' clientTop: ' + body.clientTop);

  // getBoundingClientRect
  var box = elem.getBoundingClientRect();
  console.log('BoundingBox > left: ' + box.left + ' top: ' + box.top);

  // jQuery#offset
  var offset = $(elem).offset();
  console.log('offset() > left: ' + offset.left + ' top: ' + offset.top);

});

Res in Google Chrome:

HTML > clientLeft: 10 clientTop: 10
BODY > clientLeft: 10 clientTop: 10
BoundingBox > left: 0 top: 0
offset() > left: -10 top: -10

Res in IE:

HTML > clientLeft: 10 clientTop: 10
BODY > clientLeft: 10 clientTop: 10
BoundingBox > left: 0 top: 0
offset() > left: -10 top: -10

Res in Firefox:

HTML > clientLeft: 0 clientTop: 0
BODY > clientLeft: 10 clientTop: 10
BoundingBox > left: 0 top: 0
offset() > left: 0 top: 0

But I can't understand why the method subtracts docElem.clientTop/clientLeft.
If it do that for getting the position relative to the document, it should calculate based on border-box (i.e. outline that includes border). The box1 element in above code is positioned based on border-box even if html has border.
And also, html usually has no border.

Then, I feel that left: 0, top: 0 is correct result, and it is useful.

@dmethvin
Copy link
Member

@dmethvin dmethvin commented Apr 27, 2016

If it do that for getting the position relative to the document, it should calculate based on border-box (i.e. outline that includes border)

The jQuery code predates the existence of border-box by several years, so it may have just been overlooked.

And also, html usually has no border.

I suspect that is why this hasn't been reported or noticed, if it is actually a bug.

Then, I feel that left: 0, top: 0 is correct result, and it is useful.

If the HTML element has a border would the position be from the inside of the border, or the outside? I don't know and a quick search into the specs didn't help.

Now we need some people who actually understand how we got here. Maybe @mikesherov ?

@hexalys
Copy link

@hexalys hexalys commented Apr 27, 2016

Very odd. If anything it should add margins in border-box box-sizing context.

I have just filed a Firefox bug for the clientLeft/clientTop issue.

@gibson042
Copy link
Member

@gibson042 gibson042 commented Apr 28, 2016

<html> border/padding and <body> margin/border/padding have always made things tricky, but defining .offset() to be "relative to the document" means that correct behavior is that for which the top and left values match fixed positioning (i.e., the original left: 0, top: 0 proposal of this issue).

@anseki
Copy link
Author

@anseki anseki commented Apr 28, 2016

I see.
One of important points is:
As I said, positioning by CSS uses border-box. Therefore jQuery methods that use CSS positioning are incompatible with offset method.
offset(coordinates) method (i.e. setter) is also incompatible.
For example:
https://jsfiddle.net/0ac41fxw/

<html>
  <body>
    <div id="box1"></div>
    <div id="box2"></div>
  </body>
</html>
html {
  border: 10px solid red;
}

body {
  border: 10px solid blue;
  margin: 20px;
  padding: 30px;
}

#box1, #box2 {
  position: absolute;
  width: 50px;
  height: 50px;
}

#box1 {
  background-color: rgba(0, 128, 0, 0.5); /* translucent green */
  left: 0;
  top: 0;
}

#box2 {
  background-color: rgba(128, 0, 0, 0.5); /* translucent red */
}
$('#box2').offset($('#box1').offset());

I gave a coordinates that was given from offset method to offset method. Then I expected box2 to be positioned at the position of box1.

Chrome:
ch

IE:
ie

Firefox:
ff

Result in Firefox seems correct.

The setter of offset method (jQuery.offset.setOffset) seems to subtract left/top that was returned by offset from passed left/top. The passed left/top were already subtracted. That is, those were subtracted twice. Therefore the element was positioned at 20px shifted point (not 10px).

@anseki
Copy link
Author

@anseki anseki commented Apr 28, 2016

MouseEvent.pageX/pageY also might be influenced.

( doc && doc.clientLeft || body && body.clientLeft || 0 );

If native event don't have pageX/Y, these are calculated using clientLeft/clientTop.

If html element has border-width:10px like the example above, in a browser that doesn't support event.pageX/Y and correct clientTop/clientLeft, jQuery returns 10,10 as pageX/Y, when mouse is positioned at left-top-corner of body. In a browser that returns correct clientTop/clientLeft, jQuery returns 0,0 as pageX/Y.

I feel that 10,10 is correct result.

But I don't know recent browser that doesn't support event.pageX/Y.

@gibson042
Copy link
Member

@gibson042 gibson042 commented Apr 29, 2016

Let's keep this issue focused on .offset. It seems to me that both the getter and setter should use an origin at the top left corner of the document element, just at the outer edge of any border margin that might be applied to it the document element. Fulfilling this may require a feature detect or style computation to work correctly with Firefox. Would you like to try a PR, @anseki?

@anseki
Copy link
Author

@anseki anseki commented Apr 29, 2016

@gibson042 Ok, I will make a new issue about MouseEvent.pageX/pageY.
If subtracting clientTop/clientLeft is mistake, the code can be fixed easily.
offset method does not subtract clientTop/clientLeft in Firefox by accidentally bug. Therefore, we don't need to consider that bug to remove code that subtract clientTop/clientLeft.

Yes, I can write PR. I already removed the code that subtract clientTop/clientLeft for testing this issue before I send this issue.
But I think, we should clear that why clientTop/clientLeft are subtracted.
I found this issue:
#1892
@staabm said that it do that to returns the coordinates relative to the document. That is, the outline of html is not document.
So, what is "document" jQuery think?
I think the document is html, but at least, the person who wrote this code does not think so.
Am I thinking too much?

@gibson042
Copy link
Member

@gibson042 gibson042 commented Apr 29, 2016

I just did some further exploration, and confirmed that all four engines respect document element margin as well (and do so independently of absolute positioning). So, to revise the above, .offset should operate "relative to document" by using the outermost absolute positioning origin, which is not necessarily inside the border box of the document element (and therefore clientTop/clientLeft are probably insufficient anyway). The PR should add tests for nonzero, nonpixel <html> margin and border (both separately and together), and should not break any existing tests (but it wouldn't surprise me if the second point doesn't hold because of some bad assumptions).

@anseki
Copy link
Author

@anseki anseki commented Apr 30, 2016

.offset method itself and .position method use values that are returned by .offset, and I understand that the sufficient tests are required.
Then, I hope that someone write PR because I don't know rules and styles of testing in jQuery well.

@gibson042
Copy link
Member

@gibson042 gibson042 commented Apr 30, 2016

The new tests will be limited to .offset, and similar to existing ones. And the test suite runs in-browser by loading test/index.html. We're willing to help you through it. Care to try?

@anseki
Copy link
Author

@anseki anseki commented May 1, 2016

Thank you for your care 😃
And, sorry my English is very poor.

I will send PR later.
By the way, I think that tests should contain cases of an element that is positioned with being affected by border-width of <html> too. That is, when the element is positioned relative to <body> (i.e. <body> has position:relative, or the element doesn't have position), result value from .offset should include border-width of <html>.
Anyway, I try to write tests.

anseki added a commit to anseki/jquery that referenced this issue May 1, 2016
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 anseki mentioned this issue May 1, 2016
3 of 4 tasks complete
anseki added a commit to anseki/jquery that referenced this issue May 1, 2016
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.
@timmywil timmywil added the Offset label Jun 30, 2016
@timmywil timmywil modified the milestone: 3.1.1 Jun 30, 2016
@timmywil timmywil modified the milestones: 3.1.1, 3.2.0 Jul 13, 2016
gibson042 added a commit to gibson042/jquery that referenced this issue Jan 5, 2017
gibson042 added a commit to gibson042/jquery that referenced this issue Jan 5, 2017
@gibson042 gibson042 mentioned this issue Jan 5, 2017
4 of 4 tasks complete
@timmywil timmywil modified the milestones: 3.2.0, 3.3.0 Mar 15, 2017
@timmywil timmywil modified the milestones: 3.3.0, 3.2.0 Mar 15, 2017
gibson042 added a commit to gibson042/jquery that referenced this issue Mar 20, 2017
gibson042 added a commit to gibson042/jquery that referenced this issue Mar 20, 2017
gibson042 added a commit that referenced this issue Apr 24, 2017
@anseki
Copy link
Author

@anseki anseki commented Apr 24, 2017

Thank you 😄

@lock lock bot locked as resolved and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants
You can’t perform that action at this time.