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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,11 @@ function BaseHTMLEngineProvider({textSelectable = false, children, enableExperim
contentModel: HTMLContentModel.block,
mixedUAStyles: styles.mv3,
}),
ol: HTMLElementModel.fromCustomModel({
tagName: 'ol',
contentModel: HTMLContentModel.block,
mixedUAStyles: styles.mv3,
}),
'sparkles-icon': HTMLElementModel.fromCustomModel({
tagName: 'sparkles-icon',
contentModel: HTMLContentModel.mixed,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import React from 'react';
import {View} from 'react-native';
import type {TNode} from 'react-native-render-html';
import {TNodeChildrenRenderer} from 'react-native-render-html';
import Text from '@components/Text';
import useThemeStyles from '@hooks/useThemeStyles';

type NumberedItemRendererProps = {tnode: TNode; index: number};

function NumberedItemRenderer({tnode, index}: NumberedItemRendererProps) {
const styles = useThemeStyles();

return (
<View style={[styles.flexRow, styles.w100]}>
<Text style={styles.numberedListItemMarker}>{`${index}.`}</Text>
<View style={styles.flex1}>
<TNodeChildrenRenderer tnode={tnode} />
</View>
</View>
);
}

export default NumberedItemRenderer;
51 changes: 51 additions & 0 deletions src/components/HTMLEngineProvider/HTMLRenderers/OLRenderer.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import React from 'react';
import {View} from 'react-native';
import type {CustomRendererProps, TBlock} from 'react-native-render-html';
import {TNodeRenderer} from 'react-native-render-html';
import useThemeStyles from '@hooks/useThemeStyles';
import NumberedItemRenderer from './NumberedItemRenderer';

function buildLiIndices(children: CustomRendererProps<TBlock>['tnode']['children']): Map<number, number> {
const map = new Map<number, number>();
let counter = 0;
for (const [i, child] of children.entries()) {
if (child.tagName === 'li') {
counter += 1;
map.set(i, counter);
Comment on lines +10 to +14

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Respect

    numbering attributes when rendering markers

    The new ordered-list renderer always starts numbering from 1 and increments per <li> (counter = 0 then counter += 1), so it ignores standard list attributes like <ol start>, <ol reversed>, <ol type>, and per-item value. In any HTML payload that uses those attributes (for example, continuation lists starting at 3), the rendered numbering becomes incorrect (1., 2., ...), which changes the meaning of ordered instructions.

    Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Question weather we need it? Do we expect concierge to use those ?

Comment on lines +10 to +14

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve ordered-list numbering attributes

The custom <ol> renderer always derives marker numbers from a local counter (0 -> 1, 2, ...) and never reads list/item attributes, so ordered lists with start, reversed, type, or <li value> now render incorrect numbering (e.g., continuation steps restart at 1.). This is a functional regression from native ordered-list semantics and can change instruction meaning in concierge responses or imported HTML. Fresh evidence in this commit: buildLiIndices() only increments counter, and the new tests assert only 1./2. paths without any attribute-based cases.

Useful? React with 👍 / 👎.

Comment on lines +10 to +14

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Respect ordered-list numbering attributes

When an ordered list uses standard numbering controls such as <ol start="3"> or an <li value="…"> after a split list, this renderer still initializes its counter at 0 and increments only by child position, so the UI displays 1., 2., … instead of the authored numbers. Since this replaces the default <ol> handling for rendered comments/help HTML, any continued or explicitly numbered list will show incorrect step numbers.

Useful? React with 👍 / 👎.

}
}
return map;
}

function OLRenderer({tnode, style}: CustomRendererProps<TBlock>) {
const styles = useThemeStyles();
const liIndices = buildLiIndices(tnode.children);

return (
<View style={[style, styles.gap2]}>
{tnode.children.map((child, index) => {
const key = `${child.tagName ?? 'node'}-${index}`;
const liIndex = liIndices.get(index);
if (liIndex !== undefined) {
return (
<NumberedItemRenderer
key={key}
tnode={child}
index={liIndex}
/>
);
}
return (
<TNodeRenderer
key={key}
tnode={child}
renderIndex={index}
renderLength={tnode.children.length}
/>
);
})}
</View>
);
}

export default OLRenderer;
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@ import BulletItemRenderer from './BulletItemRenderer';
/**
* Bypasses the library's internal ULRenderer (which wraps children in MarkedListItem)
* and renders <ul> as a plain block container that draws bullet markers around each
* direct <li> child — matching how <bullet-list>/<bullet-item> render. <li> is left
* unregistered globally so that <ol><li> still uses the library's default numeric markers.
* direct <li> child — matching how <bullet-list>/<bullet-item> render.
*/
function ULRenderer({tnode, style}: CustomRendererProps<TBlock>) {
const styles = useThemeStyles();
Expand Down
2 changes: 2 additions & 0 deletions src/components/HTMLEngineProvider/HTMLRenderers/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import MentionHereRenderer from './MentionHereRenderer';
import MentionReportRenderer from './MentionReportRenderer';
import MentionUserRenderer from './MentionUserRenderer';
import NextStepEmailRenderer from './NextStepEmailRenderer';
import OLRenderer from './OLRenderer';
import PreRenderer from './PreRenderer';
import RBRRenderer from './RBRRenderer';
import ShortMentionRenderer from './ShortMentionRenderer';
Expand All @@ -31,6 +32,7 @@ const HTMLEngineProviderComponentList: CustomTagRendererRecord = {
a: AnchorRenderer,
code: CodeRenderer,
img: ImageRenderer,
ol: OLRenderer,
ul: ULRenderer,
video: VideoRenderer,

Expand Down
8 changes: 5 additions & 3 deletions src/components/RenderHTML.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import useHasTextAncestor from '@hooks/useHasTextAncestor';
import useWindowDimensions from '@hooks/useWindowDimensions';
import Parser from '@libs/Parser';
import BulletItemRenderer from './HTMLEngineProvider/HTMLRenderers/BulletItemRenderer';
import OLRenderer from './HTMLEngineProvider/HTMLRenderers/OLRenderer';
import SparklesIconRenderer from './HTMLEngineProvider/HTMLRenderers/SparklesIconRenderer';
import ULRenderer from './HTMLEngineProvider/HTMLRenderers/ULRenderer';

Expand All @@ -14,8 +15,8 @@ type LinkPressHandler = NonNullable<RenderersProps['a']>['onPress'];
const RE_BRACKET_ESCAPE = /&amp;#9[13];/g;
// Matches consecutive duplicate <emoji> or </emoji> tags, keeping only the outermost one.
const RE_EMOJI_OPEN_OR_CLOSE = /(<emoji[^>]*>)(?:<emoji[^>]*>)+|(<\/emoji[^>]*>)(?:<\/emoji[^>]*>)+/g;
// Strips orphaned <br/> tags inside <ul> that would render as extra empty bullets.
const RE_BR_CLEANUP = /<br\s*\/?>\s*(<\/ul>)|(<\/li>)\s*<br\s*\/?>\s*(?=<(?:li|\/ul)>)/gi;
// Strips one or more orphaned <br/> tags inside <ul> and <ol> that would render as extra empty items.
const RE_BR_CLEANUP = /(?:\s*<br\s*\/?>)+\s*(<\/(?:ul|ol)>)|(<\/li>)(?:\s*<br\s*\/?>)+\s*(?=<(?:li|\/(?:ul|ol))>)/gi;

type RenderHTMLProps = {
/** HTML string to render */
Expand Down Expand Up @@ -46,7 +47,7 @@ function RenderHTML({html: htmlParam, onLinkPress, isSelectable}: RenderHTMLProp
.replaceAll(RE_BRACKET_ESCAPE, (m) => (m.at(7) === '1' ? '[' : ']'))
// Remove double <emoji> tag if exists and keep the outermost tag (always the original tag).
.replaceAll(RE_EMOJI_OPEN_OR_CLOSE, '$1$2')
// Strip orphaned <br/> tags inside <ul> that would render as extra empty bullets
// Strip orphaned <br/> tags inside <ul> and <ol> that would render as extra empty items
.replaceAll(RE_BR_CLEANUP, '$1$2')
);
}, [htmlParam]);
Expand All @@ -63,6 +64,7 @@ function RenderHTML({html: htmlParam, onLinkPress, isSelectable}: RenderHTMLProp
/* eslint-disable @typescript-eslint/naming-convention */
'bullet-item': BulletItemRenderer,
'sparkles-icon': SparklesIconRenderer,
ol: OLRenderer,
ul: ULRenderer,
};

Expand Down
8 changes: 8 additions & 0 deletions src/styles/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,14 @@ const staticStyles = (theme: ThemeColors) =>
fontStyle: FontUtils.fontFamily.platform.EXP_NEUE_ITALIC.fontStyle,
},

numberedListItemMarker: {
fontSize: variables.fontSizeNormal,
lineHeight: variables.fontSizeNormalHeight,
minWidth: 32,
textAlign: 'right',
paddingHorizontal: 8,
},

autoCompleteSuggestionContainer: {
flexDirection: 'row',
alignItems: 'center',
Expand Down
93 changes: 93 additions & 0 deletions tests/unit/BulletListRendererTest.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import {render, screen} from '@testing-library/react-native';
import React from 'react';
import type {CustomRendererProps, TBlock} from 'react-native-render-html';
import BulletItemRenderer from '@components/HTMLEngineProvider/HTMLRenderers/BulletItemRenderer';
import NumberedItemRenderer from '@components/HTMLEngineProvider/HTMLRenderers/NumberedItemRenderer';
import OLRenderer from '@components/HTMLEngineProvider/HTMLRenderers/OLRenderer';
import ULRenderer from '@components/HTMLEngineProvider/HTMLRenderers/ULRenderer';
import RenderHTML from '@components/RenderHTML';
import CONST from '@src/CONST';
Expand Down Expand Up @@ -82,6 +84,50 @@ describe('Bullet list rendering', () => {
});
});

describe('OLRenderer', () => {
it('wraps each <li> child with a numbered marker', () => {
render(
// @ts-expect-error — only the props read by the renderer are needed for this test
<OLRenderer
tnode={buildULTNode([
{tagName: 'li', text: 'First'},
{tagName: 'li', text: 'Second'},
])}
style={{}}
/>,
);
expect(screen.getByText('1.')).toBeTruthy();
expect(screen.getByText('2.')).toBeTruthy();
expect(screen.getByText('First')).toBeTruthy();
expect(screen.getByText('Second')).toBeTruthy();
});

it('renders non-<li> children with the default node renderer', () => {
render(
// @ts-expect-error — only the props read by the renderer are needed for this test
<OLRenderer
tnode={buildULTNode([{tagName: 'span', text: 'stray child'}])}
style={{}}
/>,
);
expect(screen.queryByText('1.')).toBeNull();
expect(screen.getByText('stray child')).toBeTruthy();
});
});

describe('NumberedItemRenderer', () => {
it('renders a numbered marker next to the item content', () => {
render(
<NumberedItemRenderer
tnode={buildTNode('First item')}
index={1}
/>,
);
expect(screen.getByText('1.')).toBeTruthy();
expect(screen.getByText('First item')).toBeTruthy();
});
});

describe('RenderHTML strips orphaned <br/> tags inside <ul>', () => {
it('strips <br/> immediately before </ul>', () => {
render(<RenderHTML html="<ul><li>One</li><li>Two</li><br/></ul>" />);
Expand All @@ -103,6 +149,16 @@ describe('Bullet list rendering', () => {
expect(capturedSource.html).toBe('<ul><li>One</li><li>Two</li></ul>');
});

it('strips multiple consecutive <br/> before </ul>', () => {
render(<RenderHTML html="<ul><li>One</li><br/><br/><br/></ul>" />);
expect(capturedSource.html).toBe('<ul><li>One</li></ul>');
});

it('strips multiple consecutive <br/> between </li> and <li>', () => {
render(<RenderHTML html="<ul><li>One</li><br/><br/><li>Two</li></ul>" />);
expect(capturedSource.html).toBe('<ul><li>One</li><li>Two</li></ul>');
});

it('does not strip <br/> outside of <ul> lists', () => {
render(<RenderHTML html="<p>line1<br/>line2</p>" />);
expect(capturedSource.html).toBe('<p>line1<br/>line2</p>');
Expand All @@ -113,4 +169,41 @@ describe('Bullet list rendering', () => {
expect(capturedSource.html).toBe('<ul><li>One<br/>still one</li><li>Two</li></ul>');
});
});

describe('RenderHTML strips orphaned <br/> tags inside <ol>', () => {
it('strips <br/> immediately before </ol>', () => {
render(<RenderHTML html="<ol><li>One</li><li>Two</li><br/></ol>" />);
expect(capturedSource.html).toBe('<ol><li>One</li><li>Two</li></ol>');
});

it('strips <br> (no slash) immediately before </ol>', () => {
render(<RenderHTML html="<ol><li>One</li><li>Two</li><br></ol>" />);
expect(capturedSource.html).toBe('<ol><li>One</li><li>Two</li></ol>');
});

it('strips <br/> appearing between </li> and the next <li> inside <ol>', () => {
render(<RenderHTML html="<ol><li>One</li><br/><li>Two</li></ol>" />);
expect(capturedSource.html).toBe('<ol><li>One</li><li>Two</li></ol>');
});

it('leaves a valid <ol>/<li> list untouched', () => {
render(<RenderHTML html="<ol><li>One</li><li>Two</li></ol>" />);
expect(capturedSource.html).toBe('<ol><li>One</li><li>Two</li></ol>');
});

it('strips multiple consecutive <br/> before </ol>', () => {
render(<RenderHTML html="<ol><li>One</li><br/><br/><br/></ol>" />);
expect(capturedSource.html).toBe('<ol><li>One</li></ol>');
});

it('strips multiple consecutive <br/> between </li> and <li> inside <ol>', () => {
render(<RenderHTML html="<ol><li>One</li><br/><br/><li>Two</li></ol>" />);
expect(capturedSource.html).toBe('<ol><li>One</li><li>Two</li></ol>');
});

it('preserves <br/> that lives inside <li> as an in-item line break', () => {
render(<RenderHTML html="<ol><li>One<br/>still one</li><li>Two</li></ol>" />);
expect(capturedSource.html).toBe('<ol><li>One<br/>still one</li><li>Two</li></ol>');
});
});
});
Loading