1674687 - scroll position of subpixel is not reported by scrollTop /scrollLeft correctly

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[mmis1000]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:84.0) Gecko/20100101 Firefox/84.0
Firefox for Android

Steps to reproduce:

1. Make a scroll container that have a non integer scroll height.
2. load the page in a real hidpi device. (this is important, or you can't reproduce it)
3. scroll the container to bottom.
4. read the scrollTop of the container.

  <div class="c">
    <div class="d">
      max scrollTop is: <span class="st"></span>
      <br>
      rect offset (real scroll top)is: <span class="br"></span>
      <br>
      dpi is: <span class="dpi"></span>
    </div>
  </div>

body, html {
  padding: 0;
  margin: 0;
}

.c {
  height: 99px;
  overflow-y: auto;
  position: realtive;
  background: blue;
  color: white;
}

.d {
  height: 150%;
}

document.querySelector('.dpi').textContent = window.devicePixelRatio

const el = document.querySelector('.c')
const elInner = document.querySelector('.d')

el.scrollTop = 9999

document.querySelector('.st').textContent = el.scrollTop
document.querySelector('.br').textContent = -( elInner.getBoundingClientRect().y - el.getBoundingClientRect().y)

el.scrollTop = 0

Actual results:

The scrollTop was always rounded to integer 50 regardless of whether the container have a non integer height.

Expected results:

The scrollTop reports correct scroll position like 49.5 as the getBoundingClientRect() suggests.
Note: The chrome reports it correctly.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Test-case from reporter.

We use integers for scrollTop and such, but the spec made them unrestricted double at some point looks like... https://drafts.csswg.org/cssom-view/#extension-to-the-element-interface

Generally in favor of making these return doubles, see https://github.com/w3c/csswg-drafts/issues/5260.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=92de33ad986c9c52a2415525c6203f72453aec71

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1674687 - [cssom-view] Make scroll-related OM properties doubles. r=smaug

As per the latest draft:

https://drafts.csswg.org/cssom-view/#extension-to-the-element-interface

This matches Chrome. WebKit bug is:

https://bugs.webkit.org/show_bug.cgi?id=188045

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:emilio, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Yeah, there are some issues with WPT on android that I need to figure out. For some reason we're using fractional scaling there, which breaks some tests.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

So there are various android tests that still need work, but they seem just rounding errors: https://treeherder.mozilla.org/jobs?repo=try&revision=99bd1537d329eced1d231983c6a89aeb0462ab50

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Ah, there's also some errors on macOS which are bug 1674687.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

There's a concerns that landing this change without bug 1946610 may cause jitter scrolling or some such if the site implements its own scroll operation logic in JS.

That's being said, such things should have been there since scrollTo and scrollBy operations have already double precision.

Anyways, I am going to land this change right now to see what happens. Since it's an early state of the current nightly, if something bad happens we will back the change out.

Changing "Assignee" to me.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3dbdd49c22ca
[cssom-view] Make scroll-related OM properties doubles. r=mats,webidl,smaug

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a96008e0c883
Revert "Bug 1674687 - [cssom-view] Make scroll-related OM properties doubles. r=mats,webidl,smaug" for causing dt failures at browser_dbg-editor-scroll.js

Comment 12

[Cristina Horotan [:chorotan]]

Backed out for causing multiple failures

Backout link

Push with failures

Failure log browser_dbg-editor-scroll.js
Failure log test_wheel_default_action.html

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Filed bug 1968629 and bug 1968837 for the failures.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/95d3cbdbfee4
https://hg.mozilla.org/integration/autoland/rev/31dc424c0170
[cssom-view] Make scroll-related OM properties doubles. r=mats,webidl,smaug

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/cc5186e898c9
https://hg.mozilla.org/integration/autoland/rev/2c000fc5e0b9
Revert "Bug 1674687 - [cssom-view] Make scroll-related OM properties doubles. r=mats,webidl,smaug" for causing mochitests-plain failures in test_bug795785.html.

Comment 16

[Serban Stanca [:SerbanS]]

Reverted this because it was causing mochitests-plain failures in test_bug795785.html.

• Backout link
• Push with failures
• Failure Log

Also, there are some wpt failures.

• Failure Log

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/e23a083a9011
https://hg.mozilla.org/integration/autoland/rev/67a46b2f7eca
[cssom-view] Make scroll-related OM properties doubles. r=mats,webidl,smaug

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/6d17a21a18da
https://hg.mozilla.org/integration/autoland/rev/96bd92b2e4fb
Revert "Bug 1674687 - [cssom-view] Make scroll-related OM properties doubles. r=mats,webidl,smaug" for multiple mochitest failures related to scroll

Comment 19

[Narcis Beleuzu [:NarcisB]]

Backed out for multiple mochitest failures related to scroll

Backout link: https://hg-edge.mozilla.org/integration/autoland/rev/96bd92b2e4fb987ae5d7acdb636d00d65ab13f8d
Fail link: https://treeherder.mozilla.org/logviewer?job_id=512067787&repo=autoland&lineNumber=5136
Fail link2: https://treeherder.mozilla.org/logviewer?job_id=512067838&repo=autoland&lineNumber=2918
Fail link3: https://treeherder.mozilla.org/logviewer?job_id=512067778&repo=autoland&lineNumber=5120
Fail link4: https://treeherder.mozilla.org/logviewer?job_id=512069059&repo=autoland&lineNumber=2710
Fail link5: https://treeherder.mozilla.org/logviewer?job_id=512068019&repo=autoland&lineNumber=3647

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/49c7e68acc55
https://hg.mozilla.org/integration/autoland/rev/69b3dcd71954
[cssom-view] Make scroll-related OM properties doubles. r=mats,webidl,smaug

Comment 21

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/69b3dcd71954