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)
Tracking
(firefox45 fixed)
RESOLVED
FIXED
Firefox 45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: kats, Assigned: kats)
References
Details
Attachments
(2 files)
20.33 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
9.23 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•9 years ago
|
||
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
Assignee | ||
Comment 2•9 years ago
|
||
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.
Assignee | ||
Comment 3•9 years ago
|
||
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)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8690472 -
Flags: review?(roc)
Assignee | ||
Comment 5•9 years ago
|
||
(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.
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8690471 -
Flags: review?(roc) → review+
Attachment #8690472 -
Flags: review?(roc) → review+
Comment 8•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/35ed8d9de74e
https://hg.mozilla.org/mozilla-central/rev/4347df8d3bcf
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•