content: Add start attribute support for ordered list #1329
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -230,6 +230,45 @@ void main() { | |
| }); | ||
| }); | ||
|
|
||
| group('ListNodeWidget', () { | ||
| testWidgets('ordered list with custom start', (tester) async { | ||
| await prepareContent(tester, plainContent('<ol start="3">\n<li>third</li>\n<li>fourth</li>\n</ol>')); | ||
| expect(find.text('3. '), findsOneWidget); | ||
| expect(find.text('4. '), findsOneWidget); | ||
| expect(find.text('third'), findsOneWidget); | ||
| expect(find.text('fourth'), findsOneWidget); | ||
| }); | ||
|
|
||
| testWidgets('list uses correct text baseline alignment', (tester) async { | ||
|
There was a problem hiding this comment. 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? |
||
| await prepareContent(tester, plainContent(ContentExample.orderedListLargeStart.html)); | ||
| final table = tester.widget<Table>(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 marker = tester.renderObject(find.textContaining('9999.')) as RenderParagraph; | ||
| // The marker has the height of just one line of text, not more. | ||
| final textHeight = marker.size.height; | ||
| final lineHeight = marker.text.style!.height! * marker.text.style!.fontSize!; | ||
| check(textHeight).equals(lineHeight); | ||
| // The marker's text didn't overflow to more lines | ||
| // (and get cut off by a `maxLines: 1`). | ||
| check(marker).didExceedMaxLines.isFalse(); | ||
| }); | ||
|
|
||
| testWidgets('ordered list markers are end-aligned', (tester) async { | ||
| await prepareContent(tester, plainContent(ContentExample.orderedListLargeStart.html)); | ||
| final marker9999 = tester.getRect(find.textContaining('9999.')); | ||
| final marker10000 = tester.getRect(find.textContaining('10000.')); | ||
| // The markers are aligned at their right edge... | ||
| check(marker9999).right.equals(marker10000.right); | ||
| // ... and not because they somehow happen to have the same width. | ||
| check(marker9999).width.isLessThan(marker10000.width); | ||
| }); | ||
| }); | ||
|
|
||
| group('Spoiler', () { | ||
| testContentSmoke(ContentExample.spoilerDefaultHeader); | ||
| testContentSmoke(ContentExample.spoilerPlainCustomHeader); | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.localNamedirectly, instead of saying they'd be deleted.)