1627362 - Fix sticky handling more with dynamic toolbar

Status: RESOLVED FIXED

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

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: dynamic-toolbar-sticky-bottom-1.html

I was doing some additional manual testing with the position:sticky support in bug 1610731 and found at least one bug with sticky positioned items. It happens primarily with WebRender enabled but also manifests transiently with WebRender disabled.

STR:

• load the attached page in Fenix (I was using the emulator) with the dynamic toolbar at the bottom.
• Scroll down to where the text "top of next div" is visible.
• Scroll up and down with that text on-screen to show and hide the dynamic toolbar.

Expected:

• the red sticky div never covers up the "top of next div" text

Actual:

• the red sticky div can end up getting shifted down to cover the "top of next div" text.

I think this might be because the red div is at outside its sticky range, and so shouldn't behave "sticky" any more. But we apply the compositor transform to it anyway

Comment 1

[Botond Ballo [:botond]]

Ah! We have a check for "currently stuck" that we probably need to use in some other codepaths, including the WR one.

Comment 2

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #1)

Ah! We have a check for "currently stuck" that we probably need to use in some other codepaths, including the WR one.

And the issue in bug 1627326 may well be that that check only handles bottom and not top.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1627362 - Remove the tree lock requirement for a couple of functions. r?botond

We can use the map lock to do a lookup in mApzcMap, instead of requiring the
tree lock to call GetTargetAPZC.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1627362 - Have (Fixed|Sticky)PositionInfo take a HTTN in the constructor. r?botond

This will make future patches simpler, as we can now create these info objects
more easily for the non-WR codepath as well.

Depends on D70906

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1627362 - Allow calling fixed/sticky helpers with (Fixed|Sticky)PositionInfo. r?botond

This sets us up to be able to use these helper methods on WR sampling codepath.

Depends on D70907

Comment 6

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=1b6a6d10f3f5b404b88eff10dad0101e1a6cafd5

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1627362 - Rewrite IsStuckAtBottom to be simpler and clearer. r?botond

I couldn't understand what it was doing before, but conceptually it should
be pretty simple.

Depends on D70908

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1627362 - Properly support items stuck to both bottom and top. r?botond

The semantics of sticky items are somewhat different from the semantics of
fixed items. For fixed items, if an item is fixed to eTop or eBottom or
eTopBottom, it is always fixed to those sides. For sticky items, however,
the sides actively stuck to are dependent on the scroll position. So we need
a mechanism to dynamically figure out which sides are stuck, and use those
sides when computing the fixed margins to apply. This patch implements that
by modifying the IsStuckToRootContentAtBottom method into a
SidesStuckToRootContent method.

Depends on D70909

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1627362 - Fix WR codepath. r?botond

This patch just ensures the changes in the previous patches get applied
to the WR codepath, and is sufficient to make all the remaining sticky
tests pass on Android+WR.

Depends on D70910

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1627362 - Improve logging for fixed/sticky data on the WR codepath. r?botond

Depends on D70911

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b6821fad28e5
Remove the tree lock requirement for a couple of functions. r=botond
https://hg.mozilla.org/integration/autoland/rev/882b9adeff90
Have (Fixed|Sticky)PositionInfo take a HTTN in the constructor. r=botond
https://hg.mozilla.org/integration/autoland/rev/35144abc5266
Allow calling fixed/sticky helpers with (Fixed|Sticky)PositionInfo. r=botond
https://hg.mozilla.org/integration/autoland/rev/ec1127afc8ad
Rewrite IsStuckAtBottom to be simpler and clearer. r=botond
https://hg.mozilla.org/integration/autoland/rev/2348d884d386
Properly support items stuck to both bottom and top. r=botond
https://hg.mozilla.org/integration/autoland/rev/5bbfa16aa52d
Fix WR codepath. r=botond
https://hg.mozilla.org/integration/autoland/rev/ba223c9eb4fd
Improve logging for fixed/sticky data on the WR codepath. r=botond

Comment 12

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/b6821fad28e5
https://hg.mozilla.org/mozilla-central/rev/882b9adeff90
https://hg.mozilla.org/mozilla-central/rev/35144abc5266
https://hg.mozilla.org/mozilla-central/rev/ec1127afc8ad
https://hg.mozilla.org/mozilla-central/rev/2348d884d386
https://hg.mozilla.org/mozilla-central/rev/5bbfa16aa52d
https://hg.mozilla.org/mozilla-central/rev/ba223c9eb4fd

Comment 13

[Sorina Florean [:sflorean]]

Tested on the latest Fenix Nightly build (78.0a1-5/12) following the steps from the description and the issue is still reproducible (the red sticky div is covering the "top of next div" text) if the toolbar is not displayed. Video: https://drive.google.com/open?id=1BH5rHaeuHa-XA_VKfmqAXRl6urNcB_i2.

Please note that this is reproducible only when scrolling slowly up with the toolbar hidden.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

I see that as well, but it's actually a separate issue from what I meant in comment 0. In your video it's the "transition point" that's glitchy but in comment 0 I was referring to jiggling up and down rapidly while the "top of next div" was already near the middle of the screen. The thing in your video should probably be filed as another bug, but it's more of an edge case.