1748891 - {inc} div gets inconsistent position in incremental vs. full layout with dynamic change to 'padding` on sticky-positioned element whose position wants to put it outside the scrollport

Status: RESOLVED FIXED

(Core :: Layout: Positioned, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.zip

Found while fuzzing m-c 20220105-138105a14659 (--enable-debug --enable-fuzzing)

To reproduce via Grizzly Replay:

$ pip install fuzzfetch grizzly-framework
$ python -m fuzzfetch -d --fuzzing -n firefox
$ python -m grizzly.replay ./firefox/firefox testcase.zip

LQC Signature: tag-div,x-extreme,left-extreme,right-extreme

LQC Results:
Conflicting dimensions for element two<div>
    Dimensions after reload: {"x":1371.683349609375,"left":1371.683349609375,"right":1424.683349609375}
    Dimensions after modify: {"x":4855.10009765625,"left":4855.10009765625,"right":4908.10009765625}
END of LQC Results
CSS Styles Used: {
    "base_styles": [
        "background-color",
        "border-right-style",
        "inset-inline-start",
        "min-height",
        "min-width",
        "overflow-x",
        "position",
        "transform",
        "width"
    ],
    "modified_styles": [
        "padding-block-start"
    ]
}

If a Pernosco session would be helpful please ni? :tsmith.

Comment 1

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Comment 2

[Bugmon [:jkratzer for issues]]

Bugmon Analysis
Unable to reproduce bug 1748891 using build mozilla-central 20220105170324-138105a14659. Without a baseline, bugmon is unable to analyze this bug.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Comment 3

[Daniel Holbert [:dholbert]]

I can reproduce. STR are
(1) Load testcase.html
(2) F12 to open console
(3) inspect the logged output

I see:

Dimensions after style changes, before reload
#two 
Object { x: 4855.10009765625, left: 4855.10009765625, right: 4908.10009765625 }

Dimensions after reload
#two 
Object { x: 1371.683349609375, left: 1371.683349609375, right: 1424.683349609375 }

(All three values are substantially larger in the "after style changes" part)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

Here's a further-reduced testcase, which I've also edited to be less-fully-automated, so you can see the rendering after the initial load + dynamic-style-change, and see how it changes when the DOM is reserialized.

In Chrome, when you load this testcase, the yellow-and-teal element is visible (and there's no rendering difference and no difference in the console.log output when you click the button)

In Firefox, when you first load this testcase, the scrollable area is fully pink. When you click the button, the yellow-and-teal element appears. (Also: scrolling is a bit horked -- if you load the testcase and try to scroll right by e.g. clicking the pink area and then pressing rightarrow on your keyboard, the scrollbar moves very slowly & jittery; it looks like we're continuously pushing the yellow and teal element out-of-view which is forever creating more scrollable distance.)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (hover and/or scroll to cause trouble with this testcase)

Here's a further-reduced testcase.

Side note, this is kinda related to the discussion on https://bugs.chromium.org/p/chromium/issues/detail?id=1327794

There's still a bit I don't understand about Chrome's behavior there, but one thing that came out of the discussion is that Chrome is computing scrollable overflow by assuming the sticky-positioned element is at its static position.

If we did that, I think that would fix this issue.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: WIP: Bug 1748891: WIP for sticky positioning LQC bug (exclude sticky position from scrollable overflow)

Comment 7

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

Attachment: Bug 1748891 Part 1 - Exclude sticky position border-box from scrollable overflow in an overflow area getter. r?dholbert,emilio

This helper is used in nsContainerFrame::ConsiderChildOverflow() and
nsLayoutUtils::UnionChildOverflow().

This patch is adapted from a WIP written by
Daniel Holbert <[e-mail]>
in https://phabricator.services.mozilla.com/D150427

Comment 8

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

Attachment: Bug 1748891 Part 2 - Stop applying sticky offset in ReflowInput::ApplyRelativePositioning(). r?dholbert,emilio

In an incremental reflow for scroll container (e.g. resizing scroll container),
toggling the visibility of vertical and horizontal scrollbars in TryLayout() can
affect the position of sticky elements. We'd want to reflow the scrolled frame
and update the sticky positions.

The WPTs are inspired from the reduced testcase in bug 1835705 comment 8. They
test block and grid containers. We'll fix flex containers in the next part.

Comment 9

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

Attachment: Bug 1748891 Part 3 - Exclude flex item's margin-box at sticky position in flex container's scrollable overflow area. r?dholbert,emilio

Comment 10

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

Attachment: Bug 1748891 Part 2 - Stop applying sticky offset in ReflowInput::ApplyRelativePositioning(). r?dholbert,emilio

A sticky element gets its offsets computed in
StickyScrollContainer::ComputeStickyOffsets(), and that is invoked in
UpdateSticky() after we reflow the scrolled frame in
nsHTMLScrollFrame::Reflow().

Therefore, in the initial reflow of the sticky element (where we haven't fully
reflow its scrolled frame container), ComputePosition() does nothing because
the elements doesn't get its offsets computed yet [2].

In an incremental reflow of a sticky element, the code can produce wrong sticky
position, because ComputePosition() and its helper ComputeStickyLimits() use
an obsoleted scrollable overflow of the scrolled frame [3] or an obsoleted
offset [4] from the last reflow. For example, in the testcases the sticky offset
has a percentage value and its basis is changed (e.g. scroll container's width
or height changed).

To summarize, computing sticky position prematurely leads to issues, and doesn't
always prevent unnecessary overflow updates, both in the initial reflow and some
incremental reflow cases. Thus, this patch removes it, and always let the scroll
container update the positions for all sticky elements.

[1] https://searchfox.org/mozilla-central/rev/6121b33709dd80979a6806ff59096a561e348ae8/layout/generic/nsGfxScrollFrame.cpp#1624
[2] To be precise, ComputedOffsetProperty() is not set, and will bail out in
StickyScrollContainer::ComputeStickyLimits().
[3] https://searchfox.org/mozilla-central/rev/6121b33709dd80979a6806ff59096a561e348ae8/layout/generic/StickyScrollContainer.cpp#198
[4] https://searchfox.org/mozilla-central/rev/6121b33709dd80979a6806ff59096a561e348ae8/layout/generic/StickyScrollContainer.cpp#161-162

Comment 11

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

Attachment: Bug 1748891 Part 3 - Exclude flex item's margin-box at sticky position in flex container's scrollable overflow area. r?dholbert,emilio

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b6ed0b385f41
Part 1 - Exclude sticky position border-box from scrollable overflow in an overflow area getter. r=emilio
https://hg.mozilla.org/integration/autoland/rev/8278ed498d02
Part 2 - Stop applying sticky offset in ReflowInput::ApplyRelativePositioning(). r=emilio
https://hg.mozilla.org/integration/autoland/rev/1f2bdb707f16
Part 3 - Exclude flex item's margin-box at sticky position in flex container's scrollable overflow area. r=emilio

Comment 13

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

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

Comment 14

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/b6ed0b385f41
https://hg.mozilla.org/mozilla-central/rev/8278ed498d02
https://hg.mozilla.org/mozilla-central/rev/1f2bdb707f16

Comment 15

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

Upstream PR merged by moz-wptsync-bot