Closed Bug 1151421 Opened 5 years ago Closed 3 years ago

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

Categories

(Core :: DOM: Core & HTML, defect)

37 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: e-mail, Assigned: bradwerth)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(3 files)

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.
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.
I tested on IE11. getBoundingClientRect is subpixel precision on IE11 like Firefox.
(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?
* These 0.4 px however be ADDED TO the value
(sorry)
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: nobody → bwerth
Attachment #8843055 - Flags: review?(bugs)
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+
(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)
Attachment #8846923 - Flags: review?(bugs)
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+
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
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.
Blocks: 1347206
Duplicate of this bug: 1347206
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)
Attachment #8847224 - Flags: review?(bugs)
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-
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 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 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+
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
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"?
Depends on: 1542676
You need to log in before you can comment on or make changes to this bug.