Make window.pageYOffset/pageXOffset/scrollX/scrollY double, not integers

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: Jan, Assigned: bradwerth)

Tracking

(Blocks: 1 bug, {dev-doc-complete, site-compat})

37 Branch
mozilla55
x86
Mac OS X
dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

3 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.
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.
(Reporter)

Comment 2

3 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?
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.

Comment 4

3 years ago
I tested on IE11. getBoundingClientRect is subpixel precision on IE11 like Firefox.
(Reporter)

Comment 5

3 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?
(Reporter)

Comment 6

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

Comment 7

3 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.
Flags: needinfo?(bzbarsky)
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.
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: Core & HTML
Ever confirmed: true
Flags: needinfo?(bzbarsky)
Product: Firefox → Core
Summary: Inconsistent results of getBoundingClientRect() → Make window.pageYOffset/pageXOffset/scrollX/scrollY double, not integers
(Assignee)

Updated

a year ago
Assignee: nobody → bwerth
Keywords: dev-doc-needed
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8843055 - Flags: review?(bugs)

Comment 10

a year 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.
Attachment #8843055 - Flags: review?(bugs) → review+
(Assignee)

Comment 11

a year 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).
Flags: needinfo?(mstange)
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.
Flags: needinfo?(mstange)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8846923 - Flags: review?(bugs)

Comment 15

a year 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
Attachment #8846923 - Flags: review?(bugs) → review+

Comment 16

a year 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
(Assignee)

Comment 17

a year 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.
(Assignee)

Updated

a year ago
Blocks: 1347206
(Assignee)

Updated

a year ago
Duplicate of this bug: 1347206

Comment 19

a year 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
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
Flags: needinfo?(bwerth)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8847224 - Flags: review?(bugs)

Comment 29

a year 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?
Attachment #8847224 - Flags: review?(bugs) → review-
(Assignee)

Comment 30

a year 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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 34

a year 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.

Comment 35

a year 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
Attachment #8847224 - Flags: review?(bugs) → review+

Comment 36

a year 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

Comment 37

a year 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
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 38

a year ago
Landed a patch that resolved the problems in the backout in comment 20.
Flags: needinfo?(bwerth)
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"?
You need to log in before you can comment on or make changes to this bug.