From ccff129d92c2919b5cf503d352fe73768ec4d971 Mon Sep 17 00:00:00 2001 From: Sergei Sharabai <105950333+sharabai@users.noreply.github.com> Date: Tue, 13 Oct 2026 16:14:32 +0200 Subject: [PATCH 1/6] Hold charts under one spinner, then fade them in --- .../BarChart/VerticalBarChartContent.tsx | 20 +-- .../Charts/BarChart/index.native.tsx | 11 +- src/components/Charts/BarChart/index.tsx | 12 +- .../Charts/LineChart/LineChartContent.tsx | 29 +-- .../Charts/LineChart/index.native.tsx | 11 +- src/components/Charts/LineChart/index.tsx | 12 +- .../Charts/PieChart/PieChartContent.tsx | 17 +- .../Charts/PieChart/index.native.tsx | 11 +- src/components/Charts/PieChart/index.tsx | 11 +- src/components/Charts/SkiaWebChart/index.tsx | 12 +- src/components/Charts/VictoryTheme.ts | 7 + .../Charts/components/ChartFrame.tsx | 28 +++ .../Charts/components/ChartReveal.tsx | 107 +++++++++++ .../Charts/hooks/useReadyCartesianCanvas.ts | 7 +- src/components/Charts/utils/chartHeights.ts | 24 +++ src/pages/home/HomePageSkeleton.tsx | 27 +-- .../InsightsSectionContent.tsx | 10 +- .../useInsightChartPaddingStyle.ts | 15 ++ src/styles/index.ts | 4 +- src/styles/variables.ts | 1 + .../components/Charts/SkiaWebChart.test.tsx | 24 +++ .../components/Charts/chartReveal.test.tsx | 170 ++++++++++++++++++ .../Charts/chartWidthHandoff.test.tsx | 32 +++- tests/unit/components/Charts/utils.test.ts | 34 +++- 24 files changed, 539 insertions(+), 97 deletions(-) create mode 100644 src/components/Charts/components/ChartFrame.tsx create mode 100644 src/components/Charts/components/ChartReveal.tsx create mode 100644 src/components/Charts/utils/chartHeights.ts create mode 100644 src/pages/home/InsightsSection/useInsightChartPaddingStyle.ts create mode 100644 tests/unit/components/Charts/chartReveal.test.tsx diff --git a/src/components/Charts/BarChart/VerticalBarChartContent.tsx b/src/components/Charts/BarChart/VerticalBarChartContent.tsx index 77f584ccec54..6257b9020b5e 100644 --- a/src/components/Charts/BarChart/VerticalBarChartContent.tsx +++ b/src/components/Charts/BarChart/VerticalBarChartContent.tsx @@ -1,4 +1,3 @@ -import ActivityIndicator from '@components/ActivityIndicator'; import {BAR_CORNER_RADIUS, BAR_HIT_GAP_RATIO, VERTICAL_BAR_DOMAIN_PADDING} from '@components/Charts/barChartConstants'; import ChartGridLines from '@components/Charts/components/ChartGridLines'; import ChartTooltipLayer from '@components/Charts/components/ChartTooltipLayer'; @@ -15,7 +14,8 @@ import { useReadyCartesianCanvas, } from '@components/Charts/hooks'; import {getBarLayout, getCartesianPlotBounds, getPrimaryValue, getXAxisLabel, getYAxisLabelWidth} from '@components/Charts/utils'; -import VictoryTheme, {CARTESIAN_Y_AXIS, CHART_CONTENT_MIN_HEIGHT, GLYPH_PADDING} from '@components/Charts/VictoryTheme'; +import {getBarChartHeight, getXAxisLabelSpace} from '@components/Charts/utils/chartHeights'; +import VictoryTheme, {CARTESIAN_Y_AXIS, GLYPH_PADDING} from '@components/Charts/VictoryTheme'; import useStyleUtils from '@hooks/useStyleUtils'; import useTheme from '@hooks/useTheme'; @@ -25,7 +25,6 @@ import variables from '@styles/variables'; import type {CartesianChartRenderArg, ChartBounds, PointsArray, Scale} from 'victory-native'; -import {View} from 'react-native'; import {GestureDetector} from 'react-native-gesture-handler'; import Animated, {useAnimatedStyle, useSharedValue} from 'react-native-reanimated'; import {Bar, CartesianChart} from 'victory-native'; @@ -200,8 +199,8 @@ function VerticalBarChartCanvas({data, series, yAxisUnit, yAxisUnitPosition = 'l ); }; - const labelSpace = shouldShowLabels ? VictoryTheme.axis.xAxisLabelGap + (xAxisLabelHeight ?? 0) : 0; - const chartHeight = CHART_CONTENT_MIN_HEIGHT + labelSpace; + const labelSpace = shouldShowLabels ? getXAxisLabelSpace(xAxisLabelHeight) : 0; + const chartHeight = getBarChartHeight(shouldShowLabels, xAxisLabelHeight); const chartPadding = {...VictoryTheme.axis.padding, bottom: labelSpace + VictoryTheme.axis.padding.bottom, right: chartPaddingRight}; return ( @@ -263,19 +262,10 @@ function VerticalBarChartCanvas({data, series, yAxisUnit, yAxisUnitPosition = 'l } function VerticalBarChartContentBody({isLoading = false, chartWidth, ...canvasProps}: BarChartContentProps) { - const styles = useThemeStyles(); const readyCanvas = useReadyCartesianCanvas(isLoading, chartWidth); - if (!isLoading && canvasProps.data.length === 0) { - return null; - } - if (!readyCanvas) { - return ( - - - - ); + return null; } return ( diff --git a/src/components/Charts/BarChart/index.native.tsx b/src/components/Charts/BarChart/index.native.tsx index 9aea26a2ab8a..e38f40c1a0c8 100644 --- a/src/components/Charts/BarChart/index.native.tsx +++ b/src/components/Charts/BarChart/index.native.tsx @@ -1,4 +1,5 @@ -import ChartWidthBox from '@components/Charts/components/ChartWidthBox'; +import ChartFrame from '@components/Charts/components/ChartFrame'; +import {getBarChartHeight} from '@components/Charts/utils/chartHeights'; import React from 'react'; @@ -8,14 +9,18 @@ import BarChartContent from './BarChartContent'; function BarChart(props: BarChartProps) { return ( - + 0} + loadingHeight={getBarChartHeight(props.shouldShowLabels)} + > {(chartWidth) => ( )} - + ); } diff --git a/src/components/Charts/BarChart/index.tsx b/src/components/Charts/BarChart/index.tsx index e3b165f0a3b5..314aa244cf86 100644 --- a/src/components/Charts/BarChart/index.tsx +++ b/src/components/Charts/BarChart/index.tsx @@ -1,21 +1,27 @@ -import ChartWidthBox from '@components/Charts/components/ChartWidthBox'; +import ChartFrame from '@components/Charts/components/ChartFrame'; import SkiaWebChart from '@components/Charts/SkiaWebChart'; +import {getBarChartHeight} from '@components/Charts/utils/chartHeights'; import React from 'react'; import type BarChartProps from './types'; const getBarChartContent = () => import('./BarChartContent'); + function BarChart(props: BarChartProps) { return ( - + 0} + loadingHeight={getBarChartHeight(props.shouldShowLabels)} + > {(chartWidth) => ( )} - + ); } diff --git a/src/components/Charts/LineChart/LineChartContent.tsx b/src/components/Charts/LineChart/LineChartContent.tsx index 4f3be4254ea1..5c5aa07d4a89 100644 --- a/src/components/Charts/LineChart/LineChartContent.tsx +++ b/src/components/Charts/LineChart/LineChartContent.tsx @@ -1,4 +1,3 @@ -import ActivityIndicator from '@components/ActivityIndicator'; import ActivePointIndicator from '@components/Charts/components/ActivePointIndicator'; import AreaGradient from '@components/Charts/components/AreaGradient'; import ChartGridLines from '@components/Charts/components/ChartGridLines'; @@ -17,16 +16,8 @@ import { useReadyCartesianCanvas, } from '@components/Charts/hooks'; import {getAxisLabelStep, getCartesianPlotBounds, getDomainPaddingForEdgeSpace, getPrimaryValue, getXAxisLabel, getYAxisLabelWidth, labelOverhang} from '@components/Charts/utils'; -import VictoryTheme, { - CARTESIAN_Y_AXIS, - CHART_CONTENT_MIN_HEIGHT, - DASH_INTERVALS, - GLYPH_PADDING, - LABEL_PADDING, - LABEL_ROTATIONS, - SIN_45, - X_AXIS_LABEL_MIN_GAP, -} from '@components/Charts/VictoryTheme'; +import {getCartesianChartHeight, getXAxisLabelSpace} from '@components/Charts/utils/chartHeights'; +import VictoryTheme, {CARTESIAN_Y_AXIS, DASH_INTERVALS, GLYPH_PADDING, LABEL_PADDING, LABEL_ROTATIONS, SIN_45, X_AXIS_LABEL_MIN_GAP} from '@components/Charts/VictoryTheme'; import useStyleUtils from '@hooks/useStyleUtils'; import useTheme from '@hooks/useTheme'; @@ -37,7 +28,6 @@ import variables from '@styles/variables'; import type {CartesianChartRenderArg, ChartBounds, Scale} from 'victory-native'; import {DashPathEffect} from '@shopify/react-native-skia'; -import {View} from 'react-native'; import {GestureDetector} from 'react-native-gesture-handler'; import Animated, {useAnimatedStyle, useDerivedValue, useSharedValue} from 'react-native-reanimated'; import {CartesianChart, Line} from 'victory-native'; @@ -181,7 +171,7 @@ function LineChartCanvas({data, series, yAxisUnit, yAxisUnitPosition = 'left', o plotRight.set(bounds.right); }; - const labelSpace = VictoryTheme.axis.xAxisLabelGap + (xAxisLabelHeight ?? 0); + const labelSpace = getXAxisLabelSpace(xAxisLabelHeight); const isInPlotArea = (args: HitTestArgs) => { 'worklet'; @@ -309,7 +299,7 @@ function LineChartCanvas({data, series, yAxisUnit, yAxisUnitPosition = 'left', o ); }; - const chartHeight = CHART_CONTENT_MIN_HEIGHT + labelSpace; + const chartHeight = getCartesianChartHeight(xAxisLabelHeight); const chartPadding = { ...VictoryTheme.axis.padding, bottom: labelSpace + VictoryTheme.axis.padding.bottom, @@ -371,19 +361,10 @@ function LineChartCanvas({data, series, yAxisUnit, yAxisUnitPosition = 'left', o } function LineChartContentBody({isLoading = false, chartWidth, ...canvasProps}: LineChartContentProps) { - const styles = useThemeStyles(); const readyCanvas = useReadyCartesianCanvas(isLoading, chartWidth); - if (!isLoading && canvasProps.data.length === 0) { - return null; - } - if (!readyCanvas) { - return ( - - - - ); + return null; } return ( diff --git a/src/components/Charts/LineChart/index.native.tsx b/src/components/Charts/LineChart/index.native.tsx index 2fd8ea813de1..9431a4472f89 100644 --- a/src/components/Charts/LineChart/index.native.tsx +++ b/src/components/Charts/LineChart/index.native.tsx @@ -1,4 +1,5 @@ -import ChartWidthBox from '@components/Charts/components/ChartWidthBox'; +import ChartFrame from '@components/Charts/components/ChartFrame'; +import {getCartesianChartHeight} from '@components/Charts/utils/chartHeights'; import React from 'react'; @@ -8,14 +9,18 @@ import LineChartContent from './LineChartContent'; function LineChart(props: LineChartProps) { return ( - + 0} + loadingHeight={getCartesianChartHeight()} + > {(chartWidth) => ( )} - + ); } diff --git a/src/components/Charts/LineChart/index.tsx b/src/components/Charts/LineChart/index.tsx index 01d3da19af53..bc27dc468207 100644 --- a/src/components/Charts/LineChart/index.tsx +++ b/src/components/Charts/LineChart/index.tsx @@ -1,21 +1,27 @@ -import ChartWidthBox from '@components/Charts/components/ChartWidthBox'; +import ChartFrame from '@components/Charts/components/ChartFrame'; import SkiaWebChart from '@components/Charts/SkiaWebChart'; +import {getCartesianChartHeight} from '@components/Charts/utils/chartHeights'; import React from 'react'; import type {LineChartProps} from './LineChartContent'; const getLineChartContent = () => import('./LineChartContent'); + function LineChart(props: LineChartProps) { return ( - + 0} + loadingHeight={getCartesianChartHeight()} + > {(chartWidth) => ( )} - + ); } diff --git a/src/components/Charts/PieChart/PieChartContent.tsx b/src/components/Charts/PieChart/PieChartContent.tsx index 691f22d55310..23efbdb56a65 100644 --- a/src/components/Charts/PieChart/PieChartContent.tsx +++ b/src/components/Charts/PieChart/PieChartContent.tsx @@ -1,4 +1,4 @@ -import ActivityIndicator from '@components/ActivityIndicator'; +import {useReportChartLoading} from '@components/Charts/components/ChartReveal'; import ChartTooltipLayer from '@components/Charts/components/ChartTooltipLayer'; import {TOOLTIP_BAR_GAP, useChartLabelFormats, useTouchPinnedTooltip} from '@components/Charts/hooks'; import type {ChartDataPoint, ChartProps, PieSlice, UnitPosition} from '@components/Charts/types'; @@ -182,6 +182,7 @@ function PieChartCanvas({data, series, valueUnit, valueUnitPosition, onSlicePres // Combined gestures - Race allows both hover and tap to work independently const combinedGesture = Gesture.Race(hoverGesture(), tapGesture()); + // `getPieChartLoadingHeight` reserves this row as one line of normal-size text, so the label has to keep that size. const renderLegendItem = (slice: PieSlice) => { return ( - - - ); - } - return ( + 0} + loadingHeight={getPieChartLoadingHeight(props.shouldShowLegend)} + > {(chartWidth) => ( )} - + ); } diff --git a/src/components/Charts/PieChart/index.tsx b/src/components/Charts/PieChart/index.tsx index 1441e2cdc847..63f909b98139 100644 --- a/src/components/Charts/PieChart/index.tsx +++ b/src/components/Charts/PieChart/index.tsx @@ -1,5 +1,6 @@ -import ChartWidthBox from '@components/Charts/components/ChartWidthBox'; +import ChartFrame from '@components/Charts/components/ChartFrame'; import SkiaWebChart from '@components/Charts/SkiaWebChart'; +import {getPieChartLoadingHeight} from '@components/Charts/utils/chartHeights'; import React from 'react'; @@ -9,14 +10,18 @@ const getPieChartContent = () => import('./PieChartContent'); function PieChart(props: PieChartProps) { return ( - + 0} + loadingHeight={getPieChartLoadingHeight(props.shouldShowLegend)} + > {(chartWidth) => ( )} - + ); } diff --git a/src/components/Charts/SkiaWebChart/index.tsx b/src/components/Charts/SkiaWebChart/index.tsx index 8268fa6de3c9..e315b0bd3f03 100644 --- a/src/components/Charts/SkiaWebChart/index.tsx +++ b/src/components/Charts/SkiaWebChart/index.tsx @@ -1,4 +1,5 @@ import ActivityIndicator from '@components/ActivityIndicator'; +import {useIsInsideChartReveal, useReportChartLoading} from '@components/Charts/components/ChartReveal'; import Icon from '@components/Icon'; import Text from '@components/Text'; @@ -30,8 +31,6 @@ type SkiaWebChartProps = { /** Props forwarded to the lazily-loaded chart component. */ componentProps: TProps; - - /** Identifies the loading skeleton span for telemetry. */ }; type ChartUnavailableProps = { @@ -51,6 +50,9 @@ function ChartUnavailable({description = 'common.webGLNotSupported'}: ChartUnava const {translate} = useLocalize(); const illustrations = useMemoizedLazyIllustrations(['MonitorSyncNo']); + // This state never loads, and Skia draws none of it, so an enclosing ChartReveal shows it at once + useReportChartLoading(false, false); + return ( ({getComponent, componentProps}: SkiaWebChartProps) { const styles = useThemeStyles(); const containerRef = useRef(null); + const isInsideChartReveal = useIsInsideChartReveal(); - // Probe once per mount (not per render) so re-rendering doesn't repeatedly create WebGL contexts, - // while a fresh chart still re-checks capability instead of trusting a stale session-wide result. + // Probed on each mount, so a fresh chart re-checks capability instead of trusting a stale session-wide result. const [isSupported] = useState(() => isSkiaWebSupported()); // The probe can pass while the renderer still ends up without a drawing surface, so also listen for the @@ -111,7 +113,7 @@ function SkiaWebChart({getComponent, componentProps}: Ski return ; } - const fallback = ( + const fallback = isInsideChartReveal ? null : ( diff --git a/src/components/Charts/VictoryTheme.ts b/src/components/Charts/VictoryTheme.ts index 2af60fdd94ad..a2cfa22d163d 100644 --- a/src/components/Charts/VictoryTheme.ts +++ b/src/components/Charts/VictoryTheme.ts @@ -2,6 +2,7 @@ * Centralized styles and layout constants for the chart components. */ import colors from '@styles/theme/colors'; +import variables from '@styles/variables'; import {CHART_FONT_FAMILY_NAMES} from './utils/chartFontConstants'; @@ -100,6 +101,11 @@ const CARTESIAN_Y_AXIS = { /** Minimum height for the chart content area (bars, Y-axis, grid lines) */ const CHART_CONTENT_MIN_HEIGHT = 250; +/** ExpensifyNeue's ascent plus descent over its em, 950 + 230 of 1000 units, which is the line height Skia reports for label text. */ +const LABEL_LINE_HEIGHT_RATIO = 1.18; + +const X_AXIS_LABEL_MIN_HEIGHT = Math.ceil(variables.iconSizeExtraSmall * LABEL_LINE_HEIGHT_RATIO); + /** Pixel height of the y-scale output range inside CartesianChart (content height minus base axis padding). */ const CHART_Y_SCALE_HEIGHT = CHART_CONTENT_MIN_HEIGHT - VictoryTheme.axis.padding.top - VictoryTheme.axis.padding.bottom; @@ -144,6 +150,7 @@ export { CHART_CONTENT_MIN_HEIGHT, X_AXIS_LABEL_MIN_GAP, CHART_Y_SCALE_HEIGHT, + X_AXIS_LABEL_MIN_HEIGHT, LABEL_ROTATIONS, SIN_45, LABEL_PADDING, diff --git a/src/components/Charts/components/ChartFrame.tsx b/src/components/Charts/components/ChartFrame.tsx new file mode 100644 index 000000000000..18ac152cb290 --- /dev/null +++ b/src/components/Charts/components/ChartFrame.tsx @@ -0,0 +1,28 @@ +import type {ReactNode} from 'react'; + +import React from 'react'; + +import ChartReveal from './ChartReveal'; +import ChartWidthBox from './ChartWidthBox'; + +type ChartFrameProps = { + isLoading?: boolean; + hasData: boolean; + + /** Must match the loaded chart's height, or the card jumps at the reveal. */ + loadingHeight: number; + + /** Receives the card's width, or null until the card has been measured. */ + children: (chartWidth: number | null) => ReactNode; +}; + +function ChartFrame({isLoading = false, hasData, loadingHeight, children}: ChartFrameProps) { + // A chart that loads empty collapses at once, so it never starts the engine download or holds a spinner over nothing + if (!isLoading && !hasData) { + return null; + } + + return {(chartWidth) => {children(chartWidth)}}; +} + +export default ChartFrame; diff --git a/src/components/Charts/components/ChartReveal.tsx b/src/components/Charts/components/ChartReveal.tsx new file mode 100644 index 000000000000..5523739fd126 --- /dev/null +++ b/src/components/Charts/components/ChartReveal.tsx @@ -0,0 +1,107 @@ +import ActivityIndicator from '@components/ActivityIndicator'; + +import useStyleUtils from '@hooks/useStyleUtils'; +import useThemeStyles from '@hooks/useThemeStyles'; + +import CONST from '@src/CONST'; + +import type {ReactNode} from 'react'; + +import React, {createContext, useContext, useEffect, useLayoutEffect, useState} from 'react'; +import {View} from 'react-native'; +import Animated, {useAnimatedStyle, useSharedValue, withTiming} from 'react-native-reanimated'; + +/** + * On web, Skia draws a chart 2 to 4 frames after it mounts. + * Counted in frames because a stalled main thread delays the draw by frames, which a timer would not wait for. + */ +const HOLD_FRAMES = 5; + +type ChartLoadState = { + isLoading: boolean; + isDrawnBySkia: boolean; +}; + +const ChartLoadingContext = createContext<((loadState: ChartLoadState) => void) | null>(null); + +function useReportChartLoading(isLoading: boolean, isDrawnBySkia = true) { + const setLoadState = useContext(ChartLoadingContext); + + // Before paint, so no frame shows the chart and the in-flow spinner stacked, or neither of them + useLayoutEffect(() => { + setLoadState?.({isLoading, isDrawnBySkia}); + }, [setLoadState, isLoading, isDrawnBySkia]); +} + +function useIsInsideChartReveal() { + return useContext(ChartLoadingContext) !== null; +} + +type ChartRevealProps = { + /** Must match the loaded chart's height, or the card jumps at the reveal. */ + loadingHeight: number; + + /** Must report through `useReportChartLoading`, or the spinner never goes away. */ + children: ReactNode; +}; + +function ChartReveal({loadingHeight, children}: ChartRevealProps) { + const styles = useThemeStyles(); + const StyleUtils = useStyleUtils(); + const [{isLoading, isDrawnBySkia}, setLoadState] = useState({isLoading: true, isDrawnBySkia: true}); + const [isRevealed, setIsRevealed] = useState(false); + const opacity = useSharedValue(0); + const fadeStyle = useAnimatedStyle(() => ({opacity: opacity.get()})); + + // Adjusted during render, not in the effect below, so the spinner changes in the same commit as what the chart reports + if (isLoading && isRevealed) { + setIsRevealed(false); + } + if (!isLoading && !isDrawnBySkia && !isRevealed) { + setIsRevealed(true); + } + + useEffect(() => { + if (isLoading) { + opacity.set(0); + return; + } + + const fadeIn = () => opacity.set(withTiming(1, {duration: CONST.ANIMATED_TRANSITION})); + if (!isDrawnBySkia) { + fadeIn(); + return; + } + + let framesLeft = HOLD_FRAMES; + let frameID = 0; + const countFrame = () => { + framesLeft -= 1; + if (framesLeft > 0) { + frameID = requestAnimationFrame(countFrame); + return; + } + setIsRevealed(true); + fadeIn(); + }; + frameID = requestAnimationFrame(countFrame); + + return () => cancelAnimationFrame(frameID); + }, [isLoading, isDrawnBySkia, opacity]); + + return ( + + + {children} + {!isRevealed && ( + + + + )} + + + ); +} + +export default ChartReveal; +export {HOLD_FRAMES, useIsInsideChartReveal, useReportChartLoading}; diff --git a/src/components/Charts/hooks/useReadyCartesianCanvas.ts b/src/components/Charts/hooks/useReadyCartesianCanvas.ts index 13e7d3288bfa..558e5903f3b6 100644 --- a/src/components/Charts/hooks/useReadyCartesianCanvas.ts +++ b/src/components/Charts/hooks/useReadyCartesianCanvas.ts @@ -1,3 +1,4 @@ +import {useReportChartLoading} from '@components/Charts/components/ChartReveal'; import {useChartFontManager} from '@components/Charts/context/ChartFontsContext'; import type {SkTypefaceFontProvider} from '@shopify/react-native-skia'; @@ -10,7 +11,11 @@ type CartesianCanvasInputs = { function useReadyCartesianCanvas(isLoading: boolean, chartWidth: number | null): CartesianCanvasInputs | null { const fontManager = useChartFontManager(); - if (isLoading || !fontManager || chartWidth === null) { + // Until the width is measured there is no canvas for Skia to draw, so the reveal must not start counting frames yet + const isChartLoading = isLoading || !fontManager || chartWidth === null; + useReportChartLoading(isChartLoading); + + if (isChartLoading) { return null; } diff --git a/src/components/Charts/utils/chartHeights.ts b/src/components/Charts/utils/chartHeights.ts new file mode 100644 index 000000000000..a4b3e658ce0c --- /dev/null +++ b/src/components/Charts/utils/chartHeights.ts @@ -0,0 +1,24 @@ +import VictoryTheme, {CHART_CONTENT_MIN_HEIGHT, X_AXIS_LABEL_MIN_HEIGHT} from '@components/Charts/VictoryTheme'; + +import variables from '@styles/variables'; + +// Imported before the chart engine has loaded, so this file must not import Skia. + +function getXAxisLabelSpace(xAxisLabelHeight = 0): number { + return VictoryTheme.axis.xAxisLabelGap + Math.max(xAxisLabelHeight, X_AXIS_LABEL_MIN_HEIGHT); +} + +function getCartesianChartHeight(xAxisLabelHeight = 0): number { + return CHART_CONTENT_MIN_HEIGHT + getXAxisLabelSpace(xAxisLabelHeight); +} + +function getBarChartHeight(shouldShowLabels = true, xAxisLabelHeight = 0): number { + return shouldShowLabels ? getCartesianChartHeight(xAxisLabelHeight) : CHART_CONTENT_MIN_HEIGHT; +} + +function getPieChartLoadingHeight(shouldShowLegend = true): number { + const legendFirstRowHeight = shouldShowLegend ? variables.pieChartLegendMargin + variables.fontSizeNormalHeight : 0; + return CHART_CONTENT_MIN_HEIGHT + legendFirstRowHeight; +} + +export {getXAxisLabelSpace, getCartesianChartHeight, getBarChartHeight, getPieChartLoadingHeight}; diff --git a/src/pages/home/HomePageSkeleton.tsx b/src/pages/home/HomePageSkeleton.tsx index 9aeeaf39f70e..48d70e0289ce 100644 --- a/src/pages/home/HomePageSkeleton.tsx +++ b/src/pages/home/HomePageSkeleton.tsx @@ -1,5 +1,5 @@ import ActivityIndicator from '@components/ActivityIndicator'; -import {CHART_CONTENT_MIN_HEIGHT} from '@components/Charts/VictoryTheme'; +import {getCartesianChartHeight} from '@components/Charts/utils/chartHeights'; import SkeletonRect from '@components/SkeletonRect'; import ItemListSkeletonView from '@components/Skeletons/ItemListSkeletonView'; import SkeletonTextLine, {BAR_HEIGHT} from '@components/Skeletons/SkeletonTextLine'; @@ -19,6 +19,7 @@ import React from 'react'; import {View} from 'react-native'; import {ICON_SLOT_SIZE, useWidgetSkeletonRowGeometry, WidgetSkeletonRowIcon} from './common/widgetSkeletonRow'; +import useInsightChartPaddingStyle from './InsightsSection/useInsightChartPaddingStyle'; // The stacked pair of text lines the two-bar rows stand in for: a merchant line over a muted label line. const FIRST_LINE_HEIGHT = lineHeightScale.text; @@ -29,8 +30,8 @@ const TRAILING_SUB_BAR_WIDTH = 40; const CARD_TITLE_WIDTH = 120; const LOWER_BAR_WIDTH = 80; -// The chart this stands in for holds its own loading spinner at exactly this height. -const SPINNER_CARD_HEIGHT = CHART_CONTENT_MIN_HEIGHT; +// Home's first insight, spend over time, is a line chart, which holds its loading spinner at this height. +const SPINNER_CARD_HEIGHT = getCartesianChartHeight(); const ROWS_PER_LIST_CARD = 3; const ROWS_PER_TABLE_CARD = CONST.HOME.SECTION_VISIBLE_LIMIT; @@ -108,9 +109,10 @@ function renderIconTwoBarWithTrailingRow(args: TrailingSkeletonRowArgs) { type HomePageSkeletonCardProps = { children: React.ReactNode; + hasBottomPadding?: boolean; }; -function HomePageSkeletonCard({children}: HomePageSkeletonCardProps) { +function HomePageSkeletonCard({children, hasBottomPadding = true}: HomePageSkeletonCardProps) { const styles = useThemeStyles(); const {shouldUseNarrowLayout} = useResponsiveLayout(); @@ -123,7 +125,7 @@ function HomePageSkeletonCard({children}: HomePageSkeletonCardProps) { barWidth={CARD_TITLE_WIDTH} /> } - containerStyles={styles.getWidgetContainerBottomPaddingStyle(shouldUseNarrowLayout)} + containerStyles={hasBottomPadding && styles.getWidgetContainerBottomPaddingStyle(shouldUseNarrowLayout)} > {children} @@ -176,14 +178,17 @@ function HomePageSkeletonTableCard() { function HomePageSkeletonSpinnerCard() { const styles = useThemeStyles(); const StyleUtils = useStyleUtils(); + const chartPaddingStyle = useInsightChartPaddingStyle(); return ( - - - + + + + + ); diff --git a/src/pages/home/InsightsSection/InsightsSectionContent.tsx b/src/pages/home/InsightsSection/InsightsSectionContent.tsx index 9dbaeec3fa45..9100b9157a26 100644 --- a/src/pages/home/InsightsSection/InsightsSectionContent.tsx +++ b/src/pages/home/InsightsSection/InsightsSectionContent.tsx @@ -6,13 +6,10 @@ import SearchChartView from '@components/Search/SearchChartView'; import WidgetContainer from '@components/WidgetContainer'; import WidgetHeaderMenu from '@components/WidgetHeaderMenu'; -import useLayoutSpacing from '@hooks/useLayoutSpacing'; import {useMemoizedLazyExpensifyIcons} from '@hooks/useLazyAsset'; import useLocalize from '@hooks/useLocalize'; import useOnyx from '@hooks/useOnyx'; import usePermissions from '@hooks/usePermissions'; -import useResponsiveLayout from '@hooks/useResponsiveLayout'; -import useThemeStyles from '@hooks/useThemeStyles'; import {setNameValuePair} from '@libs/actions/User'; import {canUseTouchScreen} from '@libs/DeviceCapabilities'; @@ -29,14 +26,13 @@ import {View} from 'react-native'; import InsightTitleDropdown from './InsightTitleDropdown'; import useHomeInsightConfigs from './useHomeInsightConfigs'; +import useInsightChartPaddingStyle from './useInsightChartPaddingStyle'; import useInsightData from './useInsightData'; function InsightsSectionContent() { - const styles = useThemeStyles(); - const {cardPaddingHorizontal, cardPaddingBottom} = useLayoutSpacing(); + const chartPaddingStyle = useInsightChartPaddingStyle(); const {translate} = useLocalize(); const icons = useMemoizedLazyExpensifyIcons(['Expand']); - const {shouldUseNarrowLayout} = useResponsiveLayout(); const {isBetaEnabled} = usePermissions(); const isInsightsPageEnabled = isBetaEnabled(CONST.BETAS.INSIGHTS_PAGE); @@ -93,7 +89,7 @@ function InsightsSectionContent() { {state === INSIGHTS_CHART_STATE.EMPTY && } {state === INSIGHTS_CHART_STATE.ERROR && } {(state === INSIGHTS_CHART_STATE.LOADING || state === INSIGHTS_CHART_STATE.READY) && ( - + { + const styles = useThemeStyles(); + const {cardPaddingHorizontal, cardPaddingBottom} = useLayoutSpacing(); + const {shouldUseNarrowLayout} = useResponsiveLayout(); + + return [cardPaddingHorizontal, cardPaddingBottom, !shouldUseNarrowLayout && styles.pt3]; +} + +export default useInsightChartPaddingStyle; diff --git a/src/styles/index.ts b/src/styles/index.ts index 447f31c0a549..bca434e5313d 100644 --- a/src/styles/index.ts +++ b/src/styles/index.ts @@ -6984,8 +6984,8 @@ const staticStyles = (theme: ThemeColors) => display: 'flex', justifyContent: 'center', alignItems: 'center', - marginHorizontal: variables.qrShareHorizontalPadding, - marginTop: variables.qrShareHorizontalPadding, + marginHorizontal: variables.pieChartLegendMargin, + marginTop: variables.pieChartLegendMargin, flexDirection: 'row', flexWrap: 'wrap', rowGap: 8, diff --git a/src/styles/variables.ts b/src/styles/variables.ts index 72376ce47d4f..60d96a00b16b 100644 --- a/src/styles/variables.ts +++ b/src/styles/variables.ts @@ -581,4 +581,5 @@ export default { editableCellEditButtonWidth: 28 + 4, // How short a value has to be before that edit button is worth reserving room for. narrowEditableContentWidth: 60, + pieChartLegendMargin: 32, } as const; diff --git a/tests/unit/components/Charts/SkiaWebChart.test.tsx b/tests/unit/components/Charts/SkiaWebChart.test.tsx index 01a592718d5c..7f519ddeb65a 100644 --- a/tests/unit/components/Charts/SkiaWebChart.test.tsx +++ b/tests/unit/components/Charts/SkiaWebChart.test.tsx @@ -1,7 +1,10 @@ import {render, screen} from '@testing-library/react-native'; +import ActivityIndicator from '@components/ActivityIndicator'; +import ChartReveal from '@components/Charts/components/ChartReveal'; import SkiaWebChart from '@components/Charts/SkiaWebChart'; import isSkiaWebSupported from '@components/Charts/SkiaWebChart/isSkiaWebSupported'; +import {getCartesianChartHeight} from '@components/Charts/utils/chartHeights'; import * as Sentry from '@sentry/react-native'; import {WithSkiaWeb} from '@shopify/react-native-skia/lib/module/web'; @@ -27,6 +30,8 @@ jest.mock('@components/Icon', () => ({ default: () => null, })); +const CHART_LOADING_HEIGHT = getCartesianChartHeight(); + const mockIsSkiaWebSupported = jest.mocked(isSkiaWebSupported); const mockWithSkiaWeb = jest.mocked(WithSkiaWeb); const mockCaptureException = jest.mocked(Sentry.captureException); @@ -53,6 +58,25 @@ describe('SkiaWebChart', () => { expect(mockWithSkiaWeb).not.toHaveBeenCalled(); }); + it('should show the unavailable message under a reveal without holding its spinner', () => { + // Given a browser without WebGL + mockIsSkiaWebSupported.mockReturnValue(false); + + // When the chart renders under a reveal + render( + + + , + ); + + // Then the message replaces the spinner in the same render, because the hold waits for a Skia draw this state never makes + expect(screen.UNSAFE_queryByType(ActivityIndicator)).toBeNull(); + expect(screen.getByText('common.unableToDisplayChart')).toBeTruthy(); + }); + it('should mount Skia when WebGL is supported', () => { mockIsSkiaWebSupported.mockReturnValue(true); diff --git a/tests/unit/components/Charts/chartReveal.test.tsx b/tests/unit/components/Charts/chartReveal.test.tsx new file mode 100644 index 000000000000..4a2eb933b990 --- /dev/null +++ b/tests/unit/components/Charts/chartReveal.test.tsx @@ -0,0 +1,170 @@ +import {act, render, screen} from '@testing-library/react-native'; + +import ActivityIndicator from '@components/ActivityIndicator'; +import ChartReveal, {HOLD_FRAMES, useReportChartLoading} from '@components/Charts/components/ChartReveal'; +import {getCartesianChartHeight} from '@components/Charts/utils/chartHeights'; +import Text from '@components/Text'; + +import React from 'react'; + +const LOADING_HEIGHT = getCartesianChartHeight(); + +let pendingFrames = new Map(); +let lastFrameID = 0; + +function runFrames(count: number) { + for (let frame = 0; frame < count; frame++) { + const callbacks = [...pendingFrames.values()]; + pendingFrames.clear(); + act(() => { + for (const callback of callbacks) { + callback(0); + } + }); + } +} + +type ChartBodyProps = { + isLoading: boolean; + label: string; +}; + +function ChartBody({isLoading, label}: ChartBodyProps) { + useReportChartLoading(isLoading); + return isLoading ? null : {label}; +} + +function EngineDownloading() { + return null; +} + +function renderChart(isLoading: boolean, label = 'chart') { + return ( + + + + ); +} + +describe('ChartReveal', () => { + beforeEach(() => { + pendingFrames = new Map(); + jest.spyOn(global, 'requestAnimationFrame').mockImplementation((callback) => { + lastFrameID += 1; + pendingFrames.set(lastFrameID, callback); + return lastFrameID; + }); + jest.spyOn(global, 'cancelAnimationFrame').mockImplementation((frameID) => { + if (frameID === null || frameID === undefined) { + return; + } + pendingFrames.delete(frameID); + }); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('should keep the spinner over the mounted chart until the last held frame', () => { + // Given a chart whose data has just become ready + render(renderChart(false)); + + // When every held frame but the last passes + runFrames(HOLD_FRAMES - 1); + + // Then the chart is mounted but still covered, because Skia may not have drawn it yet + expect(screen.getByText('chart')).toBeTruthy(); + expect(screen.UNSAFE_queryByType(ActivityIndicator)).not.toBeNull(); + + // When the last held frame passes + runFrames(1); + + // Then the spinner goes away, since the hold has outlasted Skia's first draw + expect(screen.UNSAFE_queryByType(ActivityIndicator)).toBeNull(); + }); + + it('should keep one spinner from the engine download until the reveal', () => { + // Given a chart whose engine is still downloading, so nothing below has reported yet + const {rerender} = render( + + + , + ); + const downloadSpinner = screen.UNSAFE_getByType(ActivityIndicator); + runFrames(HOLD_FRAMES); + + // When the chart replaces the download placeholder, loads, and then gets its data + rerender(renderChart(true)); + const loadingSpinner = screen.UNSAFE_getByType(ActivityIndicator); + rerender(renderChart(false)); + + // Then the spinner covering the hold is the one shown during the download, + // because a new one would restart its rotation at each of those steps + expect(loadingSpinner).toBe(downloadSpinner); + expect(screen.UNSAFE_getByType(ActivityIndicator)).toBe(downloadSpinner); + }); + + it('should not hold again when a revealed chart gets new data', () => { + // Given a chart that has been revealed + const {rerender} = render(renderChart(false)); + runFrames(HOLD_FRAMES); + + // When its data changes without it loading again + rerender(renderChart(false, 'updated chart')); + + // Then it updates in place, since Skia redraws a mounted chart without a blank frame + expect(screen.getByText('updated chart')).toBeTruthy(); + expect(screen.UNSAFE_queryByType(ActivityIndicator)).toBeNull(); + }); + + it('should hold again when a revealed chart reloads', () => { + // Given a chart that has been revealed + const {rerender} = render(renderChart(false)); + runFrames(HOLD_FRAMES); + + // When it loads again and its new data arrives + rerender(renderChart(true)); + rerender(renderChart(false)); + + // Then the remounted chart is covered again, because Skia has to draw it from scratch + expect(screen.getByText('chart')).toBeTruthy(); + expect(screen.UNSAFE_queryByType(ActivityIndicator)).not.toBeNull(); + }); + + it('should restart the hold when a chart reloads before its hold ends', () => { + // Given a chart partway through its hold + const {rerender} = render(renderChart(false)); + runFrames(HOLD_FRAMES - 2); + + // When it loads again and its new data arrives + rerender(renderChart(true)); + rerender(renderChart(false)); + runFrames(HOLD_FRAMES - 1); + + // Then it is still covered one frame short of a full hold, because the frames counted before the reload + // belonged to a chart Skia no longer shows + expect(screen.UNSAFE_queryByType(ActivityIndicator)).not.toBeNull(); + + // When the last frame of the new hold passes + runFrames(1); + + // Then the spinner goes away, since the new hold has outlasted Skia's first draw of the remounted chart + expect(screen.UNSAFE_queryByType(ActivityIndicator)).toBeNull(); + }); + + it('should stop counting frames when the chart unmounts during its hold', () => { + // Given a chart partway through its hold + const {unmount} = render(renderChart(false)); + runFrames(1); + + // When it unmounts, as it does when the user leaves the page + unmount(); + + // Then no frame is left waiting to reveal it, since that would update a component that is gone + expect(pendingFrames.size).toBe(0); + }); +}); diff --git a/tests/unit/components/Charts/chartWidthHandoff.test.tsx b/tests/unit/components/Charts/chartWidthHandoff.test.tsx index b34aefe53c3e..38eac5d6ec37 100644 --- a/tests/unit/components/Charts/chartWidthHandoff.test.tsx +++ b/tests/unit/components/Charts/chartWidthHandoff.test.tsx @@ -59,6 +59,33 @@ describe('chart width handoff', () => { expect(getMeasuredBox()).toBeTruthy(); }); + it.each([ + ['bar', BarChart], + ['line', LineChart], + ['pie', PieChart], + ])('should collapse the %s chart as soon as it loads with no data', (_name, Chart) => { + // Given a loading chart + const {rerender} = render( + , + ); + + // When it finishes loading with nothing to draw + rerender( + , + ); + + // Then the spinner goes in the same render, because holding it for Skia's first draw would leave it over an empty box + expect(screen.UNSAFE_queryByType(ActivityIndicator)).toBeNull(); + }); + it('should hand the pie chart the width measured while it was loading', () => { // Given a loading pie chart whose box has been measured const {rerender} = render( @@ -94,7 +121,8 @@ describe('chart width handoff', () => { />, ); - // Then the spinner stays and the chart does not mount, because it can only be sized once the width is known + // Then the chart waits for the width instead of measuring itself, because the reveal counts its held frames + // from the render the chart reports ready in, and a chart that measures itself draws later than that expect(PolarChart).not.toHaveBeenCalled(); expect(screen.UNSAFE_queryByType(ActivityIndicator)).not.toBeNull(); }); @@ -115,7 +143,7 @@ describe('chart width handoff', () => { fireEvent(measuredBox, 'layout', {nativeEvent: {layout: {width: 0, height: 0}}}); // Then the chart stays mounted at its last width instead of falling back to the spinner, which would drop its hover state and rebuild its canvas - expect(screen.UNSAFE_queryByType(ActivityIndicator)).toBeNull(); + expect(screen.UNSAFE_queryByType(PolarChart)).not.toBeNull(); expect(PolarChart).toHaveBeenLastCalledWith(expect.objectContaining({explicitSize: {width: CONTAINER_WIDTH, height: CHART_CONTENT_MIN_HEIGHT}}), undefined); }); }); diff --git a/tests/unit/components/Charts/utils.test.ts b/tests/unit/components/Charts/utils.test.ts index a7e0ff3c83d5..090c00f8e959 100644 --- a/tests/unit/components/Charts/utils.test.ts +++ b/tests/unit/components/Charts/utils.test.ts @@ -23,7 +23,15 @@ import { rotatedLabelYOffset, truncateLabel, } from '@components/Charts/utils'; -import VictoryTheme, {CHART_Y_SCALE_HEIGHT, DIAGONAL_ANGLE_RADIAN_THRESHOLD, LABEL_ROTATIONS, SIN_45} from '@components/Charts/VictoryTheme'; +import {getCartesianChartHeight} from '@components/Charts/utils/chartHeights'; +import VictoryTheme, { + CHART_CONTENT_MIN_HEIGHT, + CHART_Y_SCALE_HEIGHT, + DIAGONAL_ANGLE_RADIAN_THRESHOLD, + LABEL_ROTATIONS, + SIN_45, + X_AXIS_LABEL_MIN_HEIGHT, +} from '@components/Charts/VictoryTheme'; import {CHART_SERIES_KEY} from '@components/Search/buildChartSeries'; const LINE_HEIGHT = 16; @@ -755,6 +763,30 @@ describe('getBarLayout', () => { }); }); +describe('getCartesianChartHeight', () => { + const ROTATED_LABEL_HEIGHT = 120; + + it('should give the loading box and a chart with one line of labels the same height', () => { + // Given the loading box, which has no labels to measure, and a loaded chart whose labels fit on one line + const loadingHeight = getCartesianChartHeight(); + + // When the loaded chart sizes its box from the measured label strip + const loadedHeight = getCartesianChartHeight(X_AXIS_LABEL_MIN_HEIGHT - 1); + + // Then both heights match, so the card does not jump when the chart replaces the spinner + expect(loadedHeight).toBe(loadingHeight); + }); + + it('should grow the box when rotated labels need more room than one line', () => { + // Given labels rotated so their strip is taller than one line of text + // When the chart sizes its box + const height = getCartesianChartHeight(ROTATED_LABEL_HEIGHT); + + // Then the box makes room for the whole strip, since clipping the labels would be worse than a jump + expect(height).toBe(CHART_CONTENT_MIN_HEIGHT + VictoryTheme.axis.xAxisLabelGap + ROTATED_LABEL_HEIGHT); + }); +}); + describe('getCartesianPlotBounds', () => { const PADDING_LEFT = VictoryTheme.axis.padding.left; From b964b86efbf445927d1009d45c1405f08ba8dba4 Mon Sep 17 00:00:00 2001 From: Sergei Sharabai <105950333+sharabai@users.noreply.github.com> Date: Wed, 14 Oct 2026 18:14:09 +0200 Subject: [PATCH 2/6] Skip the chart spinner when only the width is pending --- .../Charts/PieChart/PieChartContent.tsx | 6 +- src/components/Charts/SkiaWebChart/index.tsx | 13 +++- .../Charts/components/ChartFrame.tsx | 13 +++- .../Charts/components/ChartReveal.tsx | 34 +++++++---- .../Charts/hooks/useReadyCartesianCanvas.ts | 5 +- .../components/Charts/SkiaWebChart.test.tsx | 59 +++++++++++++++---- .../components/Charts/chartReveal.test.tsx | 56 +++++++++++++----- .../Charts/chartWidthHandoff.test.tsx | 22 ++++--- 8 files changed, 154 insertions(+), 54 deletions(-) diff --git a/src/components/Charts/PieChart/PieChartContent.tsx b/src/components/Charts/PieChart/PieChartContent.tsx index 23efbdb56a65..334ec81cd997 100644 --- a/src/components/Charts/PieChart/PieChartContent.tsx +++ b/src/components/Charts/PieChart/PieChartContent.tsx @@ -270,11 +270,9 @@ function PieChartCanvas({data, series, valueUnit, valueUnitPosition, onSlicePres } function PieChartContent({isLoading = false, chartWidth, ...canvasProps}: PieChartContentProps) { - // Until the width is measured there is no canvas for Skia to draw, so the reveal must not start counting frames yet - const isChartLoading = isLoading || chartWidth === null; - useReportChartLoading(isChartLoading); + useReportChartLoading(isLoading); - if (isChartLoading) { + if (isLoading || chartWidth === null) { return null; } diff --git a/src/components/Charts/SkiaWebChart/index.tsx b/src/components/Charts/SkiaWebChart/index.tsx index e315b0bd3f03..e107f241aca1 100644 --- a/src/components/Charts/SkiaWebChart/index.tsx +++ b/src/components/Charts/SkiaWebChart/index.tsx @@ -66,6 +66,15 @@ function ChartUnavailable({description = 'common.webGLNotSupported'}: ChartUnava ); } +/** + * Draws nothing, so an enclosing ChartReveal keeps its spinner up while Skia loads. + * Reports loading so the reveal also holds that spinner until Skia has drawn the chart that replaces this. + */ +function ChartSkiaLoading() { + useReportChartLoading(true); + return null; +} + function ChartFailedToLoad() { return ; } @@ -113,7 +122,9 @@ function SkiaWebChart({getComponent, componentProps}: Ski return ; } - const fallback = isInsideChartReveal ? null : ( + const fallback = isInsideChartReveal ? ( + + ) : ( diff --git a/src/components/Charts/components/ChartFrame.tsx b/src/components/Charts/components/ChartFrame.tsx index 18ac152cb290..13a691fef718 100644 --- a/src/components/Charts/components/ChartFrame.tsx +++ b/src/components/Charts/components/ChartFrame.tsx @@ -22,7 +22,18 @@ function ChartFrame({isLoading = false, hasData, loadingHeight, children}: Chart return null; } - return {(chartWidth) => {children(chartWidth)}}; + return ( + + {(chartWidth) => ( + + {children(chartWidth)} + + )} + + ); } export default ChartFrame; diff --git a/src/components/Charts/components/ChartReveal.tsx b/src/components/Charts/components/ChartReveal.tsx index 5523739fd126..3216e1fbdcda 100644 --- a/src/components/Charts/components/ChartReveal.tsx +++ b/src/components/Charts/components/ChartReveal.tsx @@ -12,7 +12,7 @@ import {View} from 'react-native'; import Animated, {useAnimatedStyle, useSharedValue, withTiming} from 'react-native-reanimated'; /** - * On web, Skia draws a chart 2 to 4 frames after it mounts. + * Skia draws a chart a few frames after it mounts: 2 to 4 on web. * Counted in frames because a stalled main thread delays the draw by frames, which a timer would not wait for. */ const HOLD_FRAMES = 5; @@ -41,34 +41,48 @@ type ChartRevealProps = { /** Must match the loaded chart's height, or the card jumps at the reveal. */ loadingHeight: number; + /** Until the card is measured there is no canvas for Skia to draw, so the chart is not ready to reveal. */ + isCardMeasured: boolean; + /** Must report through `useReportChartLoading`, or the spinner never goes away. */ children: ReactNode; }; -function ChartReveal({loadingHeight, children}: ChartRevealProps) { +function ChartReveal({loadingHeight, isCardMeasured, children}: ChartRevealProps) { const styles = useThemeStyles(); const StyleUtils = useStyleUtils(); - const [{isLoading, isDrawnBySkia}, setLoadState] = useState({isLoading: true, isDrawnBySkia: true}); + const [loadState, setLoadState] = useState(null); + const [hasReportedLoading, setHasReportedLoading] = useState(false); const [isRevealed, setIsRevealed] = useState(false); const opacity = useSharedValue(0); const fadeStyle = useAnimatedStyle(() => ({opacity: opacity.get()})); + const isReady = !!loadState && !loadState.isLoading && isCardMeasured; + + // A chart that mounts with everything in memory waits only for its card's width, which lands within a frame or two, + // so it shows no spinner and fades in at once. Only a chart that has shown the spinner holds it until Skia has drawn. + const isSpinnerShown = !loadState || hasReportedLoading; + const shouldHold = hasReportedLoading && !!loadState?.isDrawnBySkia; + // Adjusted during render, not in the effect below, so the spinner changes in the same commit as what the chart reports - if (isLoading && isRevealed) { + if (loadState?.isLoading && !hasReportedLoading) { + setHasReportedLoading(true); + } + if (!isReady && isRevealed) { setIsRevealed(false); } - if (!isLoading && !isDrawnBySkia && !isRevealed) { + if (isReady && !shouldHold && !isRevealed) { setIsRevealed(true); } useEffect(() => { - if (isLoading) { + if (!isReady) { opacity.set(0); return; } const fadeIn = () => opacity.set(withTiming(1, {duration: CONST.ANIMATED_TRANSITION})); - if (!isDrawnBySkia) { + if (!shouldHold) { fadeIn(); return; } @@ -87,15 +101,15 @@ function ChartReveal({loadingHeight, children}: ChartRevealProps) { frameID = requestAnimationFrame(countFrame); return () => cancelAnimationFrame(frameID); - }, [isLoading, isDrawnBySkia, opacity]); + }, [isReady, shouldHold, opacity]); return ( {children} {!isRevealed && ( - - + + {isSpinnerShown && } )} diff --git a/src/components/Charts/hooks/useReadyCartesianCanvas.ts b/src/components/Charts/hooks/useReadyCartesianCanvas.ts index 558e5903f3b6..ab8ab352b666 100644 --- a/src/components/Charts/hooks/useReadyCartesianCanvas.ts +++ b/src/components/Charts/hooks/useReadyCartesianCanvas.ts @@ -11,11 +11,10 @@ type CartesianCanvasInputs = { function useReadyCartesianCanvas(isLoading: boolean, chartWidth: number | null): CartesianCanvasInputs | null { const fontManager = useChartFontManager(); - // Until the width is measured there is no canvas for Skia to draw, so the reveal must not start counting frames yet - const isChartLoading = isLoading || !fontManager || chartWidth === null; + const isChartLoading = isLoading || !fontManager; useReportChartLoading(isChartLoading); - if (isChartLoading) { + if (isChartLoading || chartWidth === null) { return null; } diff --git a/tests/unit/components/Charts/SkiaWebChart.test.tsx b/tests/unit/components/Charts/SkiaWebChart.test.tsx index 7f519ddeb65a..ffbf1eda6066 100644 --- a/tests/unit/components/Charts/SkiaWebChart.test.tsx +++ b/tests/unit/components/Charts/SkiaWebChart.test.tsx @@ -1,7 +1,7 @@ import {render, screen} from '@testing-library/react-native'; import ActivityIndicator from '@components/ActivityIndicator'; -import ChartReveal from '@components/Charts/components/ChartReveal'; +import ChartReveal, {useReportChartLoading} from '@components/Charts/components/ChartReveal'; import SkiaWebChart from '@components/Charts/SkiaWebChart'; import isSkiaWebSupported from '@components/Charts/SkiaWebChart/isSkiaWebSupported'; import {getCartesianChartHeight} from '@components/Charts/utils/chartHeights'; @@ -9,6 +9,7 @@ import {getCartesianChartHeight} from '@components/Charts/utils/chartHeights'; import * as Sentry from '@sentry/react-native'; import {WithSkiaWeb} from '@shopify/react-native-skia/lib/module/web'; import React from 'react'; +import {View} from 'react-native'; jest.mock('@components/Charts/SkiaWebChart/isSkiaWebSupported', () => jest.fn()); @@ -36,9 +37,26 @@ const mockIsSkiaWebSupported = jest.mocked(isSkiaWebSupported); const mockWithSkiaWeb = jest.mocked(WithSkiaWeb); const mockCaptureException = jest.mocked(Sentry.captureException); +function ReadyChart() { + useReportChartLoading(false); + return null; +} + describe('SkiaWebChart', () => { const getComponent = () => Promise.resolve({default: () => null}); + const renderRevealedChart = () => ( + + + + ); + afterEach(() => { jest.clearAllMocks(); }); @@ -58,23 +76,38 @@ describe('SkiaWebChart', () => { expect(mockWithSkiaWeb).not.toHaveBeenCalled(); }); - it('should show the unavailable message under a reveal without holding its spinner', () => { - // Given a browser without WebGL - mockIsSkiaWebSupported.mockReturnValue(false); + it('should show the failed-to-load message under a reveal without holding its spinner', () => { + // Given a chart under a reveal whose spinner is up while Skia loads + mockIsSkiaWebSupported.mockReturnValue(true); + mockWithSkiaWeb.mockImplementation(({fallback}) => {fallback}); + const {rerender} = render(renderRevealedChart()); - // When the chart renders under a reveal - render( - - - , - ); + // When CanvasKit fails to load. React logs caught render errors to console.error, so mock it to keep the test output clean. + mockWithSkiaWeb.mockImplementation(() => { + throw new Error('CanvasKit initialized without its bindings'); + }); + const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + rerender(renderRevealedChart()); // Then the message replaces the spinner in the same render, because the hold waits for a Skia draw this state never makes expect(screen.UNSAFE_queryByType(ActivityIndicator)).toBeNull(); expect(screen.getByText('common.unableToDisplayChart')).toBeTruthy(); + + consoleErrorSpy.mockRestore(); + }); + + it('should hold the reveal spinner when the chart was still loading Skia at mount', () => { + // Given a browser with WebGL where Skia has not loaded yet, so the chart shows its fallback under a reveal + mockIsSkiaWebSupported.mockReturnValue(true); + mockWithSkiaWeb.mockImplementation(({fallback}) => {fallback}); + const {rerender} = render(renderRevealedChart()); + + // When Skia loads and the chart reports it is ready + mockWithSkiaWeb.mockImplementation(() => ); + rerender(renderRevealedChart()); + + // Then the spinner the fallback put up stays for the hold, because taking it down before Skia draws would show an empty card + expect(screen.UNSAFE_queryByType(ActivityIndicator)).not.toBeNull(); }); it('should mount Skia when WebGL is supported', () => { diff --git a/tests/unit/components/Charts/chartReveal.test.tsx b/tests/unit/components/Charts/chartReveal.test.tsx index 4a2eb933b990..64b276f1bbfa 100644 --- a/tests/unit/components/Charts/chartReveal.test.tsx +++ b/tests/unit/components/Charts/chartReveal.test.tsx @@ -38,9 +38,12 @@ function EngineDownloading() { return null; } -function renderChart(isLoading: boolean, label = 'chart') { +function renderChart(isLoading: boolean, label = 'chart', isCardMeasured = true) { return ( - + { jest.restoreAllMocks(); }); + it('should fade the chart in without a spinner when it is only waiting for its card to be measured', () => { + // Given a chart whose data and fonts are already in memory, mounted before its card has been measured + const {rerender} = render(renderChart(false, 'chart', false)); + + // Then nothing covers the card, because the width lands within a frame or two and a spinner would only flash + expect(screen.UNSAFE_queryByType(ActivityIndicator)).toBeNull(); + + // When the card is measured + rerender(renderChart(false)); + + // Then the chart starts fading in at once rather than after a hold, since there is no spinner to keep up while Skia draws + expect(screen.getByText('chart')).toBeTruthy(); + expect(screen.UNSAFE_queryByType(ActivityIndicator)).toBeNull(); + expect(pendingFrames.size).toBe(0); + }); + it('should keep the spinner over the mounted chart until the last held frame', () => { - // Given a chart whose data has just become ready - render(renderChart(false)); + // Given a chart that was loading and whose data has just become ready + const {rerender} = render(renderChart(true)); + rerender(renderChart(false)); // When every held frame but the last passes runFrames(HOLD_FRAMES - 1); @@ -90,7 +110,10 @@ describe('ChartReveal', () => { it('should keep one spinner from the engine download until the reveal', () => { // Given a chart whose engine is still downloading, so nothing below has reported yet const {rerender} = render( - + , ); @@ -109,8 +132,9 @@ describe('ChartReveal', () => { }); it('should not hold again when a revealed chart gets new data', () => { - // Given a chart that has been revealed - const {rerender} = render(renderChart(false)); + // Given a chart that has been revealed after loading + const {rerender} = render(renderChart(true)); + rerender(renderChart(false)); runFrames(HOLD_FRAMES); // When its data changes without it loading again @@ -121,10 +145,10 @@ describe('ChartReveal', () => { expect(screen.UNSAFE_queryByType(ActivityIndicator)).toBeNull(); }); - it('should hold again when a revealed chart reloads', () => { - // Given a chart that has been revealed - const {rerender} = render(renderChart(false)); - runFrames(HOLD_FRAMES); + it('should hold when a chart that faded in without a spinner reloads', () => { + // Given a chart that faded in as soon as its card was measured + const {rerender} = render(renderChart(false, 'chart', false)); + rerender(renderChart(false)); // When it loads again and its new data arrives rerender(renderChart(true)); @@ -136,8 +160,9 @@ describe('ChartReveal', () => { }); it('should restart the hold when a chart reloads before its hold ends', () => { - // Given a chart partway through its hold - const {rerender} = render(renderChart(false)); + // Given a chart partway through its hold after loading + const {rerender} = render(renderChart(true)); + rerender(renderChart(false)); runFrames(HOLD_FRAMES - 2); // When it loads again and its new data arrives @@ -157,8 +182,9 @@ describe('ChartReveal', () => { }); it('should stop counting frames when the chart unmounts during its hold', () => { - // Given a chart partway through its hold - const {unmount} = render(renderChart(false)); + // Given a chart partway through its hold after loading + const {rerender, unmount} = render(renderChart(true)); + rerender(renderChart(false)); runFrames(1); // When it unmounts, as it does when the user leaves the page diff --git a/tests/unit/components/Charts/chartWidthHandoff.test.tsx b/tests/unit/components/Charts/chartWidthHandoff.test.tsx index 38eac5d6ec37..fe4f3ecc6f7a 100644 --- a/tests/unit/components/Charts/chartWidthHandoff.test.tsx +++ b/tests/unit/components/Charts/chartWidthHandoff.test.tsx @@ -110,7 +110,7 @@ describe('chart width handoff', () => { expect(PolarChart).toHaveBeenCalledWith(expect.objectContaining({explicitSize: {width: CONTAINER_WIDTH, height: CHART_CONTENT_MIN_HEIGHT}}), undefined); }); - it('should keep the pie chart under the spinner until a width has been measured', () => { + it('should wait for the pie chart width without a spinner when its data is already in memory', () => { // Given a pie chart whose box has not reported a layout yet // When it renders with its data render( @@ -121,23 +121,31 @@ describe('chart width handoff', () => { />, ); - // Then the chart waits for the width instead of measuring itself, because the reveal counts its held frames - // from the render the chart reports ready in, and a chart that measures itself draws later than that + // Then the chart waits for the width instead of measuring itself, because the reveal starts the fade in the render + // the card is measured in, and a chart that measures itself draws later than that. Nothing covers the card meanwhile, + // since the width lands within a frame or two and a spinner would only flash. expect(PolarChart).not.toHaveBeenCalled(); - expect(screen.UNSAFE_queryByType(ActivityIndicator)).not.toBeNull(); + expect(screen.UNSAFE_queryByType(ActivityIndicator)).toBeNull(); }); it('should keep the pie chart mounted when its box reports a width of 0', () => { - // Given a pie chart that has mounted at its measured width - render( + // Given a pie chart that has mounted at the width measured while it was loading + const {rerender} = render( , ); const measuredBox = getMeasuredBox(); fireEvent(measuredBox, 'layout', {nativeEvent: {layout: {width: CONTAINER_WIDTH, height: CHART_CONTENT_MIN_HEIGHT}}}); + rerender( + , + ); // When its card is hidden and the box reports a width of 0 fireEvent(measuredBox, 'layout', {nativeEvent: {layout: {width: 0, height: 0}}}); From 4c0ac5ad39b13589076d4096d4b6a0d592804ab9 Mon Sep 17 00:00:00 2001 From: Sergei Sharabai <105950333+sharabai@users.noreply.github.com> Date: Thu, 15 Oct 2026 20:15:43 +0200 Subject: [PATCH 3/6] Count the chart hold on the UI thread --- .../Charts/components/ChartReveal.tsx | 56 ++++++++++++------- .../components/Charts/chartReveal.test.tsx | 14 ++++- .../Charts/chartWidthHandoff.test.tsx | 2 +- 3 files changed, 51 insertions(+), 21 deletions(-) diff --git a/src/components/Charts/components/ChartReveal.tsx b/src/components/Charts/components/ChartReveal.tsx index 3216e1fbdcda..a12074237c26 100644 --- a/src/components/Charts/components/ChartReveal.tsx +++ b/src/components/Charts/components/ChartReveal.tsx @@ -10,10 +10,12 @@ import type {ReactNode} from 'react'; import React, {createContext, useContext, useEffect, useLayoutEffect, useState} from 'react'; import {View} from 'react-native'; import Animated, {useAnimatedStyle, useSharedValue, withTiming} from 'react-native-reanimated'; +import {scheduleOnRN, scheduleOnUI} from 'react-native-worklets'; /** * Skia draws a chart a few frames after it mounts: 2 to 4 on web. * Counted in frames because a stalled main thread delays the draw by frames, which a timer would not wait for. + * Counted on the UI thread, where native Skia draws, because frames counted on a busy JS thread run seconds past the draw. */ const HOLD_FRAMES = 5; @@ -57,9 +59,15 @@ function ChartReveal({loadingHeight, isCardMeasured, children}: ChartRevealProps const opacity = useSharedValue(0); const fadeStyle = useAnimatedStyle(() => ({opacity: opacity.get()})); + // Hidden on the UI thread as the fade starts, because unmounting it waits for a JS thread that can be busy for seconds + const spinnerStyle = useAnimatedStyle(() => ({opacity: opacity.get() > 0 ? 0 : 1})); + + // Written on the UI thread, which counts the hold, so that the cleanup there can cancel the hold's next frame + const holdFrameID = useSharedValue(0); + const isReady = !!loadState && !loadState.isLoading && isCardMeasured; - // A chart that mounts with everything in memory waits only for its card's width, which lands within a frame or two, + // A chart that mounts with everything in memory waits only for its card's first layout to report the width, // so it shows no spinner and fades in at once. Only a chart that has shown the spinner holds it until Skia has drawn. const isSpinnerShown = !loadState || hasReportedLoading; const shouldHold = hasReportedLoading && !!loadState?.isDrawnBySkia; @@ -81,36 +89,46 @@ function ChartReveal({loadingHeight, isCardMeasured, children}: ChartRevealProps return; } - const fadeIn = () => opacity.set(withTiming(1, {duration: CONST.ANIMATED_TRANSITION})); + // Read here so that the worklet below captures a number rather than all of CONST + const duration = CONST.ANIMATED_TRANSITION; if (!shouldHold) { - fadeIn(); + opacity.set(withTiming(1, {duration})); return; } - let framesLeft = HOLD_FRAMES; - let frameID = 0; - const countFrame = () => { - framesLeft -= 1; - if (framesLeft > 0) { - frameID = requestAnimationFrame(countFrame); - return; - } - setIsRevealed(true); - fadeIn(); + scheduleOnUI(() => { + 'worklet'; + + let framesLeft = HOLD_FRAMES; + const countFrame = () => { + framesLeft -= 1; + if (framesLeft > 0) { + holdFrameID.set(requestAnimationFrame(countFrame)); + return; + } + opacity.set(withTiming(1, {duration})); + scheduleOnRN(setIsRevealed, true); + }; + holdFrameID.set(requestAnimationFrame(countFrame)); + }); + + return () => { + scheduleOnUI(() => { + 'worklet'; + + cancelAnimationFrame(holdFrameID.get()); + }); }; - frameID = requestAnimationFrame(countFrame); - - return () => cancelAnimationFrame(frameID); - }, [isReady, shouldHold, opacity]); + }, [isReady, shouldHold, opacity, holdFrameID]); return ( {children} {!isRevealed && ( - + {isSpinnerShown && } - + )} diff --git a/tests/unit/components/Charts/chartReveal.test.tsx b/tests/unit/components/Charts/chartReveal.test.tsx index 64b276f1bbfa..426a38ec2fb4 100644 --- a/tests/unit/components/Charts/chartReveal.test.tsx +++ b/tests/unit/components/Charts/chartReveal.test.tsx @@ -12,7 +12,15 @@ const LOADING_HEIGHT = getCartesianChartHeight(); let pendingFrames = new Map(); let lastFrameID = 0; +/** The worklets mock runs UI-thread work on a timer and calls back into React in a microtask, which the fake timers below hold */ +function flushWorklets() { + act(() => { + jest.runOnlyPendingTimers(); + }); +} + function runFrames(count: number) { + flushWorklets(); for (let frame = 0; frame < count; frame++) { const callbacks = [...pendingFrames.values()]; pendingFrames.clear(); @@ -22,6 +30,7 @@ function runFrames(count: number) { } }); } + flushWorklets(); } type ChartBodyProps = { @@ -54,6 +63,7 @@ function renderChart(isLoading: boolean, label = 'chart', isCardMeasured = true) describe('ChartReveal', () => { beforeEach(() => { + jest.useFakeTimers(); pendingFrames = new Map(); jest.spyOn(global, 'requestAnimationFrame').mockImplementation((callback) => { lastFrameID += 1; @@ -70,13 +80,14 @@ describe('ChartReveal', () => { afterEach(() => { jest.restoreAllMocks(); + jest.useRealTimers(); }); it('should fade the chart in without a spinner when it is only waiting for its card to be measured', () => { // Given a chart whose data and fonts are already in memory, mounted before its card has been measured const {rerender} = render(renderChart(false, 'chart', false)); - // Then nothing covers the card, because the width lands within a frame or two and a spinner would only flash + // Then nothing covers the card, because the width comes from the card's first layout and a spinner would only flash expect(screen.UNSAFE_queryByType(ActivityIndicator)).toBeNull(); // When the card is measured @@ -189,6 +200,7 @@ describe('ChartReveal', () => { // When it unmounts, as it does when the user leaves the page unmount(); + flushWorklets(); // Then no frame is left waiting to reveal it, since that would update a component that is gone expect(pendingFrames.size).toBe(0); diff --git a/tests/unit/components/Charts/chartWidthHandoff.test.tsx b/tests/unit/components/Charts/chartWidthHandoff.test.tsx index fe4f3ecc6f7a..101084211368 100644 --- a/tests/unit/components/Charts/chartWidthHandoff.test.tsx +++ b/tests/unit/components/Charts/chartWidthHandoff.test.tsx @@ -123,7 +123,7 @@ describe('chart width handoff', () => { // Then the chart waits for the width instead of measuring itself, because the reveal starts the fade in the render // the card is measured in, and a chart that measures itself draws later than that. Nothing covers the card meanwhile, - // since the width lands within a frame or two and a spinner would only flash. + // since the width comes from the card's first layout and a spinner would only flash. expect(PolarChart).not.toHaveBeenCalled(); expect(screen.UNSAFE_queryByType(ActivityIndicator)).toBeNull(); }); From 6420b856ff92196cfcd76e1ef8a384cb3f97d235 Mon Sep 17 00:00:00 2001 From: Sergei Sharabai <105950333+sharabai@users.noreply.github.com> Date: Mon, 19 Oct 2026 13:05:01 +0200 Subject: [PATCH 4/6] Add ChartFrame header comment --- src/components/Charts/components/ChartFrame.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/components/Charts/components/ChartFrame.tsx b/src/components/Charts/components/ChartFrame.tsx index 13a691fef718..5785af5f3447 100644 --- a/src/components/Charts/components/ChartFrame.tsx +++ b/src/components/Charts/components/ChartFrame.tsx @@ -1,3 +1,6 @@ +/** + * Every chart entry point renders its chart inside this frame, so bar, line and pie charts measure their card and reveal the chart the same way on web and native. + */ import type {ReactNode} from 'react'; import React from 'react'; From 7178404436eb56949aaeca1b65de0a292a49026c Mon Sep 17 00:00:00 2001 From: Sergei Sharabai <105950333+sharabai@users.noreply.github.com> Date: Fri, 23 Oct 2026 13:15:51 +0200 Subject: [PATCH 5/6] Match chart loading heights to Home's responsive layouts --- .../Charts/BarChart/index.native.tsx | 4 +- src/components/Charts/BarChart/index.tsx | 4 +- .../Charts/LineChart/index.native.tsx | 2 +- src/components/Charts/LineChart/index.tsx | 2 +- .../Charts/PieChart/PieChartContent.tsx | 3 +- .../Charts/PieChart/index.native.tsx | 6 +- src/components/Charts/PieChart/index.tsx | 6 +- src/components/Charts/VictoryTheme.ts | 7 +- .../Charts/components/ChartFrame.tsx | 8 +- src/components/Charts/utils/chartHeights.ts | 32 +++++++- src/styles/index.ts | 4 +- tests/unit/components/Charts/utils.test.ts | 76 ++++++++++++++++++- 12 files changed, 134 insertions(+), 20 deletions(-) diff --git a/src/components/Charts/BarChart/index.native.tsx b/src/components/Charts/BarChart/index.native.tsx index e38f40c1a0c8..32233f3380b7 100644 --- a/src/components/Charts/BarChart/index.native.tsx +++ b/src/components/Charts/BarChart/index.native.tsx @@ -1,5 +1,5 @@ import ChartFrame from '@components/Charts/components/ChartFrame'; -import {getBarChartHeight} from '@components/Charts/utils/chartHeights'; +import {getBarChartLoadingHeight} from '@components/Charts/utils/chartHeights'; import React from 'react'; @@ -12,7 +12,7 @@ function BarChart(props: BarChartProps) { 0} - loadingHeight={getBarChartHeight(props.shouldShowLabels)} + getLoadingHeight={() => getBarChartLoadingHeight(props.shouldShowLabels, props.shouldWrapLabels)} > {(chartWidth) => ( 0} - loadingHeight={getBarChartHeight(props.shouldShowLabels)} + getLoadingHeight={() => getBarChartLoadingHeight(props.shouldShowLabels, props.shouldWrapLabels)} > {(chartWidth) => ( 0} - loadingHeight={getCartesianChartHeight()} + getLoadingHeight={() => getCartesianChartHeight()} > {(chartWidth) => ( 0} - loadingHeight={getCartesianChartHeight()} + getLoadingHeight={() => getCartesianChartHeight()} > {(chartWidth) => ( = VictoryTheme.pie.legendMinWidth; + const isLegendSideBySide = shouldUseResponsiveLegend && shouldShowLegend && hasRoomForSideLegend(chartWidth); const donutWidth = isLegendSideBySide ? canvasHeight : chartWidth; const radius = Math.min(donutWidth, canvasHeight) / 2; diff --git a/src/components/Charts/PieChart/index.native.tsx b/src/components/Charts/PieChart/index.native.tsx index 58159dbba903..2379b4230de5 100644 --- a/src/components/Charts/PieChart/index.native.tsx +++ b/src/components/Charts/PieChart/index.native.tsx @@ -1,6 +1,8 @@ import ChartFrame from '@components/Charts/components/ChartFrame'; import {getPieChartLoadingHeight} from '@components/Charts/utils/chartHeights'; +import useResponsiveLayout from '@hooks/useResponsiveLayout'; + import React from 'react'; import type {PieChartProps} from './PieChartContent'; @@ -8,11 +10,13 @@ import type {PieChartProps} from './PieChartContent'; import PieChartContent from './PieChartContent'; function PieChart(props: PieChartProps) { + const {shouldUseNarrowLayout} = useResponsiveLayout(); + return ( 0} - loadingHeight={getPieChartLoadingHeight(props.shouldShowLegend)} + getLoadingHeight={(chartWidth) => getPieChartLoadingHeight(chartWidth, shouldUseNarrowLayout, props.shouldShowLegend, props.shouldUseResponsiveLegend)} > {(chartWidth) => ( import('./PieChartContent'); function PieChart(props: PieChartProps) { + const {shouldUseNarrowLayout} = useResponsiveLayout(); + return ( 0} - loadingHeight={getPieChartLoadingHeight(props.shouldShowLegend)} + getLoadingHeight={(chartWidth) => getPieChartLoadingHeight(chartWidth, shouldUseNarrowLayout, props.shouldShowLegend, props.shouldUseResponsiveLegend)} > {(chartWidth) => ( number; /** Receives the card's width, or null until the card has been measured. */ children: (chartWidth: number | null) => ReactNode; }; -function ChartFrame({isLoading = false, hasData, loadingHeight, children}: ChartFrameProps) { +function ChartFrame({isLoading = false, hasData, getLoadingHeight, children}: ChartFrameProps) { // A chart that loads empty collapses at once, so it never starts the engine download or holds a spinner over nothing if (!isLoading && !hasData) { return null; @@ -29,7 +29,7 @@ function ChartFrame({isLoading = false, hasData, loadingHeight, children}: Chart {(chartWidth) => ( {children(chartWidth)} diff --git a/src/components/Charts/utils/chartHeights.ts b/src/components/Charts/utils/chartHeights.ts index a4b3e658ce0c..552e7ea95e40 100644 --- a/src/components/Charts/utils/chartHeights.ts +++ b/src/components/Charts/utils/chartHeights.ts @@ -1,7 +1,9 @@ -import VictoryTheme, {CHART_CONTENT_MIN_HEIGHT, X_AXIS_LABEL_MIN_HEIGHT} from '@components/Charts/VictoryTheme'; +import VictoryTheme, {CHART_CONTENT_MIN_HEIGHT, MAX_BAR_LABEL_LINES, X_AXIS_LABEL_LINE_HEIGHT, X_AXIS_LABEL_MIN_HEIGHT} from '@components/Charts/VictoryTheme'; import variables from '@styles/variables'; +import CONST from '@src/CONST'; + // Imported before the chart engine has loaded, so this file must not import Skia. function getXAxisLabelSpace(xAxisLabelHeight = 0): number { @@ -16,9 +18,33 @@ function getBarChartHeight(shouldShowLabels = true, xAxisLabelHeight = 0): numbe return shouldShowLabels ? getCartesianChartHeight(xAxisLabelHeight) : CHART_CONTENT_MIN_HEIGHT; } -function getPieChartLoadingHeight(shouldShowLegend = true): number { +// Wrapped labels keep every line of label space even when the names fit on fewer, so their height is known before the chart measures them. +function getBarChartLoadingHeight(shouldShowLabels = true, shouldWrapLabels = false): number { + return getBarChartHeight(shouldShowLabels, shouldWrapLabels ? MAX_BAR_LABEL_LINES * X_AXIS_LABEL_LINE_HEIGHT : 0); +} + +/** Whether the card leaves the responsive legend its minimum width beside a square donut. */ +function hasRoomForSideLegend(chartWidth: number): boolean { + return chartWidth - CHART_CONTENT_MIN_HEIGHT - VictoryTheme.pie.legendGap >= VictoryTheme.pie.legendMinWidth; +} + +function getPieChartLoadingHeight(chartWidth: number | null, shouldUseNarrowLayout: boolean, shouldShowLegend = true, shouldUseResponsiveLegend = false): number { + if (shouldShowLegend && shouldUseResponsiveLegend) { + // Until the card reports its width, the layout stands in for it: phone cards are too narrow for a side legend, and typical desktop cards fit one + const isLegendSideBySide = chartWidth === null ? !shouldUseNarrowLayout : hasRoomForSideLegend(chartWidth); + if (isLegendSideBySide) { + return CHART_CONTENT_MIN_HEIGHT; + } + + // The slice count arrives with the data, so this reserves a full Home top-N list, which most cards fill. + // A row is as tall as the taller of its dot column and its label's first line. + const rowCount = CONST.SEARCH.TOP_SEARCH_LIMIT; + const rowHeight = Math.max(variables.lineHeightXLarge, variables.fontSizeNormalHeight); + return CHART_CONTENT_MIN_HEIGHT + VictoryTheme.pie.legendStackGap + rowCount * rowHeight + (rowCount - 1) * VictoryTheme.pie.legendRowGap; + } + const legendFirstRowHeight = shouldShowLegend ? variables.pieChartLegendMargin + variables.fontSizeNormalHeight : 0; return CHART_CONTENT_MIN_HEIGHT + legendFirstRowHeight; } -export {getXAxisLabelSpace, getCartesianChartHeight, getBarChartHeight, getPieChartLoadingHeight}; +export {getXAxisLabelSpace, getCartesianChartHeight, getBarChartHeight, getBarChartLoadingHeight, hasRoomForSideLegend, getPieChartLoadingHeight}; diff --git a/src/styles/index.ts b/src/styles/index.ts index 839f1397d4a6..bad9fc41c177 100644 --- a/src/styles/index.ts +++ b/src/styles/index.ts @@ -7117,12 +7117,12 @@ const staticStyles = (theme: ThemeColors) => pieChartLegendSide: { width: VictoryTheme.pie.legendMinWidth, justifyContent: 'center', - rowGap: 12, + rowGap: VictoryTheme.pie.legendRowGap, }, pieChartLegendBelow: { alignSelf: 'stretch', marginTop: VictoryTheme.pie.legendStackGap, - rowGap: 12, + rowGap: VictoryTheme.pie.legendRowGap, }, pieChartLegendRow: { flexDirection: 'row', diff --git a/tests/unit/components/Charts/utils.test.ts b/tests/unit/components/Charts/utils.test.ts index 090c00f8e959..9d0f6635cbb9 100644 --- a/tests/unit/components/Charts/utils.test.ts +++ b/tests/unit/components/Charts/utils.test.ts @@ -23,17 +23,22 @@ import { rotatedLabelYOffset, truncateLabel, } from '@components/Charts/utils'; -import {getCartesianChartHeight} from '@components/Charts/utils/chartHeights'; +import {getBarChartHeight, getBarChartLoadingHeight, getCartesianChartHeight, getPieChartLoadingHeight} from '@components/Charts/utils/chartHeights'; import VictoryTheme, { CHART_CONTENT_MIN_HEIGHT, CHART_Y_SCALE_HEIGHT, DIAGONAL_ANGLE_RADIAN_THRESHOLD, LABEL_ROTATIONS, + MAX_BAR_LABEL_LINES, SIN_45, X_AXIS_LABEL_MIN_HEIGHT, } from '@components/Charts/VictoryTheme'; import {CHART_SERIES_KEY} from '@components/Search/buildChartSeries'; +import variables from '@styles/variables'; + +import CONST from '@src/CONST'; + const LINE_HEIGHT = 16; describe('truncateLabel', () => { @@ -787,6 +792,75 @@ describe('getCartesianChartHeight', () => { }); }); +describe('getBarChartLoadingHeight', () => { + // ExpensifyNeue's ascent plus descent at the 12px label size, (950 + 230) / 1000 of the em, which Skia measures for each label line + const SKIA_LABEL_LINE_HEIGHT = 14.16; + + it('should give the loading box the height of a chart whose labels wrap', () => { + // Given a bar chart with wrapped labels, which keeps two lines of label space whatever the names are + const loadedHeight = getBarChartHeight(true, MAX_BAR_LABEL_LINES * SKIA_LABEL_LINE_HEIGHT); + + // When the loading box is sized before the chart has measured anything + const loadingHeight = getBarChartLoadingHeight(true, true); + + // Then both heights match, so the card does not jump when the chart replaces the spinner + expect(loadingHeight).toBeCloseTo(loadedHeight, 5); + }); +}); + +describe('getPieChartLoadingHeight', () => { + // The narrowest card that fits the side legend: the 250px donut, the 24px gap and the 180px legend column + const SIDE_LEGEND_MIN_CARD_WIDTH = 454; + + it('should reserve the donut height for a side legend once the card is measured', () => { + // Given a responsive legend in a card just wide enough for it to sit beside the donut, on the narrow layout + // When the loading box is sized from the measured card width + const height = getPieChartLoadingHeight(SIDE_LEGEND_MIN_CARD_WIDTH, true, true, true); + + // Then it keeps the donut's height, since five legend rows fit beside it, and the measured width wins over the layout + expect(height).toBe(CHART_CONTENT_MIN_HEIGHT); + }); + + it('should reserve a full stacked legend under the donut when the card is too narrow for a side legend', () => { + // Given a responsive legend in a card 1px too narrow for the side legend, on the wide layout + // When the loading box is sized from the measured card width, before the slice count is known + const height = getPieChartLoadingHeight(SIDE_LEGEND_MIN_CARD_WIDTH - 1, false, true, true); + + // Then it reserves one row per slice Home can show, under the donut and the stack gap, since most cards fill the cap + const rowCount = CONST.SEARCH.TOP_SEARCH_LIMIT; + const legendHeight = rowCount * variables.fontSizeNormalHeight + (rowCount - 1) * VictoryTheme.pie.legendRowGap; + expect(height).toBe(CHART_CONTENT_MIN_HEIGHT + VictoryTheme.pie.legendStackGap + legendHeight); + }); + + it('should reserve only the donut when the legend is hidden', () => { + // Given a responsive pie with its legend turned off, in a card too narrow for a side legend + // When the loading box is sized + const height = getPieChartLoadingHeight(SIDE_LEGEND_MIN_CARD_WIDTH - 1, true, false, true); + + // Then it keeps the donut's height alone, since the chart draws no legend to stack + expect(height).toBe(CHART_CONTENT_MIN_HEIGHT); + }); + + it('should assume a stacked legend on the narrow layout until the card is measured', () => { + // Given a responsive legend whose card has not reported its width yet, on the narrow layout, where phone cards are too narrow for a side legend + // When the loading box is sized before the width lands + const height = getPieChartLoadingHeight(null, true, true, true); + + // Then it matches the stacked height a phone card measures into, so the box does not change when the width lands + const stackedHeight = getPieChartLoadingHeight(SIDE_LEGEND_MIN_CARD_WIDTH - 1, true, true, true); + expect(height).toBe(stackedHeight); + }); + + it('should assume a side legend on the wide layout until the card is measured', () => { + // Given a responsive legend whose card has not reported its width yet, on the wide layout, where a typical desktop card fits a side legend + // When the loading box is sized before the width lands + const height = getPieChartLoadingHeight(null, false, true, true); + + // Then it keeps the donut's height that a desktop card measures into, so the box does not change when the width lands + expect(height).toBe(CHART_CONTENT_MIN_HEIGHT); + }); +}); + describe('getCartesianPlotBounds', () => { const PADDING_LEFT = VictoryTheme.axis.padding.left; From de8d32870f60ad58c185471a947a448f93f5d293 Mon Sep 17 00:00:00 2001 From: Sergei Sharabai <105950333+sharabai@users.noreply.github.com> Date: Fri, 23 Oct 2026 13:25:36 +0200 Subject: [PATCH 6/6] Point chart loading heights and layouts at each other --- src/components/Charts/BarChart/VerticalBarChartContent.tsx | 1 + src/components/Charts/PieChart/PieChartContent.tsx | 1 + src/components/Charts/utils/chartHeights.ts | 2 ++ src/styles/index.ts | 1 + 4 files changed, 5 insertions(+) diff --git a/src/components/Charts/BarChart/VerticalBarChartContent.tsx b/src/components/Charts/BarChart/VerticalBarChartContent.tsx index 8e093985635f..9da40b5a3b59 100644 --- a/src/components/Charts/BarChart/VerticalBarChartContent.tsx +++ b/src/components/Charts/BarChart/VerticalBarChartContent.tsx @@ -114,6 +114,7 @@ function VerticalBarChartCanvas({ canvasWidth: chartWidth, } : undefined; + // `getBarChartLoadingHeight` reserves the same lines while the chart loads, so update the two together. const wrapLabelHeight = wrapConfig ? wrapConfig.maxLines * wrapConfig.lineHeight : 0; // When wrapping, labelAreaWidth is 0 above so the hook returns EMPTY_LAYOUT (horizontal, skip 1). // xAxisLabelHeight is 0 there too, so the wrap height has to be substituted explicitly. diff --git a/src/components/Charts/PieChart/PieChartContent.tsx b/src/components/Charts/PieChart/PieChartContent.tsx index 628f9c382159..e2499c27bb9b 100644 --- a/src/components/Charts/PieChart/PieChartContent.tsx +++ b/src/components/Charts/PieChart/PieChartContent.tsx @@ -286,6 +286,7 @@ function PieChartCanvas({data, series, valueUnit, valueUnitPosition, onSlicePres ); + // `getPieChartLoadingHeight` rebuilds the height of both layouts below from the same theme values, so update it with any change to their structure or spacing. if (shouldUseResponsiveLegend) { return ( diff --git a/src/components/Charts/utils/chartHeights.ts b/src/components/Charts/utils/chartHeights.ts index 552e7ea95e40..df1d6e5fb964 100644 --- a/src/components/Charts/utils/chartHeights.ts +++ b/src/components/Charts/utils/chartHeights.ts @@ -19,6 +19,7 @@ function getBarChartHeight(shouldShowLabels = true, xAxisLabelHeight = 0): numbe } // Wrapped labels keep every line of label space even when the names fit on fewer, so their height is known before the chart measures them. +// Mirrors the wrap height `VerticalBarChartContent` draws, so update the two together. function getBarChartLoadingHeight(shouldShowLabels = true, shouldWrapLabels = false): number { return getBarChartHeight(shouldShowLabels, shouldWrapLabels ? MAX_BAR_LABEL_LINES * X_AXIS_LABEL_LINE_HEIGHT : 0); } @@ -28,6 +29,7 @@ function hasRoomForSideLegend(chartWidth: number): boolean { return chartWidth - CHART_CONTENT_MIN_HEIGHT - VictoryTheme.pie.legendGap >= VictoryTheme.pie.legendMinWidth; } +// Mirrors the legend layouts `PieChartContent` draws with the `pieChartLegend*` and `pieChartResponsive*` styles, so update it with any change to their structure or spacing. function getPieChartLoadingHeight(chartWidth: number | null, shouldUseNarrowLayout: boolean, shouldShowLegend = true, shouldUseResponsiveLegend = false): number { if (shouldShowLegend && shouldUseResponsiveLegend) { // Until the card reports its width, the layout stands in for it: phone cards are too narrow for a side legend, and typical desktop cards fit one diff --git a/src/styles/index.ts b/src/styles/index.ts index bad9fc41c177..2ca45f1fe174 100644 --- a/src/styles/index.ts +++ b/src/styles/index.ts @@ -7104,6 +7104,7 @@ const staticStyles = (theme: ThemeColors) => width: 12, height: 12, }, + // `getPieChartLoadingHeight` sizes the loading chart from the responsive legend styles below, so update it with them. pieChartResponsiveRow: { flexDirection: 'row', alignItems: 'center',