From c615e37982217878a35055de8b810b3bb0ba7c46 Mon Sep 17 00:00:00 2001 From: Elias Nahum Date: Mon, 10 Nov 2025 19:18:27 +0800 Subject: [PATCH] Fix gallery image dimensions (#9247) --- app/screens/gallery/index.tsx | 22 ++++------ .../gallery/lightbox_swipeout/lightbox.tsx | 15 +++---- app/utils/images/index.test.ts | 40 +++++++++++++++++++ app/utils/images/index.ts | 12 ++++++ 4 files changed, 64 insertions(+), 25 deletions(-) diff --git a/app/screens/gallery/index.tsx b/app/screens/gallery/index.tsx index 8158de623..f389e86ae 100644 --- a/app/screens/gallery/index.tsx +++ b/app/screens/gallery/index.tsx @@ -2,8 +2,8 @@ // See LICENSE.txt for license information. import RNUtils from '@mattermost/rnutils'; -import React, {useCallback, useEffect, useMemo, useRef, useState} from 'react'; -import {DeviceEventEmitter, Platform, StyleSheet, View} from 'react-native'; +import React, {useCallback, useEffect, useRef, useState} from 'react'; +import {DeviceEventEmitter, Platform, View} from 'react-native'; import {Events} from '@constants'; import useAndroidHardwareBackHandler from '@hooks/android_back_handler'; @@ -28,12 +28,6 @@ type Props = { items: GalleryItemType[]; } -const styles = StyleSheet.create({ - container: { - flex: 1, - }, -}); - const GalleryScreen = ({componentId, galleryIdentifier, hideActions, initialIndex, items}: Props) => { const dim = useWindowDimensions(); const isTablet = useIsTablet(); @@ -41,13 +35,7 @@ const GalleryScreen = ({componentId, galleryIdentifier, hideActions, initialInde const {headerAndFooterHidden, hideHeaderAndFooter, headerStyles, footerStyles} = useGalleryControls(); const galleryRef = useRef(null); - const containerStyle = useMemo(() => { - if (Platform.OS === 'ios') { - return dim; - } - - return styles.container; - }, [dim]); + const containerStyle = dim; const onClose = useCallback(() => { // We keep the un freeze here as we want @@ -81,6 +69,10 @@ const GalleryScreen = ({componentId, galleryIdentifier, hideActions, initialInde onClose(); }); + if (Platform.OS === 'android' && Platform.Version >= 34) { + RNUtils.setNavigationBarColor('black', true); + } + return () => { listener.remove(); }; diff --git a/app/screens/gallery/lightbox_swipeout/lightbox.tsx b/app/screens/gallery/lightbox_swipeout/lightbox.tsx index 745f4aaa4..b6268fdb7 100644 --- a/app/screens/gallery/lightbox_swipeout/lightbox.tsx +++ b/app/screens/gallery/lightbox_swipeout/lightbox.tsx @@ -1,19 +1,14 @@ // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. -// See LICENSE.txt for license information. - import {Image, type ImageSource} from 'expo-image'; import React, {useEffect, useMemo, useState} from 'react'; -import {type ImageStyle, Platform, StyleSheet, View, type ViewStyle} from 'react-native'; +import {type ImageStyle, StyleSheet, View, type ViewStyle} from 'react-native'; import Animated, { interpolate, runOnJS, runOnUI, useAnimatedStyle, withTiming, } from 'react-native-reanimated'; -import {useSafeAreaInsets} from 'react-native-safe-area-context'; -import {useDefaultHeaderHeight} from '@hooks/header'; import {calculateDimensions} from '@utils/images'; import {pagerTimingConfig} from '../animation_config/timing'; @@ -66,9 +61,6 @@ export default function Lightbox({ } = useLightboxSharedValues(); const [renderChildren, setRenderChildren] = useState(false); const childLayoutTimeoutRef = React.useRef(); - const insets = useSafeAreaInsets(); - const headerHeight = useDefaultHeaderHeight() - insets.top; - const targetHeightDiff = Platform.OS === 'ios' ? 0 : (headerHeight / 2); const animateOnMount = () => { 'worklet'; @@ -94,6 +86,9 @@ export default function Lightbox({ childLayoutTimeoutRef.current = undefined; } }; + + // no need to add animateOnMount as this function uses references and is only needed on mount + // eslint-disable-next-line react-hooks/exhaustive-deps }, []); const {width: tw, height: th} = useMemo(() => calculateDimensions( @@ -139,7 +134,7 @@ export default function Lightbox({ interpolate(animationProgress.value, [0, 1], range); const targetX = (targetDimensions.width - tw) / 2; - const targetY = ((targetDimensions.height - th) / 2) - targetHeightDiff; + const targetY = ((targetDimensions.height - th) / 2); const top = translateY.value + diff --git a/app/utils/images/index.test.ts b/app/utils/images/index.test.ts index a355d7aa6..3420c2727 100644 --- a/app/utils/images/index.test.ts +++ b/app/utils/images/index.test.ts @@ -197,6 +197,46 @@ describe('calculateDimensions', () => { expect(result.width).toBe(300); expect(result.height).toBe(300); }); + + it('should constrain portrait image width when scaling to viewport height would cause overflow', () => { + // Portrait image 380×800, viewport 400×900 + // Without fix: height=900, width=900×(380/800)=427.5 (OVERFLOW!) + // With fix: should scale based on width instead + const result = calculateDimensions(800, 380, 400, 900, true); + expect(result.width).toBeLessThanOrEqual(400); + expect(result.height).toBeLessThanOrEqual(900); + + // Should constrain to width: 400w, height=400×(800/380)≈842 + expect(result.width).toBe(400); + expect(result.height).toBeCloseTo(842.1, 0); + }); + + it('should constrain landscape image height when scaling to viewport width would cause overflow', () => { + // Landscape image 800×380, viewport 900×400 + // Without fix: width=900, height=900×(380/800)=427.5 (OVERFLOW!) + // With fix: should scale based on height instead + const result = calculateDimensions(380, 800, 900, 400, true); + expect(result.width).toBeLessThanOrEqual(900); + expect(result.height).toBeLessThanOrEqual(400); + + // Should constrain to height: 400h, width=400×(800/380)≈842 + expect(result.height).toBe(400); + expect(result.width).toBeCloseTo(842.1, 0); + }); + + it('should handle extreme portrait aspect ratio with matchViewPort', () => { + // Very tall portrait image 200×1000 (1:5 ratio), viewport 300×600 + // Would try to fit to height: 600h, width=600×(200/1000)=120 (OK) + // But if viewport is narrower: 100×600 + // Would try: 600h, width=600×(200/1000)=120 (OVERFLOW!) + const result = calculateDimensions(1000, 200, 100, 600, true); + expect(result.width).toBeLessThanOrEqual(100); + expect(result.height).toBeLessThanOrEqual(600); + + // Should constrain to width: 100w, height=100×(1000/200)=500 + expect(result.width).toBe(100); + expect(result.height).toBe(500); + }); }); describe('getViewPortWidth', () => { diff --git a/app/utils/images/index.ts b/app/utils/images/index.ts index f53146129..823727ce7 100644 --- a/app/utils/images/index.ts +++ b/app/utils/images/index.ts @@ -57,10 +57,22 @@ export const calculateDimensions = (height?: number, width?: number, viewPortWid // Portrait: fit to viewport height imageHeight = viewPortHeight; imageWidth = imageHeight * heightRatio; + + // Ensure width doesn't exceed viewport + if (imageWidth > viewPortWidth) { + imageWidth = viewPortWidth; + imageHeight = imageWidth * ratio; + } } else { // Landscape or square: fit to viewport width imageWidth = viewPortWidth; imageHeight = imageWidth * ratio; + + // Ensure height doesn't exceed viewport + if (imageHeight > viewPortHeight) { + imageHeight = viewPortHeight; + imageWidth = imageHeight * heightRatio; + } } }