Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix getVisibleElements helper in RTL-locales #12464

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions test/unit/ui_utils_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -639,20 +639,20 @@ describe("ui_utils", function () {
// This function takes a fixed layout of pages and compares the system under
// test to the slower implementation above, for a range of scroll viewport
// sizes and positions.
function scrollOverDocument(pages, horizontally = false) {
function scrollOverDocument(pages, horizontally = false, rtl = false) {
const size = pages.reduce(function (max, { div }) {
return Math.max(
max,
horizontally
? div.offsetLeft + div.clientLeft + div.clientWidth
? Math.abs(div.offsetLeft + div.clientLeft + div.clientWidth)
: div.offsetTop + div.clientTop + div.clientHeight
);
}, 0);
// The numbers (7 and 5) are mostly arbitrary, not magic: increase them to
// make scrollOverDocument tests faster, decrease them to make the tests
// more scrupulous, and keep them coprime to reduce the chance of missing
// weird edge case bugs.
for (let i = 0; i < size; i += 7) {
for (let i = -size; i < size; i += 7) {
// The screen height (or width) here (j - i) doubles on each inner loop
// iteration; again, this is just to test an interesting range of cases
// without slowing the tests down to check every possible case.
Expand All @@ -671,7 +671,7 @@ describe("ui_utils", function () {
clientWidth: 10000,
};
expect(
getVisibleElements(scroll, pages, false, horizontally)
getVisibleElements(scroll, pages, false, horizontally, rtl)
).toEqual(slowGetVisibleElements(scroll, pages));
}
}
Expand Down Expand Up @@ -737,6 +737,17 @@ describe("ui_utils", function () {
scrollOverDocument(pages, true);
});

it("works with horizontal scrolling with RTL-documents", function () {
const pages = makePages([
[
[-10, 50],
[-20, 20],
[-30, 10],
],
]);
scrollOverDocument(pages, true, true);
});

it("handles `sortByVisibility` correctly", function () {
const scrollEl = {
scrollTop: 75,
Expand Down
7 changes: 6 additions & 1 deletion web/base_viewer.js
Original file line number Diff line number Diff line change
Expand Up @@ -1006,6 +1006,10 @@ class BaseViewer {
: this._scrollMode === ScrollMode.HORIZONTAL;
}

get _isContainerRtl() {
return getComputedStyle(this.container).direction === "rtl";
}

get isInPresentationMode() {
return this.presentationModeState === PresentationModeState.FULLSCREEN;
}
Expand Down Expand Up @@ -1055,7 +1059,8 @@ class BaseViewer {
this.container,
this._pages,
true,
this._isScrollModeHorizontal
this._isScrollModeHorizontal,
this._isScrollModeHorizontal && this._isContainerRtl
);
}

Expand Down
15 changes: 9 additions & 6 deletions web/ui_utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -442,7 +442,8 @@ function getVisibleElements(
scrollEl,
views,
sortByVisibility = false,
horizontal = false
horizontal = false,
rtl = false
) {
const top = scrollEl.scrollTop,
bottom = top + scrollEl.clientHeight;
Expand All @@ -465,11 +466,11 @@ function getVisibleElements(
element.offsetTop + element.clientTop + element.clientHeight;
return elementBottom > top;
}
function isElementRightAfterViewLeft(view) {
function isElementNextAfterViewHorizontally(view) {
const element = view.div;
const elementRight =
element.offsetLeft + element.clientLeft + element.clientWidth;
return elementRight > left;
const elementLeft = element.offsetLeft + element.clientLeft;
const elementRight = elementLeft + element.clientWidth;
return rtl ? elementLeft < right : elementRight > left;
}

const visible = [],
Expand All @@ -479,7 +480,9 @@ function getVisibleElements(
? 0
: binarySearchFirstItem(
views,
horizontal ? isElementRightAfterViewLeft : isElementBottomAfterViewTop
horizontal
? isElementNextAfterViewHorizontally
: isElementBottomAfterViewTop
);

// Please note the return value of the `binarySearchFirstItem` function when
Expand Down