1901652 - The hypothetical box of fixed positioned elements accounted the root scrollbar-gutter on the left twice

Status: RESOLVED FIXED

(Core :: Layout: Positioned, defect, P3)

Description

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: Testcase 1

This bug was somewhat exposed after bug 728807. With scrollbar-gutter: stable both-edges set on the root element, the hypothetical box position of fixed-pos elements accounts the scrollbar-gutter twice.

I think we need to change this part of the code https://searchfox.org/mozilla-central/rev/46d0387f0b582f00a5722c20d4e6b8693793631b/layout/generic/ReflowInput.cpp#1422-1426. When aCBReflowInput->mFrame is root frame, we need to get the scrollbar size and tweak cbOffset, similar to https://searchfox.org/mozilla-central/rev/46d0387f0b582f00a5722c20d4e6b8693793631b/layout/style/nsComputedDOMStyle.cpp#2009-2019

Comment 1

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

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

Comment 2

[Emilio Cobos Álvarez [:emilio]]

See also https://github.com/w3c/csswg-drafts/issues/9904 (only tangentially related tho)

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: Bug 1901652 Part 1 - Clean up for CalculateHypotheticalPosition. r?#layout

Remove unused nsPresContext and LayoutFrameType parameters in
InitAbsoluteConstraints() and CalculateHypotheticalPosition() (except for
one usage in NS_ASSERTION).

Improve the documentation for CalculateHypotheticalPosition() and delete the
nearly the same documentation before its definition in ReflowInput.cpp.

In InitAbsoluteConstraints(), we don't need to initialize the offsets local
variable via ComputedLogicalOffsets() since this is the function computing the
offset, and we'll set the four sides of offsets later in the function.

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: Bug 1901652 Part 2 - Implement GetScrollTargetFrame() for ViewportFrame. r?#layout

nsIFrame provides GetScrollTargetFrame() for getting the scroll container
child under a frame. We can implement this for ViewportFrame so that callers
don't need to get the first child and then downcast.

Comment 5

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: Bug 1901652 Part 2 - Fix hypothetical position for fixed-pos elements when scrollbar-gutters are on both edges. r?#layout

We have similar adjustments in ViewportFrame::AdjustReflowInputForScrollbars()
and nsComputedDOMStyle::GetUsedAbsoluteOffset.

Comment 6

[Liz Henry (:lizzard)]

Because of the severity and priority rating, I'm marking this fix-optional for current releases, since the team is aware and has rated the issue.
While we are happy to take a patch, and uplift if it's requested, we don't need to keep looking at the bug in weekly regression triage.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a7a4eb34e7b1
Part 1 - Clean up for CalculateHypotheticalPosition. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/8fc6e70268be
Part 2 - Implement GetScrollTargetFrame() for ViewportFrame. r=layout-reviewers,emilio
https://hg.mozilla.org/integration/autoland/rev/24bb7e89929c
Part 3 - Fix hypothetical position for fixed-pos elements when scrollbar-gutters are on both edges. r=layout-reviewers,emilio

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/46779 for changes under testing/web-platform/tests

Comment 9

[Sandor Molnar[:smolnar]]

Backed out for causing mochitest failures @ browser_scrollToPoint.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/23289ab8985a932c991cf504742a254dd95436b5

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | accessible/tests/browser/scroll/browser_scrollToPoint.js

Comment 10

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

Upstream PR was closed without merging

Comment 11

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Removed Bug 1901652 Part 2 - Implement GetScrollTargetFrame() for ViewportFrame (the original Part 2) that causes the test failures.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/971fe2822596
Part 1 - Clean up for CalculateHypotheticalPosition. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/66efe1aeeb01
Part 2 - Fix hypothetical position for fixed-pos elements when scrollbar-gutters are on both edges. r=layout-reviewers,emilio

Comment 13

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/971fe2822596
https://hg.mozilla.org/mozilla-central/rev/66efe1aeeb01

Comment 14

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

Upstream PR merged by moz-wptsync-bot

Comment 15

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

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

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

For more information, please visit BugBot documentation.

Comment 16

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

I'll let this ride the trains because scrollbar-gutter: stable both-edges is rare.