content: Add start attribute support for ordered list #1329

lakshya1goel · 2025-02-05T15:26:30Z

Fixes: #59
Fixes: #1356

Screenshot

Before	After

rajveermalviya

Thanks for working on this @lakshya1goel! Some small comments below, otherwise looks good.

lib/widgets/content.dart

lib/model/content.dart

test/widgets/content_test.dart

lakshya1goel · 2025-02-12T20:09:44Z

Thanks for the review @rajveermalviya, pushed the revision. PTAL.

rajveermalviya

Thanks for the revision @lakshya1goel! Left some small comments, mostly nits.

lib/model/content.dart

lib/widgets/content.dart

lakshya1goel · 2025-02-13T13:00:48Z

Pushed the revision, PTAL. Thanks!

rajveermalviya

Thanks for the revision @lakshya1goel!
Apart from one comment, this LGTM! Marking for Greg's review.

lib/widgets/content.dart

gnprice

Thanks @lakshya1goel, and thanks @rajveermalviya for the previous reviews!

Generally this looks good. Comments below, most of them small.

lib/model/content.dart

test/model/content_test.dart

lib/widgets/content.dart

gnprice · 2025-02-14T06:35:14Z

lib/widgets/content.dart

+      child: Table(
+        textBaseline: localizedTextBaseline(context),
+        defaultVerticalAlignment: TableCellVerticalAlignment.baseline,


Do we have an issue in the tracker for the problem this is fixing? Let's file one if not, and then the commit message (and PR) can be marked as fixing it.

I think we didn't have one previously so I have filed #F1356 for it.

lakshya1goel · 2025-02-14T18:18:43Z

Thanks for the detailed review @gnprice, I have filed issue #F1356 and mentioned that in commit and PR as well, also pushed the revision resolving all the comments. PTAL!

gnprice

Thanks for the revision! Mostly small comments now.

gnprice · 2025-02-18T20:59:00Z

lib/model/content.dart

-    switch (element.localName) {
-      case 'ol': listStyle = ListStyle.ordered; break;
-      case 'ul': listStyle = ListStyle.unordered; break;
-    }
-    assert(listStyle != null);


This should still have an assert for this method's expectations on element.localName.

If you look at other methods on this class, you can see that that's the pattern they follow: any facts they assume (which their callers are expected to ensure), they assert at the top. And then down at the if/else below, this assumption is essential for understanding why the logic makes sense, so it's important to make explicit within the method.

(See also #1329 (comment) — this is why I said there that the references to ListStyle could switch to referring to element.localName directly, instead of saying they'd be deleted.)

gnprice · 2025-02-18T20:59:54Z

lib/model/content.dart

+    } else {
+      return UnorderedListNode(items: items, debugHtmlNode: debugHtmlNode);


(Specifically, this only makes sense because we know that element.localName is "ol" at this point, not some arbitrary other value like "p" or "span" or "div".)

gnprice · 2025-02-18T21:07:16Z

test/model/content_test.dart

@@ -1155,6 +1158,32 @@ class ContentExample {
      ], isHeader: false),
    ]),
  ]);
+
+  static const orderedListLargeStart = ContentExample(


nit: in general, try to keep test code in the same order as the code it's testing

Here, that means let's order these examples to match the parsing code — so these list examples go above the spoiler examples, just after the many inline examples.

gnprice · 2025-02-18T21:10:05Z

test/model/content_test.dart

+      OrderedListNode(
+        start: 1,
+        items: [[
        QuotationNode([


nit: keep the existing formatting:

Suggested change

OrderedListNode(

start: 1,

items: [[

QuotationNode([

OrderedListNode(start: 1, items: [[

QuotationNode([

In particular the version in the current revision of the PR doesn't have correct indentation: the QuotationNode inside items needs to be indented more deeply than the line items starts on.

gnprice · 2025-02-18T21:12:04Z

test/model/content_test.dart

+    ],
+  );
+
+  static const orderedListCustomStart = ContentExample(


nit: put this "custom" example before "large" — after all, the "large" value is a custom value too, so it's a particular case of being "custom"

gnprice · 2025-02-18T21:21:30Z

test/widgets/content_test.dart

+      expect(find.text('fourth'), findsOneWidget);
+    });
+
+    testWidgets('list uses correct text baseline alignment', (tester) async {


Hmm, I guess this is good to check too but it's not the thing I was hoping for in #1329 (comment) — it doesn't check that #1356 was (and remains) fixed.

In particular if I make this edit:

--- lib/widgets/content.dart +++ lib/widgets/content.dart @@ -507,7 +507,7 @@ class ListNodeWidget extends StatelessWidget { defaultVerticalAlignment: TableCellVerticalAlignment.baseline, textBaseline: localizedTextBaseline(context), columnWidths: const <int, TableColumnWidth>{ - 0: IntrinsicColumnWidth(), + 0: FixedColumnWidth(20), 1: FlexColumnWidth(), },

then that should reintroduce #1356 — it'd make the new code behave pretty much just like the old code before your fix — but there isn't currently a test that would detect that and fail.

Can you find a way to write a test that checks that #1356 is fixed?

gnprice · 2025-02-18T21:22:12Z

test/widgets/content_test.dart

+      await prepareContent(tester, Directionality(
+        textDirection: TextDirection.rtl,
+        child: plainContent(ContentExample.orderedListLargeStart.html)));
+
+      final tableRtl = tester.widget<Table>(find.byType(Table));
+      check(tableRtl.defaultVerticalAlignment).equals(TableCellVerticalAlignment.baseline);
+      check(tableRtl.textBaseline).equals(localizedTextBaseline(tester.element(find.byType(Table))));


Does the baseline logic interact with the text direction?

I think it doesn't, and it doesn't seem likely that it would. So let's leave this part of the test out, as being redundant.

lakshya1goel · 2025-02-21T10:21:43Z

Pushed the revision @gnprice, PTAL. Thanks!

gnprice

Thanks for the revision! Comments below.

gnprice · 2025-02-22T00:33:49Z

test/model/content_test.dart

+    [OrderedListNode(start: 5, items: [
+        [ParagraphNode(wasImplicit: true, links: null, nodes: [TextNode('fifth')])],
+        [ParagraphNode(wasImplicit: true, links: null, nodes: [TextNode('sixth')])],
+      ])
+    ],


nit: two-space indent; and the two [-enclosed lists start on the same line, so the closing ] can go on the same line:

Suggested change

[OrderedListNode(start: 5, items: [

[ParagraphNode(wasImplicit: true, links: null, nodes: [TextNode('fifth')])],

[ParagraphNode(wasImplicit: true, links: null, nodes: [TextNode('sixth')])],

])

],

[OrderedListNode(start: 5, items: [

[ParagraphNode(wasImplicit: true, links: null, nodes: [TextNode('fifth')])],

[ParagraphNode(wasImplicit: true, links: null, nodes: [TextNode('sixth')])],

])],

gnprice · 2025-02-22T00:36:02Z

test/model/content_test.dart

+  testParseExample(ContentExample.orderedListCustomStart);
+  testParseExample(ContentExample.orderedListLargeStart);
  testParseExample(ContentExample.spoilerDefaultHeader);


nit: use blank lines to group things logically:

Suggested change

testParseExample(ContentExample.orderedListCustomStart);

testParseExample(ContentExample.orderedListLargeStart);

testParseExample(ContentExample.spoilerDefaultHeader);

testParseExample(ContentExample.orderedListCustomStart);

testParseExample(ContentExample.orderedListLargeStart);

testParseExample(ContentExample.spoilerDefaultHeader);

(including the existing blank line that was there between the multi-line group block and these function calls)

gnprice · 2025-02-22T00:36:53Z

test/model/content_test.dart

+  testParseExample(ContentExample.orderedListCustomStart);
+  testParseExample(ContentExample.orderedListLargeStart);


Oh and speaking of grouping logically: these should go inside the "parse lists" group (just above)

gnprice · 2025-02-22T00:40:43Z

test/widgets/content_test.dart

+        find.descendant(of: find.byType(Align), matching: find.byType(Text)));
+
+      for (final text in markerTexts) {


nit: avoid trailing whitespace at end of line

Suggested change

find.descendant(of: find.byType(Align), matching: find.byType(Text)));

for (final text in markerTexts) {

find.descendant(of: find.byType(Align), matching: find.byType(Text)));

for (final text in markerTexts) {

If you read your changes with git log -p, it should highlight this for you. In my terminal it looks like this:

(I recommend regularly reading Git commits — yours and other people's — with git log --stat -p, and using this "secret" for convenient navigation between commits.)

gnprice · 2025-02-22T00:58:41Z

test/widgets/content_test.dart

+        final renderParagraph = tester.renderObject(find.text(text.data!)) as RenderParagraph;
+        final textHeight = renderParagraph.size.height;
+        final lineHeight = renderParagraph.text.style!.height! * renderParagraph.text.style!.fontSize!;
+        check(textHeight).equals(lineHeight);


This is a good strategy. Let's add one other check:

Suggested change

final renderParagraph = tester.renderObject(find.text(text.data!)) as RenderParagraph;

final textHeight = renderParagraph.size.height;

final lineHeight = renderParagraph.text.style!.height! * renderParagraph.text.style!.fontSize!;

check(textHeight).equals(lineHeight);

final renderParagraph = tester.renderObject(find.text(text.data!)) 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();

That way if we had a maxLines: 1 on that Text widget (very similar to how web behaved for years until the recent fix), that would break the test too.

gnprice · 2025-02-22T01:03:21Z

test/widgets/content_test.dart

+      final markerTexts = tester.widgetList<Text>(
+        find.descendant(of: find.byType(Align), matching: find.byType(Text)));


Looking for Align here makes this tied into some fairly internal details of how ListNodeWidget is implemented.

Instead, let's use find.textContaining('9999'). That should find the same Text widget in a way that's more focused on what the user sees, rather than how the implementation works. See:
https://zulip.readthedocs.io/en/latest/testing/philosophy.html#integration-testing-or-unit-testing

(It does make this test dependent on the details of the test data it's using, from ContentExample.orderedListLargeStart. That's OK — that test data exists basically for the sake of this test and the related parsing test.)

Also we only need to check one marker, not both.

gnprice · 2025-02-22T01:04:46Z

test/widgets/content_test.dart

+        final textWidth = renderParagraph.size.width;
+        final renderBox = tester.renderObject(find.ancestor(
+            of: find.text(text.data!),
+            matching: find.byType(Align))) as RenderBox;
+        check(renderBox.size.width >= textWidth).isTrue();


I think this check can't really fail — it's checking that the parent is at least as big as the child, and that's always true unless one uses some fairly special widgets that aren't likely to get involved here. We can leave it out.

gnprice · 2025-02-22T01:07:23Z

test/widgets/content_test.dart

+        final align = tester.widget<Align>(
+          find.ancestor(of: find.text(text.data!), matching: find.byType(Align)));
+        check(align.alignment).equals(AlignmentDirectional.topEnd);


Let's adapt this so that it's in terms of what the user sees, rather than how the implementation code works. (Per https://zulip.readthedocs.io/en/latest/testing/philosophy.html#integration-testing-or-unit-testing again.)

Concretely: the content in this test has two markers. They aren't going to be the same width. So if the alignment is correct, they'll have their right edges at the same x-coordinate, and if the alignment is wrong they'll almost surely have their right edges at different x-coordinates. So to write a test, get the locations of the two marker Texts, confirm the widths are different, and then check the right edges match.

gnprice · 2025-02-22T01:08:10Z

test/widgets/content_test.dart

+      check(table.textBaseline).equals(localizedTextBaseline(tester.element(find.byType(Table))));
+    });
+
+    testWidgets('long ordered list markers render completely and are right-aligned', (tester) async {


nit: this should be two test cases (two testWidgets calls) — one for having enough room, and one for being end-aligned

(also it's not right-aligned in general — it's end-aligned, which means the left side when using an RTL language, like Persian or Arabic or Hebrew)

(it's fine not to add a test for the RTL case, just adjust this test name so it's accurate)

gnprice · 2025-02-22T01:16:23Z

lib/model/content.dart

@@ -251,21 +251,11 @@ class HeadingNode extends BlockInlineContainerNode {
  }
 }

-enum ListStyle { ordered, unordered }
+sealed class ListNode extends BlockContentNode {
+  const ListNode({required this.items, super.debugHtmlNode});


Ah I missed this earlier — let's keep the constructor calls a bit simpler:

Suggested change

const ListNode({required this.items, super.debugHtmlNode});

const ListNode(this.items, {super.debugHtmlNode});

(Just like in the existing version, and like in some other classes in this file: QuotationNode, CodeBlockNode, TextNode, and others where it's clear what a positional argument should mean.)

lakshya1goel · 2025-02-22T06:25:55Z

Thanks for the detailed review @gnprice , I have pushed the revision, PTAL.

gnprice · 2025-02-24T19:34:32Z

(I've just edited the PR description so that it's marked as fixing #1356 too — the GitHub "fixes" syntax doesn't understand "and", and needs the word "fixes" to be right next to the issue number.)

gnprice

Thanks for the revision! Just a couple of small comments now.

gnprice · 2025-02-25T22:23:54Z

test/widgets/content_test.dart

+      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);


Cool, this test is accurate, but can be simplified using getRect:

final marker9999 = tester.getRect(find.textContaining('9999.'));

then marker9999.width and marker9999.right.

(If you dig into the implementation of getRect, you'll see it's doing something similar to this test's logic.)

gnprice · 2025-02-25T22:31:52Z

test/model/content_test.dart

          ])]
-        ])],
-      ])],
+        ], start: 1)],


nit: keep start: 1 as the first argument, before the positional items argument

Dart has the handy feature of allowing named and positional arguments to be interleaved in any order, including named first. That's useful to do in a case like this where there's a named argument that's short and fits easily on the first line and then a longer multi-line argument that's positional.

lakshya1goel · 2025-02-26T10:42:51Z

Pushed the revision, Please have a look @gnprice . Thanks!

Fixes: zulip#59

Fixes: zulip#1356

…in a bit more A couple of the extension members added here don't actually end up getting used (because the "height of one line" check feels like it benefits from describing the calculation more explicitly). Still the fact we're referring to those properties in a test makes a good prompt to add the extension so future tests can use it.

gnprice · 2025-03-03T21:47:29Z

Thanks! Looks good; merging.

I made a few tweaks in the first commit along the lines of #1329 (comment), for two-space indentation and to avoid other formatting changes. Also added one commit on top which I recommend reading:
e0ee70e content test [nfc]: Use checks-extensions in list-marker tests; explain a bit more

since it points to an important bit of test-writing style which I'd missed in previous reviews.

In particular, a change like this:

-      check(marker9999.right).equals(marker10000.right);
+      check(marker9999).right.equals(marker10000.right);

means that if that check fails, the resulting error will be more informative because it will have the whole Rect value marker9999 rather than just the one piece of it that is marker9999.right. To see the difference, try sabotaging the test to fail and then trying both versions, with .right inside and outside the parens, to see the output.

lakshya1goel force-pushed the issue59 branch from 6dfb1da to c61baff Compare February 6, 2025 17:18

lakshya1goel mentioned this pull request Feb 10, 2025

Handle <ol start=…> #59

Closed

gnprice added the maintainer review PR ready for review by Zulip maintainers label Feb 11, 2025

gnprice assigned rajveermalviya Feb 11, 2025

gnprice requested a review from rajveermalviya February 11, 2025 07:31

rajveermalviya requested changes Feb 11, 2025

View reviewed changes

lakshya1goel force-pushed the issue59 branch from c61baff to 348edbc Compare February 12, 2025 14:42

lakshya1goel requested a review from rajveermalviya February 12, 2025 17:55

rajveermalviya requested changes Feb 13, 2025

View reviewed changes

lakshya1goel force-pushed the issue59 branch from a04f584 to 4cd6d1b Compare February 13, 2025 12:10

lakshya1goel requested a review from rajveermalviya February 13, 2025 13:00

rajveermalviya approved these changes Feb 13, 2025

View reviewed changes

lib/widgets/content.dart Show resolved Hide resolved

rajveermalviya requested a review from gnprice February 13, 2025 14:13

rajveermalviya assigned gnprice and unassigned rajveermalviya Feb 13, 2025

rajveermalviya added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Feb 13, 2025

lakshya1goel force-pushed the issue59 branch from 4cd6d1b to c71b346 Compare February 13, 2025 14:22

gnprice reviewed Feb 14, 2025

View reviewed changes

lakshya1goel force-pushed the issue59 branch 2 times, most recently from 7fb35b3 to 0c6fda4 Compare February 14, 2025 17:11

lakshya1goel requested a review from gnprice February 14, 2025 18:16

gnprice reviewed Feb 18, 2025

View reviewed changes

lakshya1goel force-pushed the issue59 branch from 53616a7 to 306efe6 Compare February 21, 2025 10:05

lakshya1goel requested a review from gnprice February 21, 2025 10:12

gnprice reviewed Feb 22, 2025

View reviewed changes

lakshya1goel force-pushed the issue59 branch from 306efe6 to e71ca83 Compare February 22, 2025 06:19

lakshya1goel requested a review from gnprice February 22, 2025 06:25

lakshya1goel mentioned this pull request Feb 22, 2025

Fix ordered list layout to handle long numbers in <ol> #1356

Closed

gnprice mentioned this pull request Feb 24, 2025

issue 1356 fixed #1372

Closed

gnprice reviewed Feb 25, 2025

View reviewed changes

lakshya1goel force-pushed the issue59 branch from e71ca83 to 564047c Compare February 26, 2025 10:31

lakshya1goel requested a review from gnprice February 26, 2025 10:42

lakshya1goel and others added 3 commits March 3, 2025 13:42

content: Add start attribute support for ordered list

28600b9

Fixes: zulip#59

content: Fix ordered list layout to handle long numbers in <ol>

ce338ab

Fixes: zulip#1356

gnprice force-pushed the issue59 branch from 564047c to e0ee70e Compare March 3, 2025 21:47

gnprice merged commit e0ee70e into zulip:main Mar 3, 2025

		} else {
		return UnorderedListNode(items: items, debugHtmlNode: debugHtmlNode);

		testParseExample(ContentExample.orderedListCustomStart);
		testParseExample(ContentExample.orderedListLargeStart);

		find.descendant(of: find.byType(Align), matching: find.byType(Text)));

		for (final text in markerTexts) {

		final markerTexts = tester.widgetList<Text>(
		find.descendant(of: find.byType(Align), matching: find.byType(Text)));

	const ListNode({required this.items, super.debugHtmlNode});
	const ListNode(this.items, {super.debugHtmlNode});

content: Add start attribute support for ordered list #1329

content: Add start attribute support for ordered list #1329

Conversation

lakshya1goel commented Feb 5, 2025 • edited by gnprice Loading

Screenshot

rajveermalviya left a comment

Choose a reason for hiding this comment

lakshya1goel commented Feb 12, 2025

rajveermalviya left a comment

Choose a reason for hiding this comment

lakshya1goel commented Feb 13, 2025

rajveermalviya left a comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lakshya1goel commented Feb 14, 2025

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lakshya1goel commented Feb 21, 2025

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lakshya1goel commented Feb 22, 2025 • edited Loading

gnprice commented Feb 24, 2025

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lakshya1goel commented Feb 26, 2025

gnprice commented Mar 3, 2025

lakshya1goel commented Feb 5, 2025 •

edited by gnprice

Loading

lakshya1goel commented Feb 22, 2025 •

edited

Loading