Closed Bug 1742274 Opened 3 years ago Closed 2 years ago

scrollbar may not work correctly when page height change

Categories

(Core :: Panning and Zooming, defect, P3)

Firefox 96
defect

Tracking

()

RESOLVED DUPLICATE of bug 1753881

People

(Reporter: mix5003, Unassigned)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

Attached video 20211121_105637.mp4 (deleted) —

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:96.0) Gecko/20100101 Firefox/96.0

Steps to reproduce:

Open page has a lazy load contents and scroll to bottom then refresh.
scroll bar not show position correctly until mouse hover on scroll bar or interaction to that page

Actual results:

When refresh in lazy load page. and try to scroll to top scrollbar position not show correctly

Expected results:

it should show correctly

step to reporduce

  1. go to https://ishare.in.th/tmp/bug-1742274/scroll.html
  2. scroll to bottom (end of DIV 4)
  3. F5 to reload this page
  4. wait untill DIV4 show again
  5. use mouse wheel to scroll to top. you can see when you go to div1 scrollbar not on top position

this just annoying in sometime. and it easily to workaround fix.
something like just move mouse to scrollbar, or click or type some key can solve this

Component: Untriaged → Layout: Scrolling and Overflow
Product: Firefox → Core
Status: UNCONFIRMED → NEW
Ever confirmed: true
Has Regression Range: --- → yes
Has STR: --- → yes
Keywords: regression
Regressed by: 1657822, 1669625

Thanks for finding that Alice!

The severity field is not set for this bug.
:dholbert, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(dholbert)

Tentatively reclassifying under Panning and Zooming to match component of the #1-regression Bug 1657822. Hoping the APZ folks can further-triage (and/or reclassify to WebRender [per #2 regression] or elsewhere as-appropriate).

Component: Layout: Scrolling and Overflow → Panning and Zooming
Flags: needinfo?(dholbert)

Yep, this is on our radar though as a somewhat low-severity issue.

Severity: -- → S3
Priority: -- → P3

The page adds content to the bottom by removing display none in a setTimeout from the load event handler. When that setTimeout runs we reflow the scrollframe, and then call ReflowFinished for it. This calls ScrollToRestoredPosition, mRestorePos has not been cleared yet so we try to scroll to the position before reload by calling ScrollToImpl and succeed, the layout scroll position is now at the end of the page. Then ScrollToRestoredPosition calls ScrollToVisual to update the visual scroll position. This doesn't update the visual viewport offset (which is stored on the presshell) directly, instead if sets a pending visual scroll update on the presshell. This pending visual scroll update is sent to APZ later. Then ReflowFinished calls FinishReflowForScrollbar, it uses the value of ScrollFrameHelper::GetVisualViewportOffset for the position of the scrollbar. ScrollFrameHelper::GetVisualViewportOffset first checks if there is a pending visual scroll update and uses that if there is one. Then checks the presshell visual viewport offset, and then finally the layout scroll position. So we position the scrollbars at the pending visual scroll update value, but apz is expecting the scrollbars to be at the visual scroll offset aka the visual viewport offset. When we send the pending visual scroll update to APZ it then performs this visual scroll update so it thinks it has an async scroll offset, so it adjusts the scrollbars to the wrong position.

The obvious fix of positioning the scrollbars by looking only at the visual viewport offset and ignoring the pending visual scroll update does not work for the following reason: ReflowFinished calls CurPosAttributeChanged at the end, which calls ScrollToWithOrigin, which calls ScrollToImpl which clobbers the visual viewport offset. (This call is avoided when the scrollbars are at the pending visual scroll update position because that position matching the layout scroll position so ScrollToImpl early exits.)

https://searchfox.org/mozilla-central/rev/b70bc09685763c44a8e56e4e04cb741fa020701a/gfx/layers/apz/src/ScrollThumbUtils.cpp#92

This code async adjust the scrollbars assuming that content has put them at (last content paint metrics).GetVisualViewportOffset(). However, as outlined above, we put them at ScrollFrameHelper::GetVisualViewportOffset, which looks at any pending visual viewport offset. The pending visual viewport offset gets put into the visual destination. When there is no pending vv offset update then we just put the visual viewport offset in the visual destination. So just updating that code to visual destination instead of vv scroll offset fixes the bug. The full proper fix would also need to update the scrollbars whenever we get a new pending vv offset update. So I think that is the way to go right now, I'll write patches for that and see if they agree with try server.

as i think this bug may same root cause as Bug1742241. so after that bug fixed, this bug also fixed too.

(actually mozregression said this bug fixed by Bug 1753881, so i marked duplicate with Bug 1753881)

Bug 1753881. Simplify a few IsVisualViewportOffsetSet callers.  r=botond

Depends on D139472
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: