Magic-Mirror/gecko-dev
1
0
Fork 0
mirror of https://github.com/mozilla/gecko-dev.git synced 2024-11-30 00:01:50 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Bug 1567237 - Only use scroll range to select scrollable frames to scroll to, don't use scrollbar visibility. r=tnikkel

Browse Source 
This is what other browsers do, and it does make sense to me, it's useless to
try to scroll a frame with no scroll range in a given direction.

I think all callers of this function should be treated like this, so this is
more like a RFC / feedback request than a patch per se.

The wheel handling code already checks scroll range, so there's no difference of
behavior in that case, if I'm reading the code right.

There are a few other functions that check the result of
GetPerceivedScrollingDirections(), but I think if we change this we should
change this consistently.

I also think that if we do this we should rename the method to something like
GetAvailableScrollingDirections() or such.

Anyhow, wdyt? I should also add a test for this if we go with this.

Differential Revision: https://phabricator.services.mozilla.com/D38991

--HG--
extra : moz-landing-system : lando
This commit is contained in:
Emilio Cobos Álvarez 2019-07-24 22:33:57 +00:00
parent 4098176939
commit eb7d8bffd8
8 changed files with 76 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
dom/events/WheelHandlingHelper.cpp
View File

@ -69,7 +69,7 @@ bool WheelHandlingUtils::CanScrollOn(nsIScrollableFrame* aScrollFrame,
nsPoint scrollPt = aScrollFrame->GetScrollPosition();
nsRect scrollRange = aScrollFrame->GetScrollRange();
uint32_t directions = aScrollFrame->GetPerceivedScrollingDirections();
uint32_t directions = aScrollFrame->GetAvailableScrollingDirections();
return (aDirectionX && (directions & nsIScrollableFrame::HORIZONTAL) &&
CanScrollInRange(scrollRange.x, scrollPt.x, scrollRange.XMost(),
@ -741,12 +741,12 @@ void ESMAutoDirWheelDeltaAdjuster::OnAdjusted() {
}
bool ESMAutoDirWheelDeltaAdjuster::CanScrollAlongXAxis() const {
return mScrollTargetFrame->GetPerceivedScrollingDirections() &
return mScrollTargetFrame->GetAvailableScrollingDirections() &
nsIScrollableFrame::HORIZONTAL;
}
bool ESMAutoDirWheelDeltaAdjuster::CanScrollAlongYAxis() const {
return mScrollTargetFrame->GetPerceivedScrollingDirections() &
return mScrollTargetFrame->GetAvailableScrollingDirections() &
nsIScrollableFrame::VERTICAL;
}

2
layout/base/PresShell.cpp
View File

@ -3350,7 +3350,7 @@ static void ScrollToShowRect(nsIScrollableFrame* aFrameAsScrollable,
ScrollStyles ss = aFrameAsScrollable->GetScrollStyles();
nsRect allowedRange(scrollPt, nsSize(0, 0));
bool needToScroll = false;
uint32_t directions = aFrameAsScrollable->GetPerceivedScrollingDirections();
uint32_t directions = aFrameAsScrollable->GetAvailableScrollingDirections();
if (((aScrollFlags & ScrollFlags::ScrollOverflowHidden) ||
ss.mVertical != StyleOverflow::Hidden) &&

2
layout/base/nsLayoutUtils.cpp
View File

@ -2015,7 +2015,7 @@ nsIScrollableFrame* nsLayoutUtils::GetNearestScrollableFrameForDirection(
nsIScrollableFrame* scrollableFrame = do_QueryFrame(f);
if (scrollableFrame) {
ScrollStyles ss = scrollableFrame->GetScrollStyles();
uint32_t directions = scrollableFrame->GetPerceivedScrollingDirections();
uint32_t directions = scrollableFrame->GetAvailableScrollingDirections();
if (aDirection == eVertical
? (ss.mVertical != StyleOverflow::Hidden &&
(directions & nsIScrollableFrame::VERTICAL))

1
layout/base/tests/mochitest.ini
View File

@ -419,6 +419,7 @@ support-files = resize_flush_iframe.html
[test_scroll_event_ordering.html]
[test_scroll_per_page.html]
support-files = window_empty_document.html
[test_scroll_space_no_range_overflow_scroll.html]
[test_scroll_selection_into_view.html]
skip-if = toolkit == 'android' # Bug 1355844
support-files =

64
layout/base/tests/test_scroll_space_no_range_overflow_scroll.html Normal file
View File

@ -0,0 +1,64 @@
<!doctype html>
<title>Test for bug 1567237</title>
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script src="/tests/SimpleTest/EventUtils.js"></script>
<style>
.spacer { height: 200vh; }
.scroller { height: 300px; overflow: scroll; }
</style>
<div id="unscrollable" class="scroller" tabindex=0></div>
<div id="scrollable" class="scroller" tabindex=0>
<div class="spacer"></div>
</div>
<div class="spacer"></div>
<script>
function waitForScrollEvent(target) {
return new Promise(resolve => {
target.addEventListener("scroll", resolve, { once: true });
});
}
let selectionController =
SpecialPowers.wrap(window)
.docShell
.QueryInterface(SpecialPowers.Ci.nsIInterfaceRequestor)
.getInterface(SpecialPowers.Ci.nsISelectionDisplay)
.QueryInterface(SpecialPowers.Ci.nsISelectionController);
function doPageDown(targetExpectedToScroll) {
let promise = waitForScrollEvent(targetExpectedToScroll);
selectionController.pageMove(true, false);
return promise;
}
promise_test(async function() {
await SpecialPowers.pushPrefEnv({"set": [["general.smoothScroll", false]]});
const rootScroller = document.documentElement;
const scrollable = document.querySelector("#scrollable");
const unscrollable = document.querySelector("#unscrollable");
assert_equals(rootScroller.scrollTop, 0, "Root should start unscrolled");
assert_equals(scrollable.scrollTop, 0, "#scrollable should start unscrolled");
assert_equals(unscrollable.scrollTop, 0, "#unscrollable should not be able to scroll at all");
assert_true(rootScroller.scrollTopMax > 0, "Should be able to scroll the document element");
assert_true(scrollable.scrollTopMax > 0, "Should be able to scroll #scrollable");
assert_equals(unscrollable.scrollTopMax, 0, "#unscrollable should not be able to scroll at all (checking scrollTopMax)");
scrollable.focus();
await doPageDown(scrollable);
assert_not_equals(scrollable.scrollTop, 0, "Should have scrolled when pressing space");
unscrollable.focus();
let rootScrollTop = rootScroller.scrollTop; // Could've scrolled to scroll `scrollable` into view before.
await doPageDown(window);
assert_equals(unscrollable.scrollTop, 0, "Should not be able to scroll the unscrollable div");
assert_not_equals(rootScroller.scrollTop, rootScrollTop, "Root should be able to scroll");
// Null out the controller. Otherwise we leak the whole window because
// PresShell is not cycle-collectable. See bug 1567237.
selectionController = null;
}, "Overflow scroll without range doesn't block scrolling of the main document");
</script>

2
layout/generic/nsFrameSelection.cpp
View File

@ -1622,7 +1622,7 @@ nsIFrame* nsFrameSelection::GetFrameToPageSelect() const {
if (scrollStyles.mVertical == StyleOverflow::Hidden) {
continue;
}
uint32_t directions = scrollableFrame->GetPerceivedScrollingDirections();
uint32_t directions = scrollableFrame->GetAvailableScrollingDirections();
if (directions & nsIScrollableFrame::VERTICAL) {
// If there is sub scrollable frame, let's use its page size to select.
return frame;

4
layout/generic/nsGfxScrollFrame.cpp
View File

@ -6654,10 +6654,10 @@ void ScrollFrameHelper::FireScrolledAreaEvent() {
}
}
uint32_t nsIScrollableFrame::GetPerceivedScrollingDirections() const {
uint32_t nsIScrollableFrame::GetAvailableScrollingDirections() const {
nscoord oneDevPixel =
GetScrolledFrame()->PresContext()->AppUnitsPerDevPixel();
uint32_t directions = GetScrollbarVisibility();
uint32_t directions = 0;
nsRect scrollRange = GetScrollRange();
if (scrollRange.width >= oneDevPixel) {
directions |= HORIZONTAL;

7
layout/generic/nsIScrollableFrame.h
View File

@ -81,11 +81,10 @@ class nsIScrollableFrame : public nsIScrollbarMediator {
*/
virtual uint32_t GetScrollbarVisibility() const = 0;
/**
* Returns the directions in which scrolling is perceived to be allowed.
* A direction is perceived to be allowed if there is a visible scrollbar
* for that direction or if the scroll range is at least one device pixel.
* Returns the directions in which scrolling is allowed (if the scroll range
* is at least one device pixel in that direction).
*/
uint32_t GetPerceivedScrollingDirections() const;
uint32_t GetAvailableScrollingDirections() const;
/**
* Return the actual sizes of all possible scrollbars. Returns 0 for scrollbar
* positions that don't have a scrollbar or where the scrollbar is not