1931490 - Wrong scroll height being reported in some cases

Status: VERIFIED FIXED

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

Description

[matej.mulej]

Attachment: scrollHeightRepro.html

Steps to reproduce:

The scroll height of an element is wrong in some cases.

I've attached a repro file that will fire an alert if the bug is triggered.
Firefox 130 did worked correctly, the bug was introduced in Firefox 131.

Neither Chrome or Safari fire the alert.

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'Core::Layout: Scrolling and Overflow' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[matej.mulej]

Attachment: scrollHeightRepro-2.html

The new repro also shows errors in the latest default branch on mozilla-unified (commit 06a65f0d5e9d).

Comment 3

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

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

For more information, please visit BugBot documentation.

Comment 4

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

:emilio, since you are the author of the regressor, bug 1906475, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1931490 - Fix scroll{Width,Height} of overflow: visible frames with contain: layout. r=#layout

contain: layout usually makes our kids not account for scrollable
overflow, but when computing scroll{Width,Height} we need to (as the
inner scrolled frame won't use containment).

Add a test for contain: layout + border / padding + various display
types.

After this patch, we only fail the grid ones, due to bug 1797305.

Other browsers also fail a variety of subtests here.

Comment 6

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

Set release status flags based on info from the regressing bug 1906475

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7947178ec5dd
Fix scroll{Width,Height} of overflow: visible frames with contain: layout. r=dshin

Comment 8

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/49522 for changes under testing/web-platform/tests

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df07eacc2512
Add a viewport meta to prevent rounding errors on android.
https://hg.mozilla.org/integration/autoland/rev/71557120f80e
Fix a test to match the spec.

Comment 10

[amarc]

https://hg.mozilla.org/mozilla-central/rev/7947178ec5dd
https://hg.mozilla.org/mozilla-central/rev/df07eacc2512
https://hg.mozilla.org/mozilla-central/rev/71557120f80e

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 12

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

The patch landed in nightly and beta is affected.
:emilio, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox134 to wontfix.

For more information, please visit BugBot documentation.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Probably worth letting it ride the trains. Changes to these apis are always tricky.

Comment 14

[Jan de Mooij [:jandem]]

This change regressed the matrix-react benchmark quite a lot, about 17%. Is that kind of change expected?

This benchmark has a very primitive Element-like interface and it then switches between channels many times.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

No, that's pretty unexpected, though of course it depends on what the page is doing with the scroll{Width,Height} value. Do you know where the source of that benchmark is?

Comment 16

[Jan de Mooij [:jandem]]

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

Do you know where the source of that benchmark is?

The original source is here: https://github.com/mozilla-spidermonkey/matrix-react-bench

Building it now I get a Node error though, so maybe this is easier (and matches our CI version better):

1. Clone https://github.com/mozilla/perf-automation
2. cd benchmarks/matrix-react-bench
3. python3 -m http.server
4. Open http://localhost:8000/matrix_demo.html

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Ok, diagnostic and test-case in bug 1936156. This is an artifact of the page being completely unstyled fwiw, but that causes the react code to get confused and think stuff is scrollable when it shouldn't. Still, we should fix it because the expectation of client == scrollWidth/Height if there's no overflow is legit.

Comment 18

[Jan de Mooij [:jandem]]

That was fast. Thank you!

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Given that this also fixed bug 1935927, we should probably reconsider this for 134.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9441639 [details]
Bug 1931490 - Fix scroll{Width,Height} of overflow: visible frames with contain: layout. r=#layout

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Fixes regression from padding work.
• 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: see regression, will send a request for that too
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Relatively specific codepath
• String changes made/needed: none
• Is Android affected?: Yes

Comment 21

[Ciprian Georgiu, Desktop QA]

I have reproduced this issue with the test case from comment 2, on an affected Nightly 135.0a1 (2024-11-15) running Win 11.

The issue is verified as fixed on latest Nightly 135.0a1 under macOS 14, Win 11 and Ubuntu 24.04.

Comment 22

[Pascal Chevrel:pascalc]

Comment on attachment 9441639 [details]
Bug 1931490 - Fix scroll{Width,Height} of overflow: visible frames with contain: layout. r=#layout

Approved for 134RC, thanks.

Comment 23

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/7e065532aab5
https://hg.mozilla.org/releases/mozilla-beta/rev/90c510751279
https://hg.mozilla.org/releases/mozilla-beta/rev/94f120b11e14

Comment 24

[Ciprian Georgiu, Desktop QA]

This is also verified as fixed on 134 RC (treeherder build) using Win 11, macOS 14 and Ubuntu 24.04.