From a6bc668371f0bc4df815a0178be869b9c69da041 Mon Sep 17 00:00:00 2001 From: lakshya1goel Date: Sat, 22 Feb 2025 11:47:14 +0530 Subject: [PATCH 1/2] content: Add start attribute support for ordered list Fixes: #59 --- lib/model/content.dart | 47 ++++++++++++++---------- lib/widgets/content.dart | 7 ++-- test/model/content_test.dart | 66 ++++++++++++++++++++++------------ test/widgets/content_test.dart | 11 ++++++ 4 files changed, 86 insertions(+), 45 deletions(-) diff --git a/lib/model/content.dart b/lib/model/content.dart index 8a5204973e..d3bf3a517a 100644 --- a/lib/model/content.dart +++ b/lib/model/content.dart @@ -251,21 +251,11 @@ class HeadingNode extends BlockInlineContainerNode { } } -enum ListStyle { ordered, unordered } +sealed class ListNode extends BlockContentNode { + const ListNode(this.items, {super.debugHtmlNode}); -class ListNode extends BlockContentNode { - const ListNode(this.style, this.items, {super.debugHtmlNode}); - - final ListStyle style; final List> items; - @override - void debugFillProperties(DiagnosticPropertiesBuilder properties) { - super.debugFillProperties(properties); - properties.add(FlagProperty('ordered', value: style == ListStyle.ordered, - ifTrue: 'ordered', ifFalse: 'unordered')); - } - @override List debugDescribeChildren() { return items @@ -275,6 +265,22 @@ class ListNode extends BlockContentNode { } } +class UnorderedListNode extends ListNode { + const UnorderedListNode(super.items, {super.debugHtmlNode}); +} + +class OrderedListNode extends ListNode { + const OrderedListNode(super.items, {required this.start, super.debugHtmlNode}); + + final int start; + + @override + void debugFillProperties(DiagnosticPropertiesBuilder properties) { + super.debugFillProperties(properties); + properties.add(IntProperty('start', start)); + } +} + class QuotationNode extends BlockContentNode { const QuotationNode(this.nodes, {super.debugHtmlNode}); @@ -1056,12 +1062,7 @@ class _ZulipContentParser { } BlockContentNode parseListNode(dom.Element element) { - ListStyle? listStyle; - switch (element.localName) { - case 'ol': listStyle = ListStyle.ordered; break; - case 'ul': listStyle = ListStyle.unordered; break; - } - assert(listStyle != null); + assert(element.localName == 'ol' || element.localName == 'ul'); assert(element.className.isEmpty); final debugHtmlNode = kDebugMode ? element : null; @@ -1074,7 +1075,15 @@ class _ZulipContentParser { items.add(parseImplicitParagraphBlockContentList(item.nodes)); } - return ListNode(listStyle!, items, debugHtmlNode: debugHtmlNode); + if (element.localName == 'ol') { + final startAttr = element.attributes['start']; + final start = startAttr == null ? 1 + : int.tryParse(startAttr, radix: 10); + if (start == null) return UnimplementedBlockContentNode(htmlNode: element); + return OrderedListNode(items, start: start, debugHtmlNode: debugHtmlNode); + } else { + return UnorderedListNode(items, debugHtmlNode: debugHtmlNode); + } } BlockContentNode parseSpoilerNode(dom.Element divElement) { diff --git a/lib/widgets/content.dart b/lib/widgets/content.dart index 8799e8d192..63f9fcab5b 100644 --- a/lib/widgets/content.dart +++ b/lib/widgets/content.dart @@ -482,7 +482,7 @@ class ListNodeWidget extends StatelessWidget { final items = List.generate(node.items.length, (index) { final item = node.items[index]; String marker; - switch (node.style) { + switch (node) { // TODO(#161): different unordered marker styles at different levels of nesting // see: // https://html.spec.whatwg.org/multipage/rendering.html#lists @@ -490,9 +490,8 @@ class ListNodeWidget extends StatelessWidget { // TODO proper alignment of unordered marker; should be "• ", one space, // but that comes out too close to item; not sure what's fixing that // in a browser - case ListStyle.unordered: marker = "• "; break; - // TODO(#59) ordered lists starting not at 1 - case ListStyle.ordered: marker = "${index+1}. "; break; + case UnorderedListNode(): marker = "• "; break; + case OrderedListNode(:final start): marker = "${start + index}. "; break; } return ListItemWidget(marker: marker, nodes: item); }); diff --git a/test/model/content_test.dart b/test/model/content_test.dart index 117c121660..626b7c560e 100644 --- a/test/model/content_test.dart +++ b/test/model/content_test.dart @@ -269,6 +269,26 @@ class ContentExample { url: '/#narrow/channel/378-api-design/topic/notation.20for.20near.20links/near/1972281', nodes: [TextNode('#api design > notation for near links @ 💬')])); + static const orderedListCustomStart = ContentExample( + 'ordered list with custom start', + '5. fifth\n6. sixth', + '
    \n
  1. fifth
    2. \n
  2. sixth
    3. \n
', + [OrderedListNode([ + [ParagraphNode(wasImplicit: true, links: null, nodes: [TextNode('fifth')])], + [ParagraphNode(wasImplicit: true, links: null, nodes: [TextNode('sixth')])], + ], start: 5)], + ); + + static const orderedListLargeStart = ContentExample( + 'ordered list with large start number', + '9999. first\n10000. second', + '
    \n
  1. first
    2. \n
  2. second
    3. \n
', + [OrderedListNode([ + [ParagraphNode(wasImplicit: true, links: null, nodes: [TextNode('first')])], + [ParagraphNode(wasImplicit: true, links: null, nodes: [TextNode('second')])], + ], start: 9999)], + ); + static const spoilerDefaultHeader = ContentExample( 'spoiler with default header', '```spoiler\nhello world\n```', @@ -306,13 +326,13 @@ class ContentExample { '

italic zulip

\n' '', [SpoilerNode( - header: [ListNode(ListStyle.ordered, [ - [ListNode(ListStyle.unordered, [ - [HeadingNode(level: HeadingLevel.h2, links: null, nodes: [ - TextNode('hello'), + header: [OrderedListNode([[ + UnorderedListNode([ + [HeadingNode(level: HeadingLevel.h2, links: null, nodes: [ + TextNode('hello'), + ])] ])] - ])], - ])], + ], start: 1)], content: [ParagraphNode(links: null, nodes: [ EmphasisNode(nodes: [TextNode('italic')]), TextNode(' '), @@ -836,7 +856,7 @@ class ContentExample { '
' '' '
\n', [ - ListNode(ListStyle.unordered, [[ + UnorderedListNode([[ ImageNodeList([ ImageNode(srcUrl: 'https://chat.zulip.org/user_avatars/2/realm/icon.png', thumbnailUrl: null, loading: false, @@ -858,7 +878,7 @@ class ContentExample { '
' '' '
\n', [ - ListNode(ListStyle.unordered, [[ + UnorderedListNode([[ ParagraphNode(wasImplicit: true, links: null, nodes: [ LinkNode(url: 'https://chat.zulip.org/user_avatars/2/realm/icon.png', nodes: [TextNode('icon.png')]), TextNode(' '), @@ -887,7 +907,7 @@ class ContentExample { '' '' 'more text\n', [ - ListNode(ListStyle.unordered, [[ + UnorderedListNode([[ const ParagraphNode(wasImplicit: true, links: null, nodes: [ LinkNode(url: 'https://chat.zulip.org/user_avatars/2/realm/icon.png', nodes: [TextNode('icon.png')]), TextNode(' '), @@ -1455,16 +1475,16 @@ void main() { testParse('
    ', // "1. first\n2. then" '
      \n
    1. first
      2. \n
    2. then
      3. \n
    ', const [ - ListNode(ListStyle.ordered, [ - [ParagraphNode(wasImplicit: true, links: null, nodes: [TextNode('first')])], - [ParagraphNode(wasImplicit: true, links: null, nodes: [TextNode('then')])], - ]), + OrderedListNode([ + [ParagraphNode(wasImplicit: true, links: null, nodes: [TextNode('first')])], + [ParagraphNode(wasImplicit: true, links: null, nodes: [TextNode('then')])], + ], start: 1), ]); testParse('
      ', // "* something\n* another" '
        \n
      • something
        • \n
      • another
        • \n
      ', const [ - ListNode(ListStyle.unordered, [ + UnorderedListNode([ [ParagraphNode(wasImplicit: true, links: null, nodes: [TextNode('something')])], [ParagraphNode(wasImplicit: true, links: null, nodes: [TextNode('another')])], ]), @@ -1473,8 +1493,8 @@ void main() { testParse('implicit paragraph with internal
      ', // "* a\n b" '
        \n
      • a
        \n b
        • \n
      ', const [ - ListNode(ListStyle.unordered, [ - [ParagraphNode(wasImplicit: true, links: null, nodes: [ + UnorderedListNode([[ + ParagraphNode(wasImplicit: true, links: null, nodes: [ TextNode('a'), LineBreakInlineNode(), TextNode('\n b'), // TODO: this renders misaligned @@ -1485,13 +1505,15 @@ void main() { testParse('explicit paragraphs', // "* a\n\n b" '
        \n
      • \n

        a

        \n

        b

        \n
        • \n
      ', const [ - ListNode(ListStyle.unordered, [ - [ + UnorderedListNode([[ ParagraphNode(links: null, nodes: [TextNode('a')]), ParagraphNode(links: null, nodes: [TextNode('b')]), ], ]), ]); + + testParseExample(ContentExample.orderedListCustomStart); + testParseExample(ContentExample.orderedListLargeStart); }); testParseExample(ContentExample.spoilerDefaultHeader); @@ -1524,7 +1546,7 @@ void main() { testParse('link in list item', // "* [t](/u)" '
        \n
      • t
        • \n
      ', const [ - ListNode(ListStyle.unordered, [ + UnorderedListNode([ [ParagraphNode(links: null, wasImplicit: true, nodes: [ LinkNode(url: '/u', nodes: [TextNode('t')]), ])], @@ -1585,10 +1607,10 @@ void main() { '
        \n
      1. \n
        \n
        two
        \n
          \n
        • three
          • \n' '
        \n
        \n
        '
         'four\n
        \n\n
        2. \n
      ', const [ - ListNode(ListStyle.ordered, [[ + OrderedListNode([[ QuotationNode([ HeadingNode(level: HeadingLevel.h6, links: null, nodes: [TextNode('two')]), - ListNode(ListStyle.unordered, [[ + UnorderedListNode([[ ParagraphNode(wasImplicit: true, links: null, nodes: [TextNode('three')]), ]]), ]), @@ -1596,7 +1618,7 @@ void main() { CodeBlockSpanNode(text: 'four', type: CodeBlockSpanType.text), ]), ParagraphNode(wasImplicit: true, links: null, nodes: [TextNode('\n\n')]), // TODO avoid this; it renders wrong - ]]), + ]], start: 1), ]); test('all content examples are tested', () { diff --git a/test/widgets/content_test.dart b/test/widgets/content_test.dart index 571034093d..e97e6165f9 100644 --- a/test/widgets/content_test.dart +++ b/test/widgets/content_test.dart @@ -230,6 +230,17 @@ void main() { }); }); + group('ListNodeWidget', () { + testWidgets('ordered list with custom start', (tester) async { + await prepareContent(tester, plainContent('
        \n
      1. third
        2. \n
      2. fourth
        3. \n
      ')); + + expect(find.text('3. '), findsOneWidget); + expect(find.text('4. '), findsOneWidget); + expect(find.text('third'), findsOneWidget); + expect(find.text('fourth'), findsOneWidget); + }); + }); + group('Spoiler', () { testContentSmoke(ContentExample.spoilerDefaultHeader); testContentSmoke(ContentExample.spoilerPlainCustomHeader); From e71ca832300c40ad8728f3ea254fc3931cf5233a Mon Sep 17 00:00:00 2001 From: lakshya1goel Date: Sat, 22 Feb 2025 11:47:56 +0530 Subject: [PATCH 2/2] content: Fix ordered list layout to handle long numbers in
        Fixes: #1356 --- lib/widgets/content.dart | 39 +++++++++++++--------------------- test/widgets/content_test.dart | 31 +++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 24 deletions(-) diff --git a/lib/widgets/content.dart b/lib/widgets/content.dart index 63f9fcab5b..4645ab708d 100644 --- a/lib/widgets/content.dart +++ b/lib/widgets/content.dart @@ -493,33 +493,24 @@ class ListNodeWidget extends StatelessWidget { case UnorderedListNode(): marker = "• "; break; case OrderedListNode(:final start): marker = "${start + index}. "; break; } - return ListItemWidget(marker: marker, nodes: item); + return TableRow(children: [ + Align( + alignment: AlignmentDirectional.topEnd, + child: Text(marker)), + BlockContentList(nodes: item), + ]); }); + return Padding( padding: const EdgeInsets.only(top: 2, bottom: 5), - child: Column(children: items)); - } -} - -class ListItemWidget extends StatelessWidget { - const ListItemWidget({super.key, required this.marker, required this.nodes}); - - final String marker; - final List nodes; - - @override - Widget build(BuildContext context) { - return Row( - mainAxisAlignment: MainAxisAlignment.start, - crossAxisAlignment: CrossAxisAlignment.baseline, - textBaseline: localizedTextBaseline(context), - children: [ - SizedBox( - width: 20, // TODO handle long numbers in
          , like https://github.com/zulip/zulip/pull/25063 - child: Align( - alignment: AlignmentDirectional.topEnd, child: Text(marker))), - Expanded(child: BlockContentList(nodes: nodes)), - ]); + child: Table( + defaultVerticalAlignment: TableCellVerticalAlignment.baseline, + textBaseline: localizedTextBaseline(context), + columnWidths: const { + 0: IntrinsicColumnWidth(), + 1: FlexColumnWidth(), + }, + children: items)); } } diff --git a/test/widgets/content_test.dart b/test/widgets/content_test.dart index e97e6165f9..856274771a 100644 --- a/test/widgets/content_test.dart +++ b/test/widgets/content_test.dart @@ -239,6 +239,37 @@ void main() { expect(find.text('third'), findsOneWidget); expect(find.text('fourth'), findsOneWidget); }); + + testWidgets('list uses correct text baseline alignment', (tester) async { + await prepareContent(tester, plainContent(ContentExample.orderedListLargeStart.html)); + + final table = tester.widget(find.byType(Table)); + check(table.defaultVerticalAlignment).equals(TableCellVerticalAlignment.baseline); + check(table.textBaseline).equals(localizedTextBaseline(tester.element(find.byType(Table)))); + }); + + testWidgets('ordered list markers have enough space to render completely', (tester) async { + await prepareContent(tester, plainContent(ContentExample.orderedListLargeStart.html)); + + final renderParagraph = tester.renderObject(find.textContaining('9999.')) as RenderParagraph; + final textHeight = renderParagraph.size.height; + final lineHeight = renderParagraph.text.style!.height! * renderParagraph.text.style!.fontSize!; + check(textHeight).equals(lineHeight); + check(renderParagraph.didExceedMaxLines).isFalse(); + }); + + testWidgets('ordered list markers are end-aligned', (tester) async { + await prepareContent(tester, plainContent(ContentExample.orderedListLargeStart.html)); + + final marker9999 = tester.renderObject(find.textContaining('9999.')) as RenderParagraph; + final marker10000 = tester.renderObject(find.textContaining('10000.')) as RenderParagraph; + check(marker9999.size.width != marker10000.size.width).isTrue(); + + final marker9999Pos = marker9999.localToGlobal(Offset.zero); + final marker10000Pos = marker10000.localToGlobal(Offset.zero); + check(marker9999Pos.dx + marker9999.size.width) + .equals(marker10000Pos.dx + marker10000.size.width); + }); }); group('Spoiler', () {