Skip to content

Commit

Permalink
Bug 1774315 - Don't clamp target scroll position to layer scale. r=ms…
Browse files Browse the repository at this point in the history 
…tange

This prevents scrollLeft = <integer> to return scrollLeft = <integer>,
which is right now papered by bug 1674687.

Differential Revision: https://phabricator.services.mozilla.com/D176306
  • Loading branch information
@emilio
emilio committed Apr 25, 2023
1 parent 995b617 commit f6f54ff
Show file tree
Hide file tree
Showing 12 changed files with 66 additions and 638 deletions.
58 changes: 39 additions & 19 deletions dom/events/test/window_bug1369072.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
<div id="content" style="display: none">
</div>
<pre id="test">
<script type="application/javascript">
<script>

SimpleTest.waitForExplicitFinish();
SimpleTest.waitForFocus(runTests, window);
Expand All @@ -31,6 +31,11 @@
window.opener.ok.apply(window.opener, arguments);
}

function info()
{
window.opener.info.apply(window.opener, arguments);
}

function is()
{
window.opener.is.apply(window.opener, arguments);
Expand Down Expand Up @@ -89,23 +94,38 @@
{
await resetScroll();

return new Promise(resolve => {
// Wait scroll event
function onScroll() {
SimpleTest.executeSoon(resolve);
}
window.addEventListener("scroll", onScroll, { once: true });
iframe.contentWindow.addEventListener("scroll", onScroll, { once: true });

if (aVertical) {
synthesizeKey("KEY_ArrowDown");
} else {
synthesizeKey("KEY_ArrowRight");
}
});
function promiseScrollOnWindow(win) {
is(win.document.documentElement.scrollTop, 0, "Reset didn't work?");
is(win.document.documentElement.scrollLeft, 0, "Reset didn't work?");

return new Promise(resolve => {
win.addEventListener("scroll", function listener() {
// FIXME(bug 1674687): The first scroll might be fractional (and
// scrollTop / Left will round down to zero until bug 1674687 is
// fixed). So make sure we've scrolled at least a pixel before
// resolving. This can be removed once bug 1674687 is fixed.
if (win.document.documentElement.scrollLeft || win.document.documentElement.scrollTop) {
win.removeEventListener("scroll", listener);
SimpleTest.executeSoon(resolve);
}
});
});
}

let promise = Promise.race([
promiseScrollOnWindow(window),
promiseScrollOnWindow(iframe.contentWindow),
]);

if (aVertical) {
synthesizeKey("KEY_ArrowDown");
} else {
synthesizeKey("KEY_ArrowRight");
}
return promise;
}

// When iframe element has focus and the iframe document isn't scrollable, the parent document should be scrolled instead.
info("When iframe element has focus and the iframe document isn't scrollable, the parent document should be scrolled instead.");
document.body.focus();
iframe.focus();
await tryToScrollWithKey(true);
Expand All @@ -114,7 +134,7 @@
ok(document.documentElement.scrollLeft > 0, "ArrowRight keydown event at the iframe whose content is not scrollable should cause scrolling the parent document");
await resetScroll();

// When iframe element has focus and the iframe document scrollable, the parent document shouldn't be scrolled.
info("When iframe element has focus and the iframe document scrollable, the parent document shouldn't be scrolled.");
document.body.focus();
div.style.height = "1000px";
div.style.width = "1000px";
Expand All @@ -127,7 +147,7 @@
ok(iframe.contentDocument.documentElement.scrollLeft > 0, "ArrowRight keydown event at the iframe whose content is scrollable should cause scrolling the iframe document");
await resetScroll();

// If iframe document cannot scroll to specific direction, parent document should be scrolled instead.
info("If iframe document cannot scroll to specific direction, parent document should be scrolled instead.");
div.style.height = "1px";
div.style.width = "1000px";
iframe.focus();
Expand All @@ -148,7 +168,7 @@
ok(document.documentElement.scrollLeft > 0, "ArrowRight keydown event at the iframe whose content is scrollable only vertically should cause scrolling the parent document");
await resetScroll();

// Hidden iframe shouldn't consume keyboard events if it was not scrollable.
info("Hidden iframe shouldn't consume keyboard events if it was not scrollable.");
document.body.focus();
anchor.focus();
iframe.style.display = "none";
Expand Down
4 changes: 3 additions & 1 deletion gfx/layers/apz/test/mochitest/helper_scroll_linked_effect_detector.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,9 @@
// Setup a scroll-linked effect callback.
await promiseScrollAndEvent(() => {
isnot(window.scrollY, 0, "we've already scrolled some amount");
target.style.top = window.scrollY + "px";
// ceil() makes sure that the non-effective setter below is actually
// non-effective.
target.style.top = Math.ceil(window.scrollY) + "px";
eventTimeStamp = document.timeline.currentTime;
});
is(eventTimeStamp, document.timeline.currentTime,
Expand Down
12 changes: 10 additions & 2 deletions gfx/layers/apz/test/mochitest/test_bug1151667.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,16 @@
var subframe = document.getElementById("subframe");
await promiseNativeWheelAndWaitForScrollEvent(subframe, 100, 150, 0, -10);

is(subframe.scrollTop > 0, true, "We should have scrolled the subframe down");
is(document.documentElement.scrollTop, 0, "We should not have scrolled the page");
while (!subframe.scrollTop) {
is(document.documentElement.scrollTop, 0, "We should not have scrolled the page");
info("Waiting for one more scroll");
// FIXME(bug 1674687): The first scroll might be fractional, and thus still
// round down to zero. Wait for another scroll event.
// Remove this when bug 1674687 is fixed.
await new Promise(r => subframe.addEventListener("scroll", r, { once: true }));
}
is(document.documentElement.scrollTop, 0, "We should still not have scrolled the page");
ok(subframe.scrollTop > 0, "We should have scrolled the subframe down");
}

SimpleTest.waitForExplicitFinish();
Expand Down
94 changes: 14 additions & 80 deletions layout/generic/nsGfxScrollFrame.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2787,89 +2787,28 @@ void nsHTMLScrollFrame::ScrollVisual() {

/**
* Clamp desired scroll position aDesired and range [aDestLower, aDestUpper]
* to [aBoundLower, aBoundUpper] and then select the appunit value from among
* aBoundLower, aBoundUpper and those such that (aDesired - aCurrent) *
* aRes/aAppUnitsPerPixel is an integer (or as close as we can get
* modulo rounding to appunits) that is in [aDestLower, aDestUpper] and
* closest to aDesired. If no such value exists, return the nearest in
* [aDestLower, aDestUpper].
* to [aBoundLower, aBoundUpper]
*/
static nscoord ClampAndAlignWithPixels(nscoord aDesired, nscoord aBoundLower,
static nscoord ClampScrollPositionAxis(nscoord aDesired, nscoord aBoundLower,
nscoord aBoundUpper, nscoord aDestLower,
nscoord aDestUpper,
nscoord aAppUnitsPerPixel, double aRes,
nscoord aCurrent) {
nscoord aDestUpper) {
// Intersect scroll range with allowed range, by clamping the ends
// of aRange to be within bounds
nscoord destLower = clamped(aDestLower, aBoundLower, aBoundUpper);
nscoord destUpper = clamped(aDestUpper, aBoundLower, aBoundUpper);

nscoord desired = clamped(aDesired, destLower, destUpper);

double currentLayerVal = (aRes * aCurrent) / aAppUnitsPerPixel;
double desiredLayerVal = (aRes * desired) / aAppUnitsPerPixel;
double delta = desiredLayerVal - currentLayerVal;
double nearestLayerVal = NS_round(delta) + currentLayerVal;

// Convert back from PaintedLayer space to appunits relative to the top-left
// of the scrolled frame.
nscoord aligned =
aRes == 0.0
? 0.0
: NSToCoordRoundWithClamp(nearestLayerVal * aAppUnitsPerPixel / aRes);

// Use a bound if it is within the allowed range and closer to desired than
// the nearest pixel-aligned value.
if (aBoundUpper == destUpper &&
static_cast<decltype(Abs(desired))>(aBoundUpper - desired) <
Abs(desired - aligned)) {
return aBoundUpper;
}

if (aBoundLower == destLower &&
static_cast<decltype(Abs(desired))>(desired - aBoundLower) <
Abs(aligned - desired)) {
return aBoundLower;
}

// Accept the nearest pixel-aligned value if it is within the allowed range.
if (aligned >= destLower && aligned <= destUpper) {
return aligned;
}

// Check if opposite pixel boundary fits into allowed range.
double oppositeLayerVal =
nearestLayerVal + ((nearestLayerVal < desiredLayerVal) ? 1.0 : -1.0);
nscoord opposite = aRes == 0.0
? 0.0
: NSToCoordRoundWithClamp(oppositeLayerVal *
aAppUnitsPerPixel / aRes);
if (opposite >= destLower && opposite <= destUpper) {
return opposite;
}

// No alignment available.
return desired;
return clamped(aDesired, destLower, destUpper);
}

/**
* Clamp desired scroll position aPt to aBounds and then snap
* it to the same layer pixel edges as aCurrent, keeping it within aRange
* during snapping. aCurrent is the current scroll position.
* Clamp desired scroll position aPt to aBounds, keeping it within aRange.
* aCurrent is the current scroll position.
*/
static nsPoint ClampAndAlignWithLayerPixels(const nsPoint& aPt,
const nsRect& aBounds,
const nsRect& aRange,
const nsPoint& aCurrent,
nscoord aAppUnitsPerPixel,
const MatrixScales& aScale) {
return nsPoint(
ClampAndAlignWithPixels(aPt.x, aBounds.x, aBounds.XMost(), aRange.x,
aRange.XMost(), aAppUnitsPerPixel, aScale.xScale,
aCurrent.x),
ClampAndAlignWithPixels(aPt.y, aBounds.y, aBounds.YMost(), aRange.y,
aRange.YMost(), aAppUnitsPerPixel, aScale.yScale,
aCurrent.y));
static nsPoint ClampScrollPosition(const nsPoint& aPt, const nsRect& aBounds,
const nsRect& aRange) {
return nsPoint(ClampScrollPositionAxis(aPt.x, aBounds.x, aBounds.XMost(),
aRange.x, aRange.XMost()),
ClampScrollPositionAxis(aPt.y, aBounds.y, aBounds.YMost(),
aRange.y, aRange.YMost()));
}

/* static */
Expand Down Expand Up @@ -2983,11 +2922,7 @@ void nsHTMLScrollFrame::ScrollToImpl(
}

nsPresContext* presContext = PresContext();
nscoord appUnitsPerDevPixel = presContext->AppUnitsPerDevPixel();
// 'scale' is our estimate of the scale factor that will be applied
// when rendering the scrolled content to its own PaintedLayer.
MatrixScales scale = GetPaintedLayerScaleForFrame(mScrolledFrame);
nsPoint curPos = GetScrollPosition();
const nsPoint curPos = GetScrollPosition();

// Try to align aPt with curPos so they have an integer number of layer
// pixels between them. This gives us the best chance of scrolling without
Expand All @@ -2999,8 +2934,7 @@ void nsHTMLScrollFrame::ScrollToImpl(
// and are relative to the scrollport top-left. This difference doesn't
// actually matter since all we are about is that there be an integer number
// of layer pixels between pt and curPos.
nsPoint pt = ClampAndAlignWithLayerPixels(aPt, GetLayoutScrollRange(), aRange,
curPos, appUnitsPerDevPixel, scale);
nsPoint pt = ClampScrollPosition(aPt, GetLayoutScrollRange(), aRange);
if (pt == curPos) {
// Even if we are bailing out due to no-op main-thread scroll position
// change, we might need to cancel an APZ smooth scroll that we already
Expand Down
26 changes: 0 additions & 26 deletions ...platform/meta/css/css-writing-modes/forms/text-input-vertical-overflow-no-scroll.html.ini
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,6 @@

[input[type=text\] in sideways-lr: typing characters in input should not cause the page to scroll]
expected:
if (os == "win") and (processor == "x86_64") and debug and not swgl: FAIL
if (os == "win") and (processor == "x86_64") and debug and swgl: [FAIL, PASS]
if (os == "linux") and not debug and (processor == "x86_64") and fission: [FAIL, PASS]
if (os == "win") and (processor == "x86") and debug: [FAIL, PASS]
if (os == "android") and debug and not swgl: PASS
if (os == "linux") and not debug and (processor == "x86"): [FAIL, PASS]
if (os == "linux") and debug: FAIL
if os == "mac": PASS
[PASS, FAIL]

Expand All @@ -46,12 +39,6 @@

[input[type=password\] in sideways-lr: typing characters in input should not cause the page to scroll]
expected:
if (os == "win") and not debug and (processor == "x86"): [PASS, FAIL]
if (os == "android") and debug and swgl: [PASS, FAIL]
if (os == "android") and debug and not swgl: PASS
if (os == "linux") and not debug and not fission: [PASS, FAIL]
if (os == "android") and not debug: [PASS, FAIL]
if (os == "linux") and debug: FAIL
if os == "mac": PASS
[FAIL, PASS]

Expand All @@ -73,12 +60,6 @@

[input[type=search\] in sideways-lr: typing characters in input should not cause the page to scroll]
expected:
if (os == "win") and not swgl and not debug and (processor == "x86"): [PASS, FAIL]
if (os == "android") and debug and not swgl: PASS
if (os == "android") and debug and swgl: [PASS, FAIL]
if (os == "linux") and debug: FAIL
if (os == "android") and not debug: [PASS, FAIL]
if (os == "win") and swgl: [PASS, FAIL]
if os == "mac": PASS
[FAIL, PASS]

Expand All @@ -97,13 +78,6 @@

[input[type=number\] in sideways-lr: typing characters in input should not cause the page to scroll]
expected:
if (os == "linux") and debug and fission and not swgl: FAIL
if (os == "linux") and debug and not fission: FAIL
if (os == "android") and debug and swgl: [PASS, FAIL]
if (os == "android") and debug and not swgl: PASS
if (os == "win") and debug and (processor == "x86"): FAIL
if (os == "win") and not debug and (processor == "x86_64"): [PASS, FAIL]
if (os == "android") and not debug: [PASS, FAIL]
if os == "mac": PASS
[FAIL, PASS]

Expand Down
Loading

0 comments on commit f6f54ff

Please sign in to comment.