1546139 - Bottom fixed margin layers don't stick to bottom with dynamic toolbar in fennec

Status: VERIFIED FIXED

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

Description

[Eitan Isaacson [:eeejay]]

This seems to be a bug with containerless scrolling see videos.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: container-scroll.mp4

Comment 2

[Eitan Isaacson [:eeejay]]

Attachment: containerless-scroll.mp4

Comment 3

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

Do you have more concrete STR? Like what product are you using, and what's the URL?

Comment 4

[Eitan Isaacson [:eeejay]]

Attachment: bottomfixed.html

tl;dr

AsyncCompositionManager::SetFixedLayerMargins doesn't transform the bottom margin when containerless scrolling is enabled.

STR

1. Load bottomfixed.html in Fennec nightly.
2. Observe the "Fixed to bottom" banner.
3. Start panning down until the Fennec toolbar begins to go away.

Result

Fixed banner goes away, and shows up again when the the toolbar is completely hidden. See containerless-scroll.mp4.

Expected

Fixed banner should remain fixed to the bottom of the visible viewport. See container-scroll.mp4.

Comment 5

[Botond Ballo [:botond]]

I think I broke this in bug 1525181. Prior to that, the fixed layer margins were handled here, but bug 1525181 removed that code, apparently without providing a replacement for the fixed layer margins handling.

Comment 6

[Botond Ballo [:botond]]

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

I think I broke this in bug 1525181.

No, looks like it was broken even before that. I just checked and 67 (which does not have bug 1525181) still exhibits the bug.

Comment 7

[Eitan Isaacson [:eeejay]]

Where I see the code paths diverge is that in containerless mode AsyncTransformShouldBeUnapplied never returns true, and thus AsyncCompositionManager::AdjustFixedOrStickyLayer is never called. It does a lot more transforming there besides the fixed bottom margin, so i wasn't sure if that was intentional or not.

Comment 8

[Botond Ballo [:botond]]

(In reply to Eitan Isaacson [:eeejay] from comment #7)

Where I see the code paths diverge is that in containerless mode AsyncTransformShouldBeUnapplied never returns true, and thus AsyncCompositionManager::AdjustFixedOrStickyLayer is never called. It does a lot more transforming there besides the fixed bottom margin, so i wasn't sure if that was intentional or not.

This call site of AdjustFixedOrStickyLayer not being reached with containerless scrolling is intentional.

However, prior to bug 1525181, there was another call site which should have handled it. It turns out it didn't, though, because we weren't hitting the code that picks up mFixedLayerMargins for that call site. This affects 67, and should be straightforward to fix.

For 68, we'll need an additional fix, to remedy the fact that this second call site was removed altogether.

Comment 9

[Botond Ballo [:botond]]

Attachment: Bug 1546139 - Pick up fixed layer margins in AsyncCompositionManager with containerless scrolling. r=kats

This is a fix for the 67 branch only. 68 will require a different fix due to
bug 1525181 having changed this code since then.

Comment 10

[Botond Ballo [:botond]]

^ For now, here is the 67-only fix.

Comment 11

[Botond Ballo [:botond]]

I felt like this was the sort of regression that should have been caught by an automated test, so I set out to write one. After many attempts, I think I finally have something working.

The test requires some new test infrastructure (new reftest attributes), so I don't plan on uplifting it to 67. However, in order to gain confidence in my 67-only fix, I did push the test and fix to Try, on top of a backout of bug 1525181, to approximate the state of things on the 67 branch. (I didn't actually rebase the test onto the 67 branch, because the reftest attribute implementation would need rebasing across document splitting.)

Here is a Try push showing my test failing without the fix:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=4af39b013f3eceed1f55e7b71253e837487010c8

and passing with the fix:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=b53c688e77ceda271198c4d4472c2a8d36255d10

I'll clean up and post the test patches for review tomorrow.

Comment 12

[Botond Ballo [:botond]]

Attachment: Bug 1546139 - Add support for setting fixed layer margins in a reftest. r=kats

The persistence aspect of this (that is, how long the fixed margins stick
around after being set) is a bit hacky.

For reftest-async-scroll and reftest-async-zoom, the comments above
nsIDOMWindowUtils.setAsync{ScrollOffset,Zoom} suggest that these only take
effect for a single composite. However, as far as I can tell that's a lie;
what actually happens is they're set on the AsyncPanZoomController object,
and stick around until it goes away (usually on page navigation, such as from
the test page in a reftest to the ref page).

(It's not clear that taking effect for a single composite only would be the
desirable behaviour. In my experiments, we can sometimes get multiple
composites between a call to one of these functions, and the composite that
produces the reftest snapshot.)

For the fixed layer margins, they're stored in AsyncCompositionManager, which
unlike AsyncPanZoomController doesn't go away after a page load. Therefore, we
currently require tests to reset it manually, such as by a reftests's ref page
setting zero margins after the test page set nonzero ones. This is suboptimal
and could use improvement in the future.

Comment 13

[Botond Ballo [:botond]]

Attachment: Bug 1546139 - Add a reftest. r=kats

Depends on D28728

Comment 14

[Botond Ballo [:botond]]

Attachment: Bug 1546139 - Restore the call to AdjustFixedOrStickyLayer() for layers fixed to the RCD-RSF. r=kats

This call served two purposes: (1) scroll the fixed layer by the eVisual
transform, and (2) adjust it by the fixed margins. The first purpose is now
served by applying the eVisual transform to the async zoom container, but
we still need the call for the second purpose.

Depends on D28729

Comment 15

[Botond Ballo [:botond]]

D28728 and D28729 are the test patches. D28735 is the 68 fix.

Comment 16

[Botond Ballo [:botond]]

Here is a green Try push of the 67 fix on the 67 branch:

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=8ca4660aff1be7a672c8faa14afa2de3163cfa4f

Comment 17

[Botond Ballo [:botond]]

Comment on attachment 9059945 [details]
Bug 1546139 - Pick up fixed layer margins in AsyncCompositionManager with containerless scrolling. r=kats

Beta/Release Uplift Approval Request

• User impact if declined: When using Fennec and browsing pages with elements fixed to the bottom of the screen, the fixed elements will flicker when the toolbar transitions on- and offscreen.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Please see comment 4
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Some notes on this uplift request:
  • This is a 67-only fix. 68 requires a different fix (currently under review) due to the code having changed since then.
  • The fix does have an automated test. The test is not targeting 67 because it relies on new test infrastructure, but I have used it to gain confidence in the correctness of the 67 fix (see comment 11).
  • I am requesting uplift of the 67 fix before the 68 fix lands because we are late in the 67 cycle.
  • I am characterizing this as a low-risk fix even though we are late in the cycle, because I feel the problem is well-understood and the fix is targeted.
• String changes made/needed:

Comment 18

[Pascal Chevrel:pascalc]

Comment on attachment 9059945 [details]
Bug 1546139 - Pick up fixed layer margins in AsyncCompositionManager with containerless scrolling. r=kats

Minimal patch and problem well understood, let's take it for 67 beta 15, thanks.

Comment 19

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d41af59923c9

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d441eb4ab601
Add support for setting fixed layer margins in a reftest. r=kats
https://hg.mozilla.org/integration/autoland/rev/11a838d3eab4
Add a reftest. r=kats
https://hg.mozilla.org/integration/autoland/rev/4f70b98aa870
Restore the call to AdjustFixedOrStickyLayer() for layers fixed to the RCD-RSF. r=kats

Comment 21

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/d441eb4ab601
https://hg.mozilla.org/mozilla-central/rev/11a838d3eab4
https://hg.mozilla.org/mozilla-central/rev/4f70b98aa870

Comment 22

[Laurentiu Apahidean]

Hello,

I performed the test case posted in Comment 4 on a Samsung Galaxy S9 (Android 8.0.0) and Samsung Galaxy Tab A6 (Android 7.0).
On Beta 67.0b15 and Nightly 68.0a1 (2019-05-01) the fixed banner remains fixed to the bottom when scrolling.

Due to my findings, I'll mark this issue as verified.