1831148 - getBoundingClientRect().top returns a constant value after scrollTop is too high

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Julien Wajsberg [:julienw]]

STR:

1. Open the attachment.
2. Scroll down the container.
3. Observe that the scrolltop always increases but getBoundingClientRect().top always returns a constant value at one point.

This does work in Chrome.
Note: we use this in the profiler for our virtual lists (but will probably move to scrollTop because of this issue), and we found this when looking at the marker table in this profile.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Missing attachment?

Comment 2

[Julien Wajsberg [:julienw]]

Attachment: bounding.html

🤦

Comment 3

[Julien Wajsberg [:julienw]]

(In reply to Julien Wajsberg [:julienw] from comment #0)

STR:

1. Open the attachment.
2. Scroll down the container.
3. Observe that the scrolltop always increases but getBoundingClientRect().top always returns a constant value at one point.

This does work in Chrome.
Note: we use this in the profiler for our virtual lists (but will probably move to scrollTop because of this issue), and we found this when looking at the marker table in this profile.

At 2., you'll need to scroll using the scrollbar, because you need to scroll below approximately half the height.

Comment 4

[David Shin[:dshin]]

Call tree down to:

#0  nsLayoutUtils::TransformFrameRectToAncestor (aFrame=0x7f6df13af380, aRect=..., aAncestor=..., aPreservesAxisAlignedRectangles=0x0, aMatrixCache=0x0, aStopAtStackingContextAndDisplayPortAndOOFFrame=false, aOutAncestor=0x0)
    at /home/dshin/mozilla-unified/layout/base/nsLayoutUtils.cpp:2541
#1  0x00007f6e03b59638 in nsLayoutUtils::TransformFrameRectToAncestor (aFrame=0x7f6df13af380, aRect=..., aAncestor=0x7f6df13ae020, aPreservesAxisAlignedRectangles=0x0, aMatrixCache=0x0, aStopAtStackingContextAndDisplayPortAndOOFFrame=false, aOutAncestor=0x0)
    at /home/dshin/mozilla-unified/layout/base/nsLayoutUtils.h:890
#2  0x00007f6e08f1c387 in BoxToRect::AddBox (this=0x7ffeb249eae0, aFrame=0x7f6df13af380) at /home/dshin/mozilla-unified/layout/base/nsLayoutUtils.cpp:3671
#3  0x00007f6e08ed8189 in nsLayoutUtils::AddBoxesForFrame (aFrame=0x7f6df13af380, aCallback=0x7ffeb249eae0) at /home/dshin/mozilla-unified/layout/base/nsLayoutUtils.cpp:3569
#4  0x00007f6e08ed81e0 in nsLayoutUtils::GetAllInFlowBoxes (aFrame=0x7f6df13af380, aCallback=0x7ffeb249eae0) at /home/dshin/mozilla-unified/layout/base/nsLayoutUtils.cpp:3577
#5  0x00007f6e08ed853b in nsLayoutUtils::GetAllInFlowRects (aFrame=0x7f6df13af380, aRelativeTo=0x7f6df13ae020, aCallback=0x7ffeb249eb58, aFlags=1) at /home/dshin/mozilla-unified/layout/base/nsLayoutUtils.cpp:3734
#6  0x00007f6e08ed884f in nsLayoutUtils::GetAllInFlowRectsUnion (aFrame=0x7f6df13af380, aRelativeTo=0x7f6df13ae020, aFlags=1) at /home/dshin/mozilla-unified/layout/base/nsLayoutUtils.cpp:3774
#7  0x00007f6e09072fdd in nsIFrame::GetBoundingClientRect (this=0x7f6df13af380) at /home/dshin/mozilla-unified/layout/generic/nsIFrame.cpp:7757

Then the value gets clamped here.

Comment 5

[David Shin[:dshin]]

Attachment: Bug 1831148: Increase clamping range for `TransformGfxRectToAncestor`. r=#layout-reviewers

The clamping is overaly pessimistic because the returned Rect is still
float-based, and the call site does its own clamping to nscoord.
Clamping to float's numeric limit causes significant floating point errors
when clipping the transformed rect, resulting in incorrect transforms.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1673f98a5005
Increase clamping range for `TransformGfxRectToAncestor`. r=emilio

Comment 7

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/1673f98a5005

Comment 8

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

Reproducible on a 2023-05-15 Nightly build on Windows 10 using the attachment from Comment 2.
Verified as fixed on Firefox 115.0b5(build ID: 20230613195413) and Nightly 116.0a1(build ID: 20230615094111) on Windows 10, macOS 12, Ubuntu 22.

Comment 9

[David Shin[:dshin]]

bug 1839550 demonstrates that we effectively regressed on bug 1419225.
Re-opening for further work once the backout in bug 1839550 goes through.

Comment 10

[David Shin[:dshin]]

Attachment: Regressing case from bug 1839550

Comment 11

[Mathew Hodson]

I believe this was backed out of Firefox 115 and 116 in bug 1839550

Comment 12

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1831148. Allow nsLayoutUtils::TransformFrameRectToAncestor to use full nscoord range. r?#gfx-reviewers,#layout-reviewers

The problem with the previous patch was that a rect spanning (-reallybig,reallybig) would get reduced to fit in an nscoord based (x,y,width,height) rect by clamping each of x, y, width, height individually, which would result in the clamped rect having origin nscoord_min and size nscoord_max so that it would span (nscoord_min,0), which means it does not intersect the positive axis at all, which in most cases is the only part of the coord system that is visible.

We already have a solution for this exact problem in the code base. nsLayoutUtils::RoundGfxRectToAppRect uses ConstrainToCoordValues which reduces the size of the rect equally on both ends so the above rect would span (nscoord_min/2,nscoord_max/2) and have origin nscoord_min/2 and size nscoord_max. However we can't use RoundGfxRectToAppRect here because it has different rounding behaviour (and we have tests that depend on it, so I assume web sites will too). So we create a new function that is like RoundGfxRectToAppRect but has the rounding behaviour that we have in the existing function here

I re-used David's existing test from the previous patch.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/75370b03ee98
https://hg.mozilla.org/integration/autoland/rev/4500ec883294
Allow nsLayoutUtils::TransformFrameRectToAncestor to use full nscoord range. r=gfx-reviewers,lsalzman

Comment 14

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/4500ec883294