Closed Bug 1226872 Opened 9 years ago Closed 9 years ago

Fennec with C++ APZ sometimes get stuck in a state with scrollbars in the top-left and zooming not working properly

Categories

(Firefox for Android Graveyard :: Toolbar, defect)

All
Android
defect
Not set
normal

Tracking

(firefox45 fixed)

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(2 files)

While testing with C++ APZ I'm able to reproduce (somewhat intermittently) a scenario where upon zooming in, the page remains low-res. If I scroll at this point so that the dynamic toolbar goes on or off, the page immediately zooms back out. Also the scrollbars end up positioned on a fictitious viewport area in the top-left corner of the screen. We saw this on nytimes.com as well a few times last week. What I'm seeing when it gets into this state is that the presShell resolution has been reset to 1.0 somehow (not yet sure how) and so when APZCCH gets a repaint request from the APZ, it doesn't update the resolution because there's a resolution mismatch [1]. The root cause here is the resolution getting set to 1.0 out of band, not yet sure why that's happening. [1] https://dxr.mozilla.org/mozilla-central/rev/3f5afaf4e6b72c4b1a20749b4ce7d945add5299f/gfx/layers/apz/util/APZCCallbackHelper.cpp#219
It looks like the resolution is being read from the root scrollframe rather than the presShell [1], and the root scrollframe's resolution is 1.0. It actually gets set to the correct resolution initially but then we throw away that scrollframe and create about 40 new ones in quick succession which all have a 1.0 resolution. I think we might need to have the root scrollframe pick up the resolution from the presShell on creation. Still digging. [1] https://dxr.mozilla.org/mozilla-central/rev/3f5afaf4e6b72c4b1a20749b4ce7d945add5299f/layout/base/nsLayoutUtils.cpp#8450
So yeah, picking up the resolution from the presShell in the ScrollFrameHelper constructor fixes the problem. However storing the resolution in two different places seems unnecessary and redundant. IIRC this was added for fennec to be able to restore the zoom when going back (so that the zoom is saved along with the scroll position in the history) but maybe we can find a better way to do this so it doesn't duplicate state which can get out of sync.
This stuff was added back in bug 611556 to be able to save the resolution when navigating back to a page. However, having two copies of the resolution means that they can get out of sync, which is what was causing the bug here. We can either get rid of the resolution from the presshell (which is what was discussed in that bug) or get rid of it from the scrollframe (which is what I'm doing here). The reason I chose to do this was because our current architecture is more consistent with the resolution being a property of the presshell; additional resolution-related state is also currently on the presshell. Also I'm not sure there's much value is supporting zoom on arbitrary scrollable elements; it just feels cleaner to only allow it per-presshell.
Attachment #8690471 - Flags: review?(roc)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #1) > It > actually gets set to the correct resolution initially but then we throw away > that scrollframe and create about 40 new ones in quick succession which all > have a 1.0 resolution. Just to clarify the "40 new ones" wasn't accurate - we only re-created the root scrollframe once, the rest of the scrollframes I was seeing were not root scrollframes. So the problem here was that: - the presShell and root scrollframe were created (with 1.0 resolution) - a 0.39 resolution was set on the presshell and root scrollframe - the root scrollframe was destroyed a new one was created, with a 1.0 resolution - no saved state was found to restore onto the new root scrollframe This left things in a state where the presShell had resolution 0.39 and the root scrollframe had resolution 1.0 which screwed up a lot of stuff.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Depends on: 1228357
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: