Jan Reporter
	Description • 9 years ago

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/41.0.2272.118 Safari/537.36

Steps to reproduce:

element.getBoundingClientRect() is frequently used to determine the position of an element on the page.
Taking into account the current scroll position it should return a consistent offset position for every element.

I noticed the returned values are inconsistent though and I created a simplified test scenario: http://jsfiddle.net/janpaepke/okeg4a7n/

In this scenario I return the current document offset of an element and log it to the console.



Actual results:

When scrolling up and down, while looking at the console, the values aren't constant.

Tested and confirmed also in Firefox Nightly 40.0a1 (2015-04-06)


Expected results:

The value should be consistant, regardless of scroll position, as the position of the element doesn't change.

Safari and Chrome both handle this as expected.
Safari doesn't seem to support Sub-Pixel, which might be the reason it works there.
Chrome does, though, and it works fine there as well.

	Markus Stange [:mstange]
	Comment 1 • 9 years ago

The reason for this is that getBoundingClientRect gives you subpixel precision, but window.pageYOffset rounds. If window.pageYOffset were subpixel-precise as well, the value would be consistent.

	Jan Reporter
	Comment 2 • 9 years ago

If that's true, why does the bug not occur on Chrome, where getBoundingClientRect is also sub-pixel precise and window.pageYOffset seems to be rounded?

And what would you suggest to use as a reliable form of document related position calculation?

	Markus Stange [:mstange]
	Comment 3 • 9 years ago

I don't know what Chrome does. Gecko adjusts the page scroll position by a fractional amount once you hit the bottom of the page (if the scrollable area has a fractional height). Chrome probably doesn't do that.

As for the reliable form, maybe you can compare with the getBoundingClientRect for a different element? I'd need to do some more testing first.

	Alice0775 White
	Comment 4 • 9 years ago

I tested on IE11. getBoundingClientRect is subpixel precision on IE11 like Firefox.

	Jan Reporter
	Comment 5 • 9 years ago

(In reply to Alice0775 White from comment #4)
> I tested on IE11. getBoundingClientRect is subpixel precision on IE11 like
> Firefox.

I think that's a good thing. getBoundingClientRect SHOULD be sub-pixel precise. But still the returned value should be independent of scroll position or rounding issues in scroll position, don't you agree?

What I think may be happening is that Firefox also supports sub-pixels in scrolling, but doesn't return them. So if the total page height is not a full pixel value the scroll position will increment by full px until the bottom of the page is reached. then, because there is still say 0.4 px more to scroll, it will scroll it, causing the real scroll position to be 0.4 px more than what is returned using window.pageYOffset.
These 0.4 px will however be substracted from the value returned by getBoundingClientRect, as it is related to viewport and the viewport is 0.4 px further down.

But as these 0.4 px are not reflected in window.pageYOffset because it is rounded (as Markus says) the calculated position for the element changes.

One solution would be to return the real, unrounded scroll position.
But my guess is there's a convention somewhere that says it needs to be full px.

So how about getBoundingClientRect returns the position of the element as if the viewport was where the returned scroll position suggests it is?

	Jan Reporter
	Comment 6 • 9 years ago

* These 0.4 px however be ADDED TO the value
(sorry)

	ovidiu boca[:Ovidiu]
	Comment 7 • 9 years ago

Hi, 

This is a bit too technical for me to start debugging, but this looks like related to Bug 1014738. Maybe it would be best if someone with more experience on the matter would take a look over this.

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 9 years ago

The right solution here is to change window.pageYOffset to return non-integer values, if that's web-compatible.  Looks like Blink has in fact done that, so maybe it is.  I thought we had an existing bug on it, but I don't see one.

I'm not sure what set of props we need to switch over all at once, but seems like at least scrollX/Y and pageX/YOffset on Window should change all together.  And maybe scrollTop/Left on elements, though note that scrollWidth/Height on elements are still ints in Blink.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 10 • 7 years ago
mozreview-review

Comment on attachment 8843055 [details]
Bug 1151421 Part 1: Change Window scroll and offset properties to be doubles instead of ints.

https://reviewboard.mozilla.org/r/116802/#review118810

We need some test for this, hopefully wpt test.
Since scrollTo etc already use double, hopefully writing a test should be quite easy.
At least some test that if subpixel scrolling is happening, right values are then reported in the attributes.

r+, assuming a test is written.

	Brad Werth [:bradwerth] Assignee
	Comment 11 • 7 years ago

(In reply to Markus Stange [:mstange] from comment #3)
> I don't know what Chrome does. Gecko adjusts the page scroll position by a
> fractional amount once you hit the bottom of the page (if the scrollable
> area has a fractional height). 

Markus, can you direct me to the code that does this? I'm trying to write a test for the patch, and I can only trigger the test condition with manual scrolling. Instead of rigging a test to simulate a mouse input just for this effect, I'd like to inspect the code path and see if there is an API I can use to set the offset for testing (scrollTo rounds to layer pixels).

	Markus Stange [:mstange]
	Comment 12 • 7 years ago

We changed this in bug 1012752 so that it no longer happens. We now try to have the scroll position always be a multiple of the device pixel size.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 15 • 7 years ago
mozreview-review

Comment on attachment 8846923 [details]
Bug 1151421 Part 2: Add test of fractional scroll position properties.

https://reviewboard.mozilla.org/r/119946/#review121976

If there no way to check that we actually get fractional scroll position occasionally?
But this at least verifies sane (content.getBoundingClientRect().top + cw.pageYOffset); handling

	Pulsebot
	Comment 16 • 7 years ago

Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7ce372883419
Part 1: Change Window scroll and offset properties to be doubles instead of ints. r=smaug
https://hg.mozilla.org/integration/autoland/rev/a84ec98ccaab
Part 2: Add test of fractional scroll position properties. r=smaug

	Brad Werth [:bradwerth] Assignee
	Comment 17 • 7 years ago
mozreview-review-reply

Comment on attachment 8846923 [details]
Bug 1151421 Part 2: Add test of fractional scroll position properties.

https://reviewboard.mozilla.org/r/119946/#review121976

There's currently no setter that can set a fractional pixel -- it gets rounded off to "Layer" pixels pretty deep in the call stack. The original testcase of forced scrolling to the bottom was the only way I could find to trigger a fractional pageYOffset. I'm sorry to say that I missed this review comment before landing the patches. I will open a new bug to modify the test to check that pageYOffset has a fractional component after the scroll.

	Pulsebot
	Comment 19 • 7 years ago

Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/486692b1d179
Backed out changeset a84ec98ccaab 
https://hg.mozilla.org/integration/autoland/rev/0bf31f0819dc
Backed out changeset 7ce372883419 for failures in Android mochitests, e.g. test_bug1151421.html. r=backout

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 20 • 7 years ago

Backed out for failures in Android mochitests, e.g. test_bug1151421.html:

https://hg.mozilla.org/integration/autoland/rev/0bf31f0819dc20a0736803b814d4f88f95609031
https://hg.mozilla.org/integration/autoland/rev/486692b1d1792d304deeb10ca2d657cba790aa9c

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a84ec98ccaabd854a38d9d33b632b39f500ef7e8&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=83737591&repo=autoland

[task 2017-03-14T16:36:39.708242Z] 16:36:39     INFO -  71 INFO TEST-START | docshell/test/test_bug1151421.html
[task 2017-03-14T16:36:39.711096Z] 16:36:39     INFO -  Buffered messages logged at 16:36:34
[task 2017-03-14T16:36:39.716943Z] 16:36:39     INFO -  72 INFO TEST-PASS | docshell/test/test_bug1151421.html | top plus offset should remain invariant across scrolling.
[task 2017-03-14T16:36:39.717005Z] 16:36:39     INFO -  73 INFO TEST-PASS | docshell/test/test_bug1151421.html | top plus offset should remain invariant across scrolling.
[task 2017-03-14T16:36:39.717055Z] 16:36:39     INFO -  Buffered messages finished
[task 2017-03-14T16:36:39.717141Z] 16:36:39     INFO -  74 INFO TEST-UNEXPECTED-FAIL | docshell/test/test_bug1151421.html | top plus offset should remain invariant across scrolling. - got 87.99999594688416, expected 88
[task 2017-03-14T16:36:39.717198Z] 16:36:39     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:310:5
[task 2017-03-14T16:36:39.717262Z] 16:36:39     INFO -      childLoad2/<@docshell/test/test_bug1151421.html:43:5
[task 2017-03-14T16:36:39.717323Z] 16:36:39     INFO -      childLoad2@docshell/test/test_bug1151421.html:41:3

More failures in https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2d9fb6e6b9ca1771622f6df413ccec2539d2708d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 29 • 7 years ago
mozreview-review

Comment on attachment 8847224 [details]
Bug 1151421 Part 3: Update tests of pageYOffset/scrollY to round off when checking expected results.

https://reviewboard.mozilla.org/r/120232/#review122944

I think tests comparing to 0 should work without any changes. It is rather important to not break that case.
Also, I didn't see test failures comparing about those 0 case. Or did I miss something?

::: docshell/test/navigation/file_scrollRestoration.html:29
(Diff revision 4)
>              window.location.reload(false);
>              break;
>            }
>            case 2: {
>              opener.is(event.persisted, false, "Shouldn't have persisted session history entry.");
> -            opener.isnot(window.scrollY, 0, "Should have restored scrolling.");
> +            opener.isnot(Math.round(window.scrollY), 0, "Should have restored scrolling.");

Why you need to change any of these window.scrollY != 0 tests? That is rather surprising.
Which case is failing on Android?

And even scrollY = 0 case... if expect that we haven't scrolled at all, how come we get non 0?

	Brad Werth [:bradwerth] Assignee
	Comment 30 • 7 years ago
mozreview-review

Comment on attachment 8847224 [details]
Bug 1151421 Part 3: Update tests of pageYOffset/scrollY to round off when checking expected results.

https://reviewboard.mozilla.org/r/120232/#review123012

::: docshell/test/navigation/file_scrollRestoration.html:29
(Diff revision 4)
>              window.location.reload(false);
>              break;
>            }
>            case 2: {
>              opener.is(event.persisted, false, "Shouldn't have persisted session history entry.");
> -            opener.isnot(window.scrollY, 0, "Should have restored scrolling.");
> +            opener.isnot(Math.round(window.scrollY), 0, "Should have restored scrolling.");

I changed ALL the tests of scroll and pageOffset to round off, for consistency. It's possible these initial condition tests don't fail. I'll scale back to only round on non-zero tests and we'll see what the try server says.

	Brad Werth [:bradwerth] Assignee
	Comment 34 • 7 years ago
mozreview-review-reply

Comment on attachment 8847224 [details]
Bug 1151421 Part 3: Update tests of pageYOffset/scrollY to round off when checking expected results.

https://reviewboard.mozilla.org/r/120232/#review123012

> I changed ALL the tests of scroll and pageOffset to round off, for consistency. It's possible these initial condition tests don't fail. I'll scale back to only round on non-zero tests and we'll see what the try server says.

With most recent patch, I changed all of the is 0 tests to remove the rounding. For isnot 0, it seemed clear that rounding was appropriate, otherwise 0.000111 != 0, clearly against the intent of the test.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 35 • 7 years ago
mozreview-review

Comment on attachment 8847224 [details]
Bug 1151421 Part 3: Update tests of pageYOffset/scrollY to round off when checking expected results.

https://reviewboard.mozilla.org/r/120232/#review123116

	Pulsebot
	Comment 36 • 7 years ago

Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/77abcda78b97
Part 1: Change Window scroll and offset properties to be doubles instead of ints. r=smaug
https://hg.mozilla.org/integration/autoland/rev/8c993a5d99b8
Part 2: Add test of fractional scroll position properties. r=smaug
https://hg.mozilla.org/integration/autoland/rev/a2ee4de4944c
Part 3: Update tests of pageYOffset/scrollY to round off when checking expected results. r=smaug

	Carsten Book [:Tomcat]
	Comment 37 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/77abcda78b97
https://hg.mozilla.org/mozilla-central/rev/8c993a5d99b8
https://hg.mozilla.org/mozilla-central/rev/a2ee4de4944c

	Brad Werth [:bradwerth] Assignee
	Comment 38 • 7 years ago

Landed a patch that resolved the problems in the backout in comment 20.

	Kohei Yoshino [:kohei]
	Comment 39 • 7 years ago

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/scrollx-scrolly-pagexoffset-pageyoffset-now-return-double-instead-of-integer/

	Xidorn Quan [:xidorn] UTC+11
	Comment 40 • 7 years ago

I intended to do the same change in bug 1217330 but ceased because of bug 1012752.

Given bug 1012752 is fixed, we probably should also try to fix the rest part of bug 1217330 which includes Window.scroll{Max,Min}{X,Y} and Element.scroll{Left,Top}{,Max,Min}.

Also I guess this kind of change may be worth an "Intend to ship"?

	Eric Shepherd [:sheppy]
	Comment 41 • 7 years ago

Documentation updated:

https://developer.mozilla.org/en-US/docs/Web/API/Window/scrollX
https://developer.mozilla.org/en-US/docs/Web/API/Window/scrollY

https://developer.mozilla.org/en-US/Firefox/Releases/55