Closed Bug 1772859 Opened 4 months ago Closed 1 month ago

element positioned:fixed on bottom may disappear

Categories

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

defect

Tracking

()

RESOLVED FIXED
105 Branch
Tracking Status
firefox105 --- fixed

People

(Reporter: dlrobertson, Assigned: dlrobertson)

References

Details

Attachments

(2 files)

Steps to reproduce:

  • on a android device with the toolbar set at the bottom
  • navigate to a page with a position: fixed element that is smaller than the height of the bottom bar (See the example)
  • scroll down to collapse the bottom toolbar
  • click the element several times

Expected results:

  • The button still exists

Actual Results:

  • The button may disappear.
Attached file position-fixed.html

Thanks for filing! This looks pretty bad, potentially a candidate for one of our upcoming Android sprints.

Severity: -- → S3
Priority: -- → P2

Did a little debugging on this today and after a first pass, it appears the bottom element in the testcase doesn't exist in mFixedPositionInfo, so SampleForWebRender will not adjust the fixed content to a visible position.

(In reply to Dan Robertson (:dlrobertson) from comment #3)

Did a little debugging on this today and after a first pass, it appears the bottom element in the testcase doesn't exist in mFixedPositionInfo, so SampleForWebRender will not adjust the fixed content to a visible position.

After some more debugging, this seems to occur because the element fixed to the bottom doesn't end up making it into the display list, and only happens when the toolbar is completely hidden. This makes me wonder if there is a spot in the building of the display list where the visual viewport size is not the result of GetVisualViewportSizeUpdatedByDynamicToolbar().

(In reply to Dan Robertson (:dlrobertson) from comment #4)

After some more debugging, this seems to occur because the element fixed to the bottom doesn't end up making it into the display list, and only happens when the toolbar is completely hidden. This makes me wonder if there is a spot in the building of the display list where the visual viewport size is not the result of GetVisualViewportSizeUpdatedByDynamicToolbar().

Interesting finding! Display list building skipping the fixed element because the visible/dirty rect is not sized to include the dynamic toolbar does sound like a plausible explanation.

I think a relevant place to look further is the DescendIntoChild() call in BuildDisplayListForChild() (and the similar call in BuildDisplayListForSimpleChild()), to see if we fail to descend into the fixed element as a result of this check. The implementation of this function checks if the frame in question intersects the visible and dirty rects here.

Since this bug is about a position fixed item one thing that may be important is how we handle he dirty rects for out of flows like fixed pos.

When we enter the geometric parent frame during display list building we call MarkAbsoluteFramesForDisplayList

https://searchfox.org/mozilla-central/rev/284187d0a7130d21042beeff0af0627c8e68cacc/layout/generic/nsIFrame.cpp#4381

and that stores a dirty rect for all absolute children (and fixed = absolute frames on the viewport).

And then when we descend into the frame via it's content parent frame we retrieve the stored dirty rect here

https://searchfox.org/mozilla-central/rev/284187d0a7130d21042beeff0af0627c8e68cacc/layout/generic/nsIFrame.cpp#4140

and thats when we actually build the display items. It's done like this because of how css defines painting order.

The function nsDisplayListBuilder::OutOfFlowDisplayData::ComputeVisibleRectForFrame looks like it could be involved.

https://searchfox.org/mozilla-central/rev/284187d0a7130d21042beeff0af0627c8e68cacc/layout/painting/nsDisplayList.cpp#474

Assignee: nobody → drobertson

I'm starting to get a better idea of what is going on. Still a bit fuzzy on the details, but this only happens for a partial display list build. This appears to be a regression from bug 1650438. Due to ComputeVisibleRectForFrame not including the fixed position content, it doesn't make it into the partial display list and the the FixedPositionInfo is lost here. I think there are a few routes forward:

  1. Do not clear the APZCManager::mFixedPositionInfo when we receive a partial display list build. Instead update the info stored with any new or updated values.
  2. Revert or reduce the impact of the patch to fix bug 1650438. Simple local tests pass and the fixed position elements no longer disappear.

One simple approach along the lines of (2) to consider could be, "force a full display list rebuild when the toolbar transitions from not-fully-hidden to fully-hidden".

The alternative would be to make partial updates correctly handle that transition. Not clearing mFixedPositionInfo on a partial update might be one of things needed to make that work, but I'm not sure whether other pieces would be required as well.

One simple approach along the lines of (2) to consider could be, "force a full display list rebuild when the toolbar transitions from not-fully-hidden to fully-hidden".

This is a good point! I wonder if there are other cases where us not forcing a full display list rebuild on toolbar transitions could be a source of buggyness.

This is an existing place where we do a similar check for "something changed that requires a full update".

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

This is an existing place where we do a similar check for "something changed that requires a full update".

Hmmmm forcing a full update on the dynamic toolbar state change doesn't seem to fix the issue. I'll have to do some more digging on how partial updates work.

I've done a lot more debugging on this over the past few days, and while the following suggestion does make the fixed box disappear less, it does not fix the issue.

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

One simple approach along the lines of (2) to consider could be, "force a full display list rebuild when the toolbar transitions from not-fully-hidden to fully-hidden".

After more debugging it appears there is some sort of error in the calculation of the dirty area when the toolbar is collapsed. So the fact that this only occurs with a partial update is sort of a red herring. For the example provided, when the button fixed to the bottom is clicked the display list builders current mDirtyRect is always (0, 0, 0, 0) for me, but when the toolbar is in transition (even when almost entirely collapsed), the mDirtyRect is populated with a value that makes a lot more sense.

(In reply to Dan Robertson (:dlrobertson) from comment #12)

After more debugging it appears there is some sort of error in the calculation of the dirty area when the toolbar is collapsed. So the fact that this only occurs with a partial update is sort of a red herring. For the example provided, when the button fixed to the bottom is clicked the display list builders current mDirtyRect is always (0, 0, 0, 0) for me, but when the toolbar is in transition (even when almost entirely collapsed), the mDirtyRect is populated with a value that makes a lot more sense.

This is the dirty rect inside the fixed subtree? Have you tried looking at the values in the places in comment 6? That, and related code, is where I would expect the dirty rect to be determined for the fixed subtree.

(In reply to Timothy Nikkel (:tnikkel) from comment #13)

This is the dirty rect inside the fixed subtree? Have you tried looking at the values in the places in comment 6? That, and related code, is where I would expect the dirty rect to be determined for the fixed subtree.

Yeah, the dirty rect handling here does seem off. I just haven't figured out how yet. I did note that we never hit this in the fixed-pos + collapsed toolbar case.

When the dynamic toolbar is completely collapsed and calculating the visible
rect for out of flow content, ensure that the visible rect is expanded to
include the dynamic toolbar max height.

Pushed by drobertson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fae7a5651bb9
Expand visible rect height if dynamic toolbar is collapsed. r=tnikkel
Status: NEW → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → 105 Branch
See Also: → 1790347
You need to log in before you can comment on or make changes to this bug.