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.

Sign up for GitHub

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

Jump to bottom

Right align scrollToIndex in RTL #38737

Closed
wants to merge 2 commits into from
Closed

Right align scrollToIndex in RTL #38737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eb41627
Return right edge in RTL Cell metrics
NickGerleman Aug 4, 2023
0949fa6
Right align scrollToIndex in RTL (#38737)
NickGerleman Aug 4, 2023
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
9 changes: 6 additions & 3 deletions packages/virtualized-lists/Lists/ListMetricsAggregator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,8 +256,8 @@ export default class ListMetricsAggregator {
}

/**
* Converts a cartesian offset along the x or y axis to a flow-relative
* offset, (e.g. starting from the left in LTR, but right in RTL).
* Finds the flow-relative offset (e.g. starting from the left in LTR, but
* right in RTL) from a layout box.
*/
flowRelativeOffset(layout: Layout, referenceContentLength?: ?number): number {
const {horizontal, rtl} = this._orientation;
Expand All @@ -268,7 +268,10 @@ export default class ListMetricsAggregator {
contentLength != null,
'ListMetricsAggregator must be notified of list content layout before resolving offsets',
);
return contentLength - this._selectOffset(layout);
return (
contentLength -
(this._selectOffset(layout) + this._selectLength(layout))
);
} else {
return this._selectOffset(layout);
}
Expand Down
70 changes: 35 additions & 35 deletions packages/virtualized-lists/Lists/VirtualizedList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,10 +276,23 @@ class VirtualizedList extends StateSafePureComponent<Props, State> {

scrollRef.scrollTo({
animated,
...this._cartesianScrollOffset(offset),
...this._scrollToParamsFromOffset(offset),
});
}

_scrollToParamsFromOffset(offset: number): {x?: number, y?: number} {
const {horizontal, rtl} = this._orientation();
if (horizontal && rtl) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change broke lists in our iOS app in RTL. We heavily rely on horizontal lists and ship to both iOS and Android in LTR and RTL. This line should've been if (horizontal && rtl && Platform.OS !== 'ios') {. We tested that fix in our app and that has resolved things for us. @NickGerleman is Meta using large horizontal lists in any part of its localized RN surfaces? If so, is there something I'm missing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Able to repro native scrollTo not using right coordinate space on iOS old arch. New arch has different scrollview component, and seems to be doing the right thing.

There are a good number of changes since this one, but there was an effort to get all the events/platforms onto consistent coordinate spaces. So, no "if iOS, on Paper, do opposite".

I can make quick PR to try to fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try out this change: https://github.com/facebook/react-native/pull/42094/files

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can confirm that change also fixes it

// Add the visible length of the scrollview so that the offset is right-aligned
const cartOffset = this._listMetrics.cartesianOffset(
offset + this._scrollMetrics.visibleLength,
);
return horizontal ? {x: cartOffset} : {y: cartOffset};
} else {
return horizontal ? {x: offset} : {y: offset};
}
}

recordInteraction() {
this._nestedChildLists.forEach(childList => {
childList.recordInteraction();
Expand Down Expand Up @@ -1480,39 +1493,6 @@ class VirtualizedList extends StateSafePureComponent<Props, State> {
: metrics.width;
}

_flowRelativeScrollOffset(
metrics: $ReadOnly<{
x: number,
y: number,
...
}>,
contentSize: $ReadOnly<{
width: number,
height: number,
...
}>,
): number {
let offset = this._selectOffset(metrics);

const {horizontal, rtl} = this._orientation();
if (horizontal && rtl && Platform.OS !== 'ios') {
offset = this._selectLength(contentSize) - offset;
}

return offset;
}

_cartesianScrollOffset(offset: number): {x?: number, y?: number} {
const {horizontal, rtl} = this._orientation();
const normalizedOffset =
horizontal && rtl && Platform.OS !== 'ios'
? this._listMetrics.getContentLength() - offset
: offset;

const cartOffset = this._listMetrics.cartesianOffset(normalizedOffset);
return horizontal ? {x: cartOffset} : {y: cartOffset};
}

_selectOffset({x, y}: $ReadOnly<{x: number, y: number, ...}>): number {
return this._orientation().horizontal ? x : y;
}
Expand Down Expand Up @@ -1688,7 +1668,7 @@ class VirtualizedList extends StateSafePureComponent<Props, State> {
const timestamp = e.timeStamp;
let visibleLength = this._selectLength(e.nativeEvent.layoutMeasurement);
let contentLength = this._selectLength(e.nativeEvent.contentSize);
let offset = this._flowRelativeScrollOffset(
let offset = this._offsetFromScrollEvent(
e.nativeEvent.contentOffset,
e.nativeEvent.contentSize,
);
Expand Down Expand Up @@ -1755,6 +1735,26 @@ class VirtualizedList extends StateSafePureComponent<Props, State> {
this._scheduleCellsToRenderUpdate();
};

_offsetFromScrollEvent(
contentOffset: $ReadOnly<{
x: number,
y: number,
...
}>,
contentSize: $ReadOnly<{
width: number,
height: number,
...
}>,
): number {
const {horizontal, rtl} = this._orientation();
if (Platform.OS === 'ios' || !(horizontal && rtl)) {
return this._selectOffset(contentOffset);
}

return this._selectLength(contentSize) - this._selectOffset(contentOffset);
}

_scheduleCellsToRenderUpdate(opts?: {allowImmediateExecution?: boolean}) {
const allowImmediateExecution = opts?.allowImmediateExecution ?? true;

Expand Down