Bug 1304689 - Ensure frame reconstructions don't clobber a 'stronger' scroll origin with a 'weaker' one. r?tnikkel
If, within a single refresh driver tick, the scroll position is updated by JS
explicitly, and then subsequently also updated by a frame reconstruction, the
scroll origin from the former (nsGkAtoms::other) can get clobbered by the latter
(to nsGkAtoms::restore). The restore scroll origin is "weaker" in that it can
be ignored by the APZ code in some circumstances. This is undesirable because
it means the JS scroll update also gets ignored. This patch ensures that when
setting the scroll origin we don't do this clobbering of stronger origins with
weaker origins.
MozReview-Commit-ID: DA4EHp1Debu
--- a/gfx/layers/apz/test/mochitest/mochitest.ini
+++ b/gfx/layers/apz/test/mochitest/mochitest.ini
@@ -58,8 +58,10 @@
skip-if = (toolkit == 'android') # mouse events not supported on mobile
[test_touch_listeners_impacting_wheel.html]
skip-if = (toolkit == 'android') || (toolkit == 'cocoa') # wheel events not supported on mobile, and synthesized wheel smooth-scrolling not supported on OS X
[test_bug1253683.html]
skip-if = (os == 'android') || (os == 'b2g') # uses wheel events which are not supported on mobile
[test_group_zoom.html]
skip-if = (toolkit != 'android') # only android supports zoom
[test_group_pointerevents.html]
+[test_bug1304689.html]
+[test_bug1304689-2.html]
new file mode 100644
--- /dev/null
+++ b/gfx/layers/apz/test/mochitest/test_bug1304689-2.html
@@ -0,0 +1,131 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1304689
+-->
+<head>
+ <meta charset="utf-8">
+ <title>Test for Bug 1285070</title>
+ <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+ <script type="application/javascript" src="/tests/SimpleTest/paint_listener.js"></script>
+ <script type="application/javascript" src="apz_test_utils.js"></script>
+ <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+ <style type="text/css">
+ #outer {
+ height: 400px;
+ width: 415px;
+ overflow: scroll;
+ position: relative;
+ scroll-behavior: smooth;
+ }
+ #outer.contentBefore::before {
+ top: 0;
+ content: '';
+ display: block;
+ height: 2px;
+ position: absolute;
+ width: 100%;
+ z-index: 99;
+ }
+ </style>
+ <script type="application/javascript">
+
+function* test(testDriver) {
+ var utils = SpecialPowers.DOMWindowUtils;
+ var elm = document.getElementById('outer');
+
+ // Set margins on the element, to ensure it is layerized
+ utils.setDisplayPortMarginsForElement(0, 0, 0, 0, elm, /*priority*/ 1);
+ yield waitForAllPaints(function() {
+ flushApzRepaints(testDriver);
+ });
+
+ // Take control of the refresh driver
+ utils.advanceTimeAndRefresh(0);
+
+ // Start a smooth-scroll animation in the compositor and let it go a few
+ // frames, so that there is some "user scrolling" going on (per the comment
+ // in AsyncPanZoomController::NotifyLayersUpdated)
+ elm.scrollTop = 10;
+ utils.advanceTimeAndRefresh(16);
+ utils.advanceTimeAndRefresh(16);
+ utils.advanceTimeAndRefresh(16);
+ utils.advanceTimeAndRefresh(16);
+
+ // Do another scroll update but also do a frame reconstruction within the same
+ // tick of the refresh driver.
+ elm.scrollTop = 100;
+ elm.classList.add('contentBefore');
+
+ // Now let everything settle and all the animations run out
+ for (var i = 0; i < 60; i++) {
+ utils.advanceTimeAndRefresh(16);
+ }
+ utils.restoreNormalRefresh();
+
+ yield flushApzRepaints(testDriver);
+ is(elm.scrollTop, 100, "The scrollTop now should be y=100");
+}
+
+if (isApzEnabled()) {
+ SimpleTest.waitForExplicitFinish();
+ pushPrefs([["apz.displayport_expiry_ms", 0]])
+ .then(waitUntilApzStable)
+ .then(runContinuation(test))
+ .then(SimpleTest.finish);
+}
+
+ </script>
+</head>
+<body>
+ <div id="outer">
+ <div id="inner">
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ </div>
+ </div>
+</body>
+</html>
new file mode 100644
--- /dev/null
+++ b/gfx/layers/apz/test/mochitest/test_bug1304689.html
@@ -0,0 +1,135 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1304689
+-->
+<head>
+ <meta charset="utf-8">
+ <title>Test for Bug 1285070</title>
+ <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+ <script type="application/javascript" src="/tests/SimpleTest/paint_listener.js"></script>
+ <script type="application/javascript" src="apz_test_utils.js"></script>
+ <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
+ <style type="text/css">
+ #outer {
+ height: 400px;
+ width: 415px;
+ overflow: scroll;
+ position: relative;
+ scroll-behavior: smooth;
+ }
+ #outer.instant {
+ scroll-behavior: auto;
+ }
+ #outer.contentBefore::before {
+ top: 0;
+ content: '';
+ display: block;
+ height: 2px;
+ position: absolute;
+ width: 100%;
+ z-index: 99;
+ }
+ </style>
+ <script type="application/javascript">
+
+function* test(testDriver) {
+ var utils = SpecialPowers.DOMWindowUtils;
+ var elm = document.getElementById('outer');
+
+ // Set margins on the element, to ensure it is layerized
+ utils.setDisplayPortMarginsForElement(0, 0, 0, 0, elm, /*priority*/ 1);
+ yield waitForAllPaints(function() {
+ flushApzRepaints(testDriver);
+ });
+
+ // Take control of the refresh driver
+ utils.advanceTimeAndRefresh(0);
+
+ // Start a smooth-scroll animation in the compositor and let it go a few
+ // frames, so that there is some "user scrolling" going on (per the comment
+ // in AsyncPanZoomController::NotifyLayersUpdated)
+ elm.scrollTop = 10;
+ utils.advanceTimeAndRefresh(16);
+ utils.advanceTimeAndRefresh(16);
+ utils.advanceTimeAndRefresh(16);
+ utils.advanceTimeAndRefresh(16);
+
+ // Do another scroll update but also do a frame reconstruction within the same
+ // tick of the refresh driver.
+ elm.classList.add('instant');
+ elm.scrollTop = 100;
+ elm.classList.add('contentBefore');
+
+ // Now let everything settle and all the animations run out
+ for (var i = 0; i < 60; i++) {
+ utils.advanceTimeAndRefresh(16);
+ }
+ utils.restoreNormalRefresh();
+
+ yield flushApzRepaints(testDriver);
+ is(elm.scrollTop, 100, "The scrollTop now should be y=100");
+}
+
+if (isApzEnabled()) {
+ SimpleTest.waitForExplicitFinish();
+ pushPrefs([["apz.displayport_expiry_ms", 0]])
+ .then(waitUntilApzStable)
+ .then(runContinuation(test))
+ .then(SimpleTest.finish);
+}
+
+ </script>
+</head>
+<body>
+ <div id="outer">
+ <div id="inner">
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ this is some scrollable text.<br>
+ this is a second line to make the scrolling more obvious.<br>
+ and a third for good measure.<br>
+ </div>
+ </div>
+</body>
+</html>
--- a/layout/base/nsLayoutUtils.cpp
+++ b/layout/base/nsLayoutUtils.cpp
@@ -8890,16 +8890,17 @@ nsLayoutUtils::ComputeScrollMetadata(nsI
// its scroll offset. We want to distinguish the case where the scroll offset
// was "restored" because in that case the restored scroll position should
// not overwrite a user-driven scroll.
if (scrollableFrame->LastScrollOrigin() == nsGkAtoms::restore) {
metrics.SetScrollOffsetRestored(scrollableFrame->CurrentScrollGeneration());
} else if (CanScrollOriginClobberApz(scrollableFrame->LastScrollOrigin())) {
metrics.SetScrollOffsetUpdated(scrollableFrame->CurrentScrollGeneration());
}
+ scrollableFrame->AllowScrollOriginDowngrade();
nsIAtom* lastSmoothScrollOrigin = scrollableFrame->LastSmoothScrollOrigin();
if (lastSmoothScrollOrigin) {
metrics.SetSmoothScrollOffsetUpdated(scrollableFrame->CurrentScrollGeneration());
}
nsSize lineScrollAmount = scrollableFrame->GetLineScrollAmount();
LayoutDeviceIntSize lineScrollAmountInDevPixels =
--- a/layout/base/nsPresState.h
+++ b/layout/base/nsPresState.h
@@ -16,16 +16,17 @@
#include "nsAutoPtr.h"
class nsPresState
{
public:
nsPresState()
: mContentData(nullptr)
, mScrollState(0, 0)
+ , mAllowScrollOriginDowngrade(true)
, mResolution(1.0)
, mScaleToResolution(false)
, mDisabledSet(false)
, mDisabled(false)
, mDroppedDown(false)
{}
void SetScrollState(const nsPoint& aState)
@@ -33,16 +34,26 @@ public:
mScrollState = aState;
}
nsPoint GetScrollPosition() const
{
return mScrollState;
}
+ void SetAllowScrollOriginDowngrade(bool aAllowScrollOriginDowngrade)
+ {
+ mAllowScrollOriginDowngrade = aAllowScrollOriginDowngrade;
+ }
+
+ bool GetAllowScrollOriginDowngrade()
+ {
+ return mAllowScrollOriginDowngrade;
+ }
+
void SetResolution(float aSize)
{
mResolution = aSize;
}
float GetResolution() const
{
return mResolution;
@@ -99,16 +110,17 @@ public:
{
return mDroppedDown;
}
// MEMBER VARIABLES
protected:
nsCOMPtr<nsISupports> mContentData;
nsPoint mScrollState;
+ bool mAllowScrollOriginDowngrade;
float mResolution;
bool mScaleToResolution;
bool mDisabledSet;
bool mDisabled;
bool mDroppedDown;
};
#endif /* nsPresState_h_ */
--- a/layout/generic/nsGfxScrollFrame.cpp
+++ b/layout/generic/nsGfxScrollFrame.cpp
@@ -2010,16 +2010,17 @@ ScrollFrameHelper::ScrollFrameHelper(nsC
, mVScrollbarBox(nullptr)
, mScrolledFrame(nullptr)
, mScrollCornerBox(nullptr)
, mResizerBox(nullptr)
, mOuter(aOuter)
, mAsyncScroll(nullptr)
, mAsyncSmoothMSDScroll(nullptr)
, mLastScrollOrigin(nsGkAtoms::other)
+ , mAllowScrollOriginDowngrade(false)
, mLastSmoothScrollOrigin(nullptr)
, mScrollGeneration(++sScrollGenerationCounter)
, mDestination(0, 0)
, mScrollPosAtLastPaint(0, 0)
, mRestorePos(-1, -1)
, mLastPos(-1, -1)
, mScrollPosForLayerPixelAlignment(-1, -1)
, mLastUpdateFramesPos(-1, -1)
@@ -2815,17 +2816,31 @@ ScrollFrameHelper::ScrollToImpl(nsPoint
nsRect oldDisplayPort;
nsIContent* content = mOuter->GetContent();
nsLayoutUtils::GetHighResolutionDisplayPort(content, &oldDisplayPort);
oldDisplayPort.MoveBy(-mScrolledFrame->GetPosition());
// Update frame position for scrolling
mScrolledFrame->SetPosition(mScrollPort.TopLeft() - pt);
- mLastScrollOrigin = aOrigin;
+
+ // If |mLastScrollOrigin| is already set to something that can clobber APZ's
+ // scroll offset, then we don't want to change it to something that can't.
+ // If we allowed this, then we could end up in a state where APZ ignores
+ // legitimate scroll offset updates because the origin has been masked by
+ // a later change within the same refresh driver tick.
+ bool isScrollOriginDowngrade =
+ nsLayoutUtils::CanScrollOriginClobberApz(mLastScrollOrigin) &&
+ !nsLayoutUtils::CanScrollOriginClobberApz(aOrigin);
+ bool allowScrollOriginChange = mAllowScrollOriginDowngrade ||
+ !isScrollOriginDowngrade;
+ if (allowScrollOriginChange) {
+ mLastScrollOrigin = aOrigin;
+ mAllowScrollOriginDowngrade = false;
+ }
mLastSmoothScrollOrigin = nullptr;
mScrollGeneration = ++sScrollGenerationCounter;
ScrollVisual();
bool schedulePaint = true;
if (nsLayoutUtils::AsyncPanZoomEnabled(mOuter) && gfxPrefs::APZPaintSkipping()) {
// If APZ is enabled with paint-skipping, there are certain conditions in
@@ -5900,46 +5915,53 @@ ScrollFrameHelper::SaveState() const
// Don't store a scroll state if we never have been scrolled or restored
// a previous scroll state, and we're not in the middle of a smooth scroll.
bool isInSmoothScroll = IsProcessingAsyncScroll() || mLastSmoothScrollOrigin;
if (!mHasBeenScrolled && !mDidHistoryRestore && !isInSmoothScroll) {
return nullptr;
}
nsPresState* state = new nsPresState();
+ bool allowScrollOriginDowngrade =
+ !nsLayoutUtils::CanScrollOriginClobberApz(mLastScrollOrigin) ||
+ mAllowScrollOriginDowngrade;
// Save mRestorePos instead of our actual current scroll position, if it's
// valid and we haven't moved since the last update of mLastPos (same check
// that ScrollToRestoredPosition uses). This ensures if a reframe occurs
// while we're in the process of loading content to scroll to a restored
// position, we'll keep trying after the reframe. Similarly, if we're in the
// middle of a smooth scroll, store the destination so that when we restore
// we'll jump straight to the end of the scroll animation, rather than
// effectively dropping it. Note that the mRestorePos will override the
// smooth scroll destination if both are present.
nsPoint pt = GetLogicalScrollPosition();
if (isInSmoothScroll) {
pt = mDestination;
+ allowScrollOriginDowngrade = false;
}
if (mRestorePos.y != -1 && pt == mLastPos) {
pt = mRestorePos;
}
state->SetScrollState(pt);
+ state->SetAllowScrollOriginDowngrade(allowScrollOriginDowngrade);
if (mIsRoot) {
// Only save resolution properties for root scroll frames
nsIPresShell* shell = mOuter->PresContext()->PresShell();
state->SetResolution(shell->GetResolution());
state->SetScaleToResolution(shell->ScaleToResolution());
}
return state;
}
void
ScrollFrameHelper::RestoreState(nsPresState* aState)
{
mRestorePos = aState->GetScrollPosition();
+ MOZ_ASSERT(mLastScrollOrigin == nsGkAtoms::other);
+ mAllowScrollOriginDowngrade = aState->GetAllowScrollOriginDowngrade();
mDidHistoryRestore = true;
mLastPos = mScrolledFrame ? GetLogicalScrollPosition() : nsPoint(0,0);
// Resolution properties should only exist on root scroll frames.
MOZ_ASSERT(mIsRoot || (!aState->GetScaleToResolution() &&
aState->GetResolution() == 1.0));
if (mIsRoot) {
--- a/layout/generic/nsGfxScrollFrame.h
+++ b/layout/generic/nsGfxScrollFrame.h
@@ -414,16 +414,17 @@ public:
void ResetDisplayPortExpiryTimer();
void ScheduleSyntheticMouseMove();
static void ScrollActivityCallback(nsITimer *aTimer, void* anInstance);
void HandleScrollbarStyleSwitching();
nsIAtom* LastScrollOrigin() const { return mLastScrollOrigin; }
+ void AllowScrollOriginDowngrade() { mAllowScrollOriginDowngrade = true; }
nsIAtom* LastSmoothScrollOrigin() const { return mLastSmoothScrollOrigin; }
uint32_t CurrentScrollGeneration() const { return mScrollGeneration; }
nsPoint LastScrollDestination() const { return mDestination; }
void ResetScrollInfoIfGeneration(uint32_t aGeneration) {
if (aGeneration == mScrollGeneration) {
mLastScrollOrigin = nullptr;
mLastSmoothScrollOrigin = nullptr;
}
@@ -474,16 +475,17 @@ public:
nsIFrame* mScrollCornerBox;
nsIFrame* mResizerBox;
nsContainerFrame* mOuter;
RefPtr<AsyncScroll> mAsyncScroll;
RefPtr<AsyncSmoothMSDScroll> mAsyncSmoothMSDScroll;
RefPtr<ScrollbarActivity> mScrollbarActivity;
nsTArray<nsIScrollPositionListener*> mListeners;
nsIAtom* mLastScrollOrigin;
+ bool mAllowScrollOriginDowngrade;
nsIAtom* mLastSmoothScrollOrigin;
Maybe<nsPoint> mApzSmoothScrollDestination;
uint32_t mScrollGeneration;
nsRect mScrollPort;
// Where we're currently scrolling to, if we're scrolling asynchronously.
// If we're not in the middle of an asynchronous scroll then this is
// just the current scroll position. ScrollBy will choose its
// destination based on this value.
@@ -896,16 +898,19 @@ public:
return mHelper.IsRectNearlyVisible(aRect);
}
virtual nsRect ExpandRectToNearlyVisible(const nsRect& aRect) const override {
return mHelper.ExpandRectToNearlyVisible(aRect);
}
virtual nsIAtom* LastScrollOrigin() override {
return mHelper.LastScrollOrigin();
}
+ virtual void AllowScrollOriginDowngrade() override {
+ mHelper.AllowScrollOriginDowngrade();
+ }
virtual nsIAtom* LastSmoothScrollOrigin() override {
return mHelper.LastSmoothScrollOrigin();
}
virtual uint32_t CurrentScrollGeneration() override {
return mHelper.CurrentScrollGeneration();
}
virtual nsPoint LastScrollDestination() override {
return mHelper.LastScrollDestination();
@@ -1308,16 +1313,19 @@ public:
return mHelper.IsRectNearlyVisible(aRect);
}
virtual nsRect ExpandRectToNearlyVisible(const nsRect& aRect) const override {
return mHelper.ExpandRectToNearlyVisible(aRect);
}
virtual nsIAtom* LastScrollOrigin() override {
return mHelper.LastScrollOrigin();
}
+ virtual void AllowScrollOriginDowngrade() override {
+ mHelper.AllowScrollOriginDowngrade();
+ }
virtual nsIAtom* LastSmoothScrollOrigin() override {
return mHelper.LastSmoothScrollOrigin();
}
virtual uint32_t CurrentScrollGeneration() override {
return mHelper.CurrentScrollGeneration();
}
virtual nsPoint LastScrollDestination() override {
return mHelper.LastScrollDestination();
--- a/layout/generic/nsIScrollableFrame.h
+++ b/layout/generic/nsIScrollableFrame.h
@@ -347,16 +347,25 @@ public:
virtual nsRect ExpandRectToNearlyVisible(const nsRect& aRect) const = 0;
/**
* Returns the origin that triggered the last instant scroll. Will equal
* nsGkAtoms::apz when the compositor's replica frame metrics includes the
* latest instant scroll.
*/
virtual nsIAtom* LastScrollOrigin() = 0;
/**
+ * Sets a flag on the scrollframe that indicates the current scroll origin
+ * has been sent over in a layers transaction, and subsequent changes to
+ * the scroll position by "weaker" origins are permitted to overwrite the
+ * the scroll origin. Scroll origins that nsLayoutUtils::CanScrollOriginClobberApz
+ * returns false for are considered "weaker" than scroll origins for which
+ * that function returns true.
+ */
+ virtual void AllowScrollOriginDowngrade() = 0;
+ /**
* Returns the origin that triggered the last smooth scroll.
* Will equal nsGkAtoms::apz when the compositor's replica frame
* metrics includes the latest smooth scroll. The compositor will always
* perform an instant scroll prior to instantiating any smooth scrolls
* if LastScrollOrigin and LastSmoothScrollOrigin indicate that
* an instant scroll and a smooth scroll have occurred since the last
* replication of the frame metrics.
*