Bottom fixed margin layers don't stick to bottom with dynamic toolbar in fennec
Categories
(Core :: Panning and Zooming, defect, P2)
Tracking
()
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)
(deleted),
video/mp4
|
Details | |
(deleted),
video/mp4
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
Bug 1546139 - Restore the call to AdjustFixedOrStickyLayer() for layers fixed to the RCD-RSF. r=kats
(deleted),
text/x-phabricator-request
|
Details |
This seems to be a bug with containerless scrolling see videos.
Reporter | ||
Comment 1•6 years ago
|
||
Reporter | ||
Comment 2•6 years ago
|
||
Comment 3•6 years ago
|
||
Do you have more concrete STR? Like what product are you using, and what's the URL?
Reporter | ||
Comment 4•6 years ago
|
||
tl;dr
AsyncCompositionManager::SetFixedLayerMargins
doesn't transform the bottom margin when containerless scrolling is enabled.
STR
- Load bottomfixed.html in Fennec nightly.
- Observe the "Fixed to bottom" banner.
- 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
.
Assignee | ||
Comment 5•6 years ago
|
||
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.
Assignee | ||
Comment 6•6 years ago
|
||
(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.
Reporter | ||
Comment 7•6 years ago
|
||
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.
Assignee | ||
Comment 8•6 years ago
|
||
(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 thusAsyncCompositionManager::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 | ||
Comment 9•6 years ago
|
||
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.
Assignee | ||
Comment 10•6 years ago
|
||
^ For now, here is the 67-only fix.
Assignee | ||
Comment 11•6 years ago
|
||
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:
and passing with the fix:
I'll clean up and post the test patches for review tomorrow.
Updated•6 years ago
|
Assignee | ||
Comment 12•6 years ago
|
||
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.
Assignee | ||
Comment 13•6 years ago
|
||
Depends on D28728
Assignee | ||
Comment 14•6 years ago
|
||
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
Assignee | ||
Comment 15•6 years ago
|
||
Assignee | ||
Comment 16•6 years ago
|
||
Here is a green Try push of the 67 fix on the 67 branch:
Assignee | ||
Comment 17•6 years ago
|
||
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:
Assignee | ||
Updated•6 years ago
|
Comment 18•6 years ago
|
||
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•6 years ago
|
||
bugherder uplift |
Comment 20•6 years ago
|
||
Updated•6 years ago
|
Comment 21•6 years ago
|
||
bugherder |
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•6 years ago
|
||
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.
Updated•6 years ago
|
Updated•5 years ago
|
Updated•3 years ago
|
Description
•