Closed Bug 1546139 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla68
Tracking Status
firefox-esr60 --- unaffected
firefox66 --- unaffected
firefox67 --- verified
firefox68 --- verified

People

(Reporter: eeejay, Assigned: botond)

References

(Regression)

Details

(Keywords: regression)

Attachments

(6 files, 1 obsolete file)

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

Attached video container-scroll.mp4 (deleted) —
Attached video containerless-scroll.mp4 (deleted) —

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

Flags: needinfo?(eitan)
Attached file bottomfixed.html (deleted) —

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.

Flags: needinfo?(eitan)

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.

Blocks: 1516048

(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.

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.

(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.

Assignee: nobody → botond

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.

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

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.

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.

Attached file Bug 1546139 - Add a reftest. r=kats (deleted) —

Depends on D28728

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

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

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:
Attachment #9059945 - Flags: approval-mozilla-beta?
Flags: qe-verify+

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.

Attachment #9059945 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Pushed by bballo@mozilla.com: 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
Attachment #9059945 - Attachment is obsolete: true

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: