Closed Bug 1559627 Opened 5 months ago Closed 5 months ago

Scrolling is jumpy / hesitant on some webpages

Categories

(Core :: Layout: Scrolling and Overflow, defect)

69 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla69
Tracking Status
firefox-esr68 --- verified
firefox69 --- verified

People

(Reporter: jvgoransson, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [layout:triage-discuss][wptsync upstream])

Attachments

(2 files)

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

Steps to reproduce:

69.0a1 (2019-06-15) Nightly-comunity
When scrolling slowly i can't go below the top part of this site
https://www.tweaktown.com/reviews/index.html

Actual results:

When scrolling down fast i can scroll down to the rest of the site although with great jankiness with the webcontent jumping down the window a couple of times before it releases the top part and i can scroll down.

Expected results:

Smooth scrolling down of the web page.

Product: Firefox → Core
Component: Untriaged → Layout: Scrolling and Overflow

It works here on Nightly with and without WR enabled, any of the following would be helpful to further diagnose this:

  • Attach about:support.
  • Maybe attach a recording.
  • Maybe find a regression range?

Does this happen with add-ons disabled?

I can reproduce the issue on Nightly 69.0a1 Windows.

STR:
Wait page loading
Using down arrow key
Using Auto scrolling (middle click, and then move pointer downward(20-30px))

Actual results:
Page jumps to top

STR2:
Wait page loading
Turn mouse wheel by one notch

Actual results:
Scroll and half back

Disabled scroll anchoring fixes the issue.

Status: UNCONFIRMED → NEW
Ever confirmed: true

Ah indeed, maybe my trackpad is too fast, but scrolling a bit more slowly I can see that.

I think this code is supposed to handle that: https://searchfox.org/mozilla-central/rev/8c09b0389ff4e6263a3f10b1dbd2b6145d7a2aa7/layout/base/RestyleManager.cpp#1597

Probably it's not doing it somehow, I can put this on my queue... Though we have quite a few scroll-anchoring regressions that someone needs to go through and fix...

Flags: needinfo?(emilio)
Whiteboard: [layout:triage-discuss]

Joel,

I think I can reproduce your description and with Firefox 69.0a1 buildID=20190615214733. But please answer these questions:

  • how do you actually scroll? By dragging the vertical scrollbar thumb? By rolling mouse-wheel? By keyboard?
  • do you have smooth scrolling enabled? See next-to-last section in about:preferences#general or see general.smoothScroll property in about:config

Emilio,
<span class="stickymenu" style="position: static; top: 0px;">
seems to be repositioned or reset dynamically with a javascript function when the viewport height is small (say 300px or 400px tall). 'position' changes from 'static' to 'fixed' and then back to 'static' when I roll the mousewheel 1 notch at a time and then, indeed, I can not go below the top part of the page.
I have general.smoothScroll disabled .

I can not reproduce the problem when dragging down the vertical scrollbar thumb.
I can not reproduce the problem when typing [PgDn] key.
But I can reproduce the problem with Firefox 69.0a1 buildID=20190615214733 when rolling down several times 1 notch at a time the mouse-wheel and when viewport height is less than 400px (eg window.innerHeight is == 336px).
In about:config
general.smoothScroll.mouseWheel is set to true
mousewheel.min_line_scroll_amount is set to 5
I tried the same procedures with Firefox 60.7.0 ESR and I got expected results.

If I roll mousewheel 4 or 5 consecutive notches with Firefox 69.0a1 buildID=20190615214733, then I can reach beyond the header part of the webpage.

I can also reproduce the problem with Firefox 69.0a1 buildID=20190615214733 if I type in the navigation down key while I can not with Firefox 60.7.0 ESR.

No longer blocks: scroll-anchoring
Status: NEW → UNCONFIRMED
Ever confirmed: false
Whiteboard: [layout:triage-discuss]

I can reproduce the problem with Firefox 69 regardless of viewport height. Rolling the mousewheel one notch at a time or pressing the navigation down arrow key once at a time gives me actual result.

Was the status changes and such intentional? I assume not.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [layout:triage-discuss]
Attached file Reduced test-case.

So the issue is that the sticky span is an inline (and indeed the test-case works correctly if you add display: block to the .sticky rule).

The frame tree looks like:

Block(header)(0)@7f11a434ebe8 parent=7f11a434eae0 next=7f11a434f358 {0,0,76080,5310} [state=0000020000100200] [content=7f119f848f70] [cs=7f119f84b4c8]<
  line 7f11a434f218: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x108] {0,0,76080,2310} <
    Block(h1)(1)@7f11a434eca0 parent=7f11a434ebe8 next=7f11a434ee40 {0,0,76080,2310} [state=0000020000100200] [content=7f11a08d3ca0] [cs=7f119f84b798]<
      line 7f11a434edf0: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x100] {0,0,11070,2310} <
        Text(0)"Some title"@7f11a434ed58 parent=7f11a434eca0 {0,0,11070,2310} [state=00000000b0600000] [content=7f119f413c00] [cs=7f119f84bc48:-moz-text] [run=7f11ab1d2790][0,10,T] 
      >
    >
  >
  line 7f11a434f268: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x380] {0,2310,0,0} vis-overflow=0,1410,0,0 scr-overflow=0,1410,0,1140 <
    Inline(span)(3)@7f11a434ee40 parent=7f11a434ebe8 next=7f11a434f078 IBSplitSibling=7f11a434f078 {0,1410,0,1140} [state=0080000000008200] [content=7f11a44b7040] [cs=7f119f84b888]<>
  >
  line 7f11a434f2b8: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x108] {0,2310,76080,3000} <
    Block(span)(3)@7f11a434f078 parent=7f11a434ebe8 next=7f11a434f180 IBSplitSibling=7f11a434f180 IBSplitPrevSibling=7f11a434ee40 {0,2310,76080,3000} [state=0000020000108000] [content=7f11a44b7040] [cs=7f119f84be28:-moz-block-inside-inline-wrapper]<
      line 7f11a434f130: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x108] {0,0,76080,3000} <
        Block(div)(0)@7f11a434eed8 parent=7f11a434f078 {0,0,76080,3000} [state=0000028000100200] [content=7f11a66331f0] [cs=7f119f84b978]<
          line 7f11a434f028: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x100] {0,0,6690,1140} vis-overflow=0,0,6720,1140 scr-overflow=0,0,6690,1140 <
            Text(0)"Sticky header"@7f11a434ef90 parent=7f11a434eed8 {0,0,6690,1140} vis-overflow=0,0,6720,1140 [state=00000000b0600000] [content=7f119f413d80] [cs=7f119f84bd38:-moz-text] [run=7f1199cfa100][0,13,T] 
          >
        >
      >
    >
  >
  line 7f11a434f308: count=1 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x380] {0,5310,0,0} vis-overflow=0,4410,0,0 scr-overflow=0,4410,0,1140 <
    Inline(span)(3)@7f11a434f180 parent=7f11a434ebe8 IBSplitPrevSibling=7f11a434f078 {0,4410,0,1140} [state=0000000000008200] [content=7f11a44b7040] [cs=7f119f84b888]<>
  >
>

And when toggling the position on the <span> we reframe the whole containing block (the header) rather than just removing the span.

This means that scroll anchoring gets a 0x0 size when inspecting the old frame tree, and fails to select the <header> as an anchor. As a result we think the new anchor (the content) has moved and adjust it.

Ryan, do you have any idea about how this is supposed to work?

The fact that scroll anchoring looks at the state before reflow but after styling to select an anchor means that suddenly all the code that reframes too much to deal with frame tree rebuilds has side effects like this.

Maybe removing the scroll anchor frame should only move the anchor to the closest ancestor rather than forcing another anchor selection? That would mitigate this case at least, but seems unfortunate and still not quite correct... Maybe we should preserve a bit more state across the reframe instead, so that we can get back and set the new anchor if needed. So something like the pending scroll anchor selection also having an RefPtr<nsIContent> mOldAnchorNode;, or such. And if that content still had a frame afterwards, then we'd auto-select that frame as an anchor, even if it hasn't been reflowed yet...

Ryan, ideas?

Flags: needinfo?(emilio) → needinfo?(rhunt)

Oh, I see what's going on, the issue is that the suppression check in here:

Is not getting called at all, since when we're reframing the ancestor we return before constructing frames...

Assignee: nobody → emilio
Flags: needinfo?(rhunt)

Now on this page it does something different yet similar. When scrolling down it suddenly skips a couple of rows in the top part of the page before it continues to the rest of the page. When scrolling back up it's the same thing.
https://www.mister-auto.se/motorolja/bolk/bol-d091016/

That looks like a different bug. That one reproduces with scroll anchoring disabled, so mind filing it separately? It's not the same root cause.

I had filed bug 1531653 for the same issue, which got duped to bug 1526776

See Also: → 1531653

Emilio, have you tested this on any of the other scroll anchoring regressions to see if this resolves them? If not, I can.

Flags: needinfo?(emilio)

I have not, yet at least. If you could it'd be great, otherwise lmk and I'll do.

Flags: needinfo?(emilio)
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f64391685781
More reliably detect position changes during reframe. r=rhunt
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/17484 for changes under testing/web-platform/tests
Whiteboard: [layout:triage-discuss] → [layout:triage-discuss][wptsync upstream]
Upstream PR merged

(In reply to Emilio Cobos Álvarez (:emilio) from comment #13)

That looks like a different bug. That one reproduces with scroll anchoring disabled, so mind filing it separately? It's not the same root cause.

Sure will do!

And great work on getting the main issue of this tread fixed :D

Comment on attachment 9072456 [details]
Bug 1559627 - More reliably detect position changes during reframe. r=rhunt

Beta/Release Uplift Approval Request

  • User impact if declined: Scrolling gets stuck sometimes.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: comment 0
  • List of other uplifts needed: None
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): Not a trivial patch, but has landed for a while without known regressions.
  • String changes made/needed: none
Attachment #9072456 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Comment on attachment 9072456 [details]
Bug 1559627 - More reliably detect position changes during reframe. r=rhunt

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Scrolling gets stuck on some sites.
  • User impact if declined: Scrolling gets stuck on some sites.
  • Fix Landed on Version: 69
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): see above
  • String or UUID changes made by this patch: none
Attachment #9072456 - Flags: approval-mozilla-beta? → approval-mozilla-esr68?
QA Whiteboard: [qa-triaged]

Hello!
Using the test case from comment 7 I successfully reproduced the issue with Firefox 69.0a1 (20190615214733) on Windows 10x64.
The issue is verified fixed using Firefox 70.0a1 (20190725215157) and 69.0b8 (20190725174626) on Windows 10x64, Ubuntu 18.04 and macOS 10.14. The page is scrolled as expected using different scrolling methods.

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression

Comment on attachment 9072456 [details]
Bug 1559627 - More reliably detect position changes during reframe. r=rhunt

Fixes a scroll anchoring regression which has caused issues on various websites. Approved for 68.1esr.

Attachment #9072456 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

Hello,
The issue is verified fixed with Firefox 68.1.0esr (20190729203625) on Windows 10x64, macOS 10.14 and Ubuntu 18.04. Tested using STR from comment 0 and test case from comment 7.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.