content: Handle `<table>` elements #1031

rajveermalviya · 2024-10-28T20:03:35Z

Fixes: #360

Flutter	Web

chrisbobbe

Great to see this! I might be jumping on this draft PR too early, but I had a few thoughts from a quick skim 🙂. One comment below, and:

In the 'strong (bold)' tests, let's test that a **bold** span in a header cell gets a font weight of 900, even when that span is nested inside another span. (900 is 700 from the header-cell styling plus 200 from the bold-span code.) So, another pair of tests like these:

    testFontWeight('in spoiler header',
      expectedWght: 900,
      // ```spoiler regular **bold**
      // content
      // ```
      content: plainContent(
        '<div class="spoiler-block"><div class="spoiler-header">\n'
          '<p>regular <strong>bold</strong></p>\n'
          '</div><div class="spoiler-content" aria-hidden="true">\n'
          '<p>content</p>\n'
          '</div></div>'
      ),
      styleFinder: findWordBold,
    );

    testFontWeight('in different kind of span in spoiler header',
      expectedWght: 900,
      // ```spoiler *italic **bold***
      // content
      // ```
      content: plainContent(
        '<div class="spoiler-block"><div class="spoiler-header">\n'
          '<p><em>italic <strong>bold</strong></em></p>\n'
          '</div><div class="spoiler-content" aria-hidden="true">\n'
          '<p>content</p>\n'
          '</div></div>'
      ),
      styleFinder: findWordBold,
    );

chrisbobbe · 2024-10-28T23:10:04Z

lib/widgets/content.dart

+                    .merge(weightVariableTextStyle(context, wght: 700))
+                : DefaultTextStyle.of(context).style,
+              textAlign: node.textAlign))
+        : const SizedBox.shrink());


Should we include the Padding in this case too? This looks like it could make a cell that doesn't take up any space at all, and I don't think that's consistent with the web app. Here's what the web app does with a table that's nothing but empty cells:

https://chat.zulip.org/#narrow/channel/7-test-here/topic/Chris/near/1971419

and each one does take up some space, and from the inspector it looks like that's coming from some padding (though 4px in all directions instead of 5px).

rajveermalviya · 2024-11-04T19:19:51Z

Thanks for the initial comments @chrisbobbe! This is now ready for review, PTAL.

chrisbobbe

Thanks! Comments below.

I wonder if tools/content/check-features shows any surprises after implementing table support; do you have time to run that?

chrisbobbe · 2024-11-04T20:40:58Z

lib/model/content.dart

@@ -1,3 +1,5 @@
+import 'dart:ui';


We might prefer keeping dart:ui out of our model code, I'm not sure. @gnprice, what do you think? This revision uses this to put TextAlign enum values in content-node objects. Instead, we could use our own small custom enum and give our widget code the job of invoking dart:ui code.

Yeah, that adjustment SGTM.

We already import package:flutter/foundation.dart in a number of places in our model code, I think mostly for ChangeNotifier, and that transitively pulls in dart:ui. So if something would be messy to do without dart:ui, it'd be fine to import it.

But referring to something UI-specific like TextAlign is definitely a good prompt to think about if the logic would best live in our widgets code. In this example, I think this resulting change in the later revision, where this bit of logic moved from the parser to the widget:

switch (cellStyle) { case null: - // Default text alignment specified in for `tr > th` element - // in `web/styles/rendered_markdown.css`: - // https://github.com/zulip/zulip/blob/d556c0e0a5e9e8ba0ff548354bf2ba6ef2c97d4b/web/styles/rendered_markdown.css#L140 - textAlign = isHeader ? TextAlign.left : null; + textAlignment = TableCellTextAlignment.defaults;

+ // The web client sets `text-align: left;` for the header cells, + // overriding the default browser alignment (which is `center` for header + // and `start` for body). By default, the [Table] widget uses `start` for + // text alignment, a saner choice that supports RTL text. So, defer to that. + // See discussion: + // https://github.com/zulip/zulip-flutter/pull/1031#discussion_r1831950371 + TableCellTextAlignment.defaults => null,

was a improvement in clarity.

chrisbobbe · 2024-11-07T02:01:01Z

lib/model/content.dart

+      switch (cellStyle) {
+        case null:
+          // Default text alignment specified in for `tr > th` element
+          // in `web/styles/rendered_markdown.css`:
+          //  https://github.com/zulip/zulip/blob/d556c0e0a5e9e8ba0ff548354bf2ba6ef2c97d4b/web/styles/rendered_markdown.css#L140
+          textAlign = isHeader ? TextAlign.left : null;
+        case 'text-align: left;':
+          textAlign = TextAlign.left;
+        case 'text-align: center;':
+          textAlign = TextAlign.center;
+        case 'text-align: right;':
+          textAlign = TextAlign.right;
+        default:
+          return null;
+      }


Some thoughts on what we're modeling, and how to split responsibilities between model code and UI code.

A message author has four options to set the text alignment of a column's cells (as in GFM):

|: - | all cells' text left-aligned

|: - :| all cells' text center-aligned

| - :| all cells' text right-aligned

| - | accept default for each cell.

In the HTML we're parsing, the setting is represented as:

all the column's cells have style="text-align: left;"

all the column's cells have style="text-align: center;"

all the column's cells have style="text-align: right;"

all the column's cells have no style attribute.

Putting something on each cell is understandable, I guess, since there isn't a per-column element to attach it to; a table's cells are organized as a list of rows, not columns.

I have doubts about RTL support with the "left" and "right" options, but I don't fully understand the bugs involved, so for now I think it's best to just interpret and apply those options literally. So, as in this revision, left-aligning each cell that has text-align: left; and right-aligning each cell that has text-align: right;, whether the text is LTR or RTL. Let's include a TODO(i18n) pointing to that RTL issue I filed in zulip/zulip.

The "center" option should be fine because center means the same in RTL and LTR.

For the fourth option, "defaults", how about we let UI code handle the specifics of what the defaults are? So, remove (from this file) the code that sets left-alignment for header cells. Web's idea of "default" alignment comes from the user-agent stylesheet (typically start-alignment for non-header cells) and from some custom CSS that you linked to (left-alignment for header cells)¹. Actually, web's choice to left-align instead of start-align header cells sounds like a bug that we don't want to carry over; let's start-align all cells. From a quick test, that's the default behavior from Flutter's table widgets anyway, so that'll be easy.

How about representing those four options with an enum, like:

// The text-alignment setting that applies to a cell's column, from the delimiter row. // // See GitHub-flavored Markdown: // https://github.github.com/gfm/#tables-extension- enum TableColumnTextAlignment { /// All cells' text left-aligned, represented in Markdown as `|: --- |`. left, // TODO(i18n) RTL issues? https://github.com/zulip/zulip/issues/32265 /// All cells' text center-aligned, represented in Markdown as `|: --- :|`. center, /// All cells' text right-aligned, represented in Markdown as `| --- :|`. right, // TODO(i18n) RTL issues? https://github.com/zulip/zulip/issues/32265 /// Cells' text aligned the default way, represented in Markdown as `| --- |`. defaults, }

? And the widget code does the appropriate thing with those values. It'll be fine to store the value on each cell, given that our representation (like the HTML) lacks a single per-column place for it.

I understand we could get correct behavior by just having a dart:ui TextAlign? on the TableCellNode. But I think it's helpful to connect the HTML to the per-column setting, for readers, and the enum's dartdoc is a convenient place to do that. Also, as mentioned, I like delegating the details of the defaults option to UI code, even if the details are very simple, as they (currently) are.

Footnotes

Apparently the user-agent stylesheet typically center-aligns text in header cells; I guess the custom CSS is a reaction to that. ↩

chrisbobbe · 2024-11-12T20:22:05Z

lib/widgets/content.dart

+    return TableCell(
+      verticalAlignment: TableCellVerticalAlignment.middle,
+      child: Padding(
+        padding: const EdgeInsets.all(4),


Suggested change

padding: const EdgeInsets.all(4),

// Web has 4px padding and 1px border on all sides.

// In web, the 1px border grows each cell by 0.5px in all directions.

// Our border doesn't affect the layout, it's just painted on,

// so we add 0.5px on all sides to match web.

padding: const EdgeInsets.all(4 + 0.5),

Interesting, found an upstream issue for this: flutter/flutter#78691.
Included the link for reference.

rajveermalviya · 2024-11-15T17:15:23Z

Thanks for the review @chrisbobbe! Pushed a new revision.
I also ran the tools/content/check-features script and found that with this PR there are zero failures for the <table> content 🎉. This is hopefully accurate, as the current parser impl is overly strict about missing or extra classes, attributes and nodes.

chrisbobbe

Thanks! Small comments below.

chrisbobbe · 2024-11-20T19:53:57Z

lib/model/content.dart

+// row based and needs alignment information to be per cell.
+enum TableCellTextAlignment {
+  /// Text inside the table cell is left-aligned.
+  // Represented in markdown as `|: --- |` for the specific column in the delimeter row.


nit: "delimiter" (here and below)

chrisbobbe · 2024-11-20T21:16:32Z

lib/model/content.dart

+/// The text-alignment that applies to text inside the table cell.
+///
+/// See GitHub-flavored Markdown:
+///   https://github.github.com/gfm/#tables-extension-
+// In Markdown, alignment is defined per column using the delimiter row.
+// However, the generated HTML specifies alignment for each cell in a row
+// individually, that matches the UI widget implementation which is also
+// row based and needs alignment information to be per cell.
+enum TableCellTextAlignment {
+  /// Text inside the table cell is left-aligned.
+  // Represented in markdown as `|: --- |` for the specific column in the delimeter row.
+  // TODO(i18n) RTL issues? https://github.com/zulip/zulip/issues/32265
+  left,
+  /// Text inside the table cell is center-aligned.
+  // Represented in markdown as `|: --- :|` for the specific column in the delimeter row.
+  center,
+  /// Text inside the table cell is right-aligned.
+  // Represented in markdown as `| --- :|` for the specific column in the delimeter row.
+  // TODO(i18n) RTL issues? https://github.com/zulip/zulip/issues/32265
+  right,
+  /// Text inside the table cell is aligned the default way.
+  // Represented in markdown as `| --- |` for the specific column in the delimeter row.
+  defaults
+}


In how we name and document this enum, I'd like to be more explicit that what's being represented is a per-column setting, in the string from the server and in our model. That's how users experience the feature: we tell them they can set the alignment per-column, but not per individual cell (or for a row, or for the whole table, etc.).

It's true that the enum value is repeated on all of a column's cells, and our parsing logic has to understand that fact in order to recognize what the setting is for a given column. I also think it's fine for our model to repeat something on all of a column's cells too, because that's convenient for our parsing code and our UI code. But when doing those things, I'd still like the per-column abstraction to be clear somewhere, and I think the best place for that is in this enum's name and documentation. As in my proposal in #1031 (comment) :

// The text-alignment setting that applies to a cell's column, from the delimiter row. // // See GitHub-flavored Markdown: // https://github.github.com/gfm/#tables-extension- enum TableColumnTextAlignment { /// All cells' text left-aligned, represented in Markdown as `|: --- |`. left, // TODO(i18n) RTL issues? https://github.com/zulip/zulip/issues/32265 /// All cells' text center-aligned, represented in Markdown as `|: --- :|`. center, /// All cells' text right-aligned, represented in Markdown as `| --- :|`. right, // TODO(i18n) RTL issues? https://github.com/zulip/zulip/issues/32265 /// Cells' text aligned the default way, represented in Markdown as `| --- |`. defaults, }

In theory, our parsing code could grow a validation step to make sure the setting is expressed coherently, i.e., scan through a column's cells to check that their style attribute is consistent. I don't think we have to do that (could be expensive), but I do think it would be consistent with what's supposed to be represented.

I think the explicit per-column abstraction will be helpful when we come back to address the RTL issues. As part of that work, we'll want to get clear ideas of what we expect to happen vs. what's actually happening, at all of the steps from the compose-box input to how the message appears in the message list.

Makes sense, updated the enum's name and the documentation with this proposal.

rajveermalviya · 2024-11-21T18:06:33Z

Thanks for the review @chrisbobbe! Pushed a new revision, PTAL.

chrisbobbe · 2024-11-22T04:12:46Z

Thanks, LGTM! Marking for Greg's review.

gnprice

Thanks @rajveermalviya for building this, and @chrisbobbe for the previous reviews!

Generally this looks great. Various comments below, mainly small.

gnprice · 2024-11-25T05:00:18Z

lib/model/content.dart

+  @override
+  void debugFillProperties(DiagnosticPropertiesBuilder properties) {
+    super.debugFillProperties(properties);
+    properties.add(DiagnosticsProperty<bool>('isHeader', isHeader));


nit: instead of DiagnosticsProperty<bool>, use FlagProperty

(see the latter's docs; and for examples, see the handful of existing places we use it in this file)

gnprice · 2024-11-25T05:23:54Z

test/model/content_test.dart

+  static const tableWithDifferentTextAligmentInColumns = ContentExample(
+    'table with different text aligment in columns',


nit: spelling

Suggested change

static const tableWithDifferentTextAligmentInColumns = ContentExample(

'table with different text aligment in columns',

static const tableWithDifferentTextAlignmentInColumns = ContentExample(

'table with different text alignment in columns',

gnprice · 2024-11-25T05:26:31Z

lib/model/content.dart

+  @override
+  void debugFillProperties(DiagnosticPropertiesBuilder properties) {
+    super.debugFillProperties(properties);
+    properties.add(DiagnosticsProperty<TableColumnTextAlignment>('textAlign', textAlignment));


nit: use EnumProperty

And set defaultValue to TableColumnTextAlignment.defaults — that way this won't get printed when that's the value, since that's the typical value and it's boring for a cell/column to have that value

gnprice · 2024-11-25T05:31:39Z

lib/model/content.dart

      case 'blockquote':
      case 'div':
+      case 'table':


nit: keep in same order as parsing logic above, for ease of comparison

Suggested change

case 'blockquote':

case 'div':

case 'table':

case 'blockquote':

case 'table':

case 'div':

gnprice · 2024-11-25T05:51:02Z

lib/model/content.dart

+    TableCellNode? parseTableCell(dom.Element node, bool isHeader) {
+      assert(node.localName == (isHeader ? 'th' : 'td'));
+      assert(node.className.isEmpty);
+      assert(node.attributes.length <= 1);


Our existing parsing code tolerates attributes it didn't expect, while being strict about classes it didn't expect. So let's follow that here.

(It's possible somewhere else would be a better place to draw the line in general. But until we revisit that, it's easier to think about if the line is the same for different types of content.)

gnprice · 2024-11-25T06:07:51Z

lib/widgets/content.dart

+          ? _buildBlockInlineContainer(
+            node: node,


nit: indentation

Suggested change

? _buildBlockInlineContainer(

node: node,

? _buildBlockInlineContainer(

node: node,

gnprice · 2024-11-25T06:09:31Z

lib/widgets/content.dart

+        child: node.nodes.isNotEmpty
+          ? _buildBlockInlineContainer(


nit: put short exceptional case first, then long normal case (just like we regularly do with early returns)

Suggested change

child: node.nodes.isNotEmpty

? _buildBlockInlineContainer(

child: node.nodes.isEmpty

? const SizedBox.shrink()

: _buildBlockInlineContainer(

gnprice · 2024-11-25T06:47:51Z

test/model/content_test.dart

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


This and the one with links in body cells can be just one example, with one column and a total of two cells, and a link in each of them.

That'd cover everything these do, right?

gnprice · 2024-11-25T06:49:21Z

test/model/content_test.dart

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


How about a version of this with just one column, and one image?

gnprice · 2024-11-25T06:56:59Z

lib/model/content.dart

+        if (node.localName != 'tr') return false;
+        if (node.className.isNotEmpty) return false;
+        if (node.attributes.isNotEmpty) return false;
+        if (node.nodes.isEmpty) return false;


It looks like this case can happen:
https://chat.zulip.org/#narrow/channel/7-test-here/topic/table/near/1987383

| table | | - |

… Huh but I'm puzzled because the app with this PR handles that message just fine. Not sure how.

Oh I see, it's that the HTML has no tbody element at all.

Anyway, let's have a test for that edge case.

(I found this edge case by following the handy link to here: https://github.github.com/gfm/#tables-extension- and then reading through it looking for any odd cases that would make it through to the HTML. That's the only one I found.)

Hmm, looks like for this case the generated HTML adds the empty <td> node:

<table> <thead> <tr> <th>table</th> </tr> </thead> <tbody> <tr> <td></td> </tr> </tbody> </table>

Something similar happens when markdown misses a column in the body row, the generated HTML here again contains an empty <td> node for the missing cell in markdown:

| a | b | | - | - | | a |

<table> <thead> <tr> <th>a</th> <th>b</th> </tr> </thead> <tbody> <tr> <td>a</td> <td></td> </tr> </tbody> </table>

I've added tests for these two cases, and kept this assert in to catch if that changes in future.

Additionally, I've also restructured the parsing code for <thead> and <tbody> elements to ensure that both nodes are always present.

Hmm, looks like for this case the generated HTML adds the empty <td> node:

I see — and a <tr> around that. I guess that also suffices for causing this code (with the rejection on node.nodes.isEmpty) to work correctly.

I was looking at the GFM spec:
https://github.github.com/gfm/#example-205
but hadn't checked what Zulip actually produces. Good to have checked!

Something similar happens when markdown misses a column in the body row,

Yeah. That part matches the spec:
https://github.github.com/gfm/#example-204

rajveermalviya · 2024-11-25T18:20:15Z

Thanks for the review @gnprice! Pushed a new revision, PTAL.

gnprice · 2024-11-25T19:19:25Z

lib/model/content.dart

+    properties.add(EnumProperty<TableColumnTextAlignment>(
+      'textAlignment', textAlignment,


nit: type can be inferred

Suggested change

properties.add(EnumProperty<TableColumnTextAlignment>(

'textAlignment', textAlignment,

properties.add(EnumProperty('textAlignment', textAlignment,

gnprice · 2024-11-25T19:27:54Z

lib/model/content.dart

+        if (node.localName != 'tr') return false;
+        if (node.className.isNotEmpty) return false;
+        if (node.attributes.isNotEmpty) return false;
+        if (node.nodes.isEmpty) return false;


Hmm, looks like for this case the generated HTML adds the empty <td> node:

I see — and a <tr> around that. I guess that also suffices for causing this code (with the rejection on node.nodes.isEmpty) to work correctly.

I was looking at the GFM spec:
https://github.github.com/gfm/#example-205
but hadn't checked what Zulip actually produces. Good to have checked!

Something similar happens when markdown misses a column in the body row,

Yeah. That part matches the spec:
https://github.github.com/gfm/#example-204

gnprice

Thanks for the revision! Comments below — one more test case and otherwise all nits.

gnprice · 2024-11-25T19:31:55Z

lib/model/content.dart

+        if (tbodyElement.className.isNotEmpty) return null;
+        if (tbodyElement.nodes.isEmpty) return null;
+
+        late final int headerColumnCount;


nit: can avoid late:

Suggested change

late final int headerColumnCount;

final int headerColumnCount;

That way the compiler is asked to prove to itself that the variable gets correctly initialized, so we'd get a static error if some future change broke that.

gnprice · 2024-11-25T19:34:31Z

test/model/content_test.dart

+  static const tableWithDifferentTextAlignmentInColumns = ContentExample(
+    'table with different text aligment in columns',


nit: bump #1031 (comment) 🙂

Suggested change

static const tableWithDifferentTextAlignmentInColumns = ContentExample(

'table with different text aligment in columns',

static const tableWithDifferentTextAlignmentInColumns = ContentExample(

'table with different text alignment in columns',

gnprice · 2024-11-25T19:43:02Z

lib/model/content.dart

+    super.debugHtmlNode,
+    required super.nodes,
+    required super.links,
+    required this.textAlignment,


nit:

content: Handle column `text-alignment` in `<table>`

The backticks on "text-alignment" mark it as code, not prose. When a name is formatted as code, it should match exactly the name of something it's referring to in the code. But I don't think there's anything in the code called text-alignment.

Could make it not code, just normal English prose:

content: Handle column text-alignment in <table>

Or could refer to a name found in the code; for example:

content: Handle text-align in <table>

(for the CSS property; which then isn't so much about a column as a cell).

gnprice · 2024-11-25T19:43:45Z

lib/model/content.dart

+    properties.add(EnumProperty(
+      'textAlignment', textAlignment,


nit: tighten (bump #1031 (comment))

Suggested change

properties.add(EnumProperty(

'textAlignment', textAlignment,

properties.add(EnumProperty('textAlignment', textAlignment,

gnprice · 2024-11-25T19:56:13Z

test/model/content_test.dart

+    ])
+  ]);


nit: use trailing comma whenever not closing list on same line

Suggested change

])

]);

]),

]);

(here and a couple of other spots)

gnprice · 2024-11-25T19:58:02Z

test/model/content_test.dart

+  static const tableWithImagesInCells = ContentExample(
+    'table with images in cells',


nit: just one image (and "in cells" is redundant):

Suggested change

static const tableWithImagesInCells = ContentExample(

'table with images in cells',

static const tableWithImage = ContentExample(

'table with image',

gnprice · 2024-11-25T19:59:50Z

test/model/content_test.dart

+    ])
+  ]);
+
+  static const tableWithoutAnyBodyCellsInMarkdown = ContentExample(


Here's a comment to clarify a bit why we bother to have this test case:

Suggested change

static const tableWithoutAnyBodyCellsInMarkdown = ContentExample(

// As is, this HTML doesn't look particularly different to our parser.

// But if Zulip's table support followed GFM, this would have no <tbody>:

// https://github.github.com/gfm/#example-205

// https://github.com/zulip/zulip-flutter/pull/1031#discussion_r1855931989

static const tableWithoutAnyBodyCellsInMarkdown = ContentExample(

gnprice · 2024-11-25T20:02:31Z

lib/widgets/content.dart

-      style: widget.style, nodes: widget.nodes);
+      style: widget.style, nodes: widget.nodes, textAlign: widget.textAlign);


Cool, I think this fixes #1031 (comment) .

Let's have a test case that would have caught that bug. Doesn't need to exercise all the possible alignments (that's covered by another test case you already have), just one of them.

rajveermalviya · 2024-11-25T20:42:23Z

Thanks for the review @gnprice! Pushed a new revision, PTAL.

Fixes: zulip#360

gnprice · 2024-11-25T20:55:18Z

Thanks! Looks good; merging.

rajveermalviya force-pushed the pr-tables branch 2 times, most recently from 8db80df to ce3b7fe Compare October 28, 2024 20:24

chrisbobbe reviewed Oct 28, 2024

View reviewed changes

rajveermalviya force-pushed the pr-tables branch from ce3b7fe to 8bbca00 Compare November 4, 2024 19:07

rajveermalviya requested a review from chrisbobbe November 4, 2024 19:19

rajveermalviya marked this pull request as ready for review November 4, 2024 19:32

PIG208 assigned chrisbobbe Nov 12, 2024

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Nov 12, 2024

chrisbobbe reviewed Nov 12, 2024

View reviewed changes

rajveermalviya force-pushed the pr-tables branch 2 times, most recently from 9450a99 to 979b7ce Compare November 15, 2024 16:51

chrisbobbe reviewed Nov 20, 2024

View reviewed changes

rajveermalviya force-pushed the pr-tables branch from 979b7ce to 2e5200d Compare November 21, 2024 15:39

rajveermalviya requested a review from chrisbobbe November 21, 2024 18:06

chrisbobbe assigned gnprice and unassigned chrisbobbe Nov 22, 2024

chrisbobbe 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 Nov 22, 2024

gnprice reviewed Nov 25, 2024

View reviewed changes

rajveermalviya force-pushed the pr-tables branch from 2e5200d to 0bc7383 Compare November 25, 2024 17:47

rajveermalviya requested a review from gnprice November 25, 2024 18:20

gnprice reviewed Nov 25, 2024

View reviewed changes

rajveermalviya force-pushed the pr-tables branch from 0bc7383 to 0b6fb83 Compare November 25, 2024 19:36

gnprice reviewed Nov 25, 2024

View reviewed changes

rajveermalviya force-pushed the pr-tables branch from 0b6fb83 to f8b1334 Compare November 25, 2024 20:38

rajveermalviya requested a review from gnprice November 25, 2024 20:42

rajveermalviya added 2 commits November 25, 2024 12:53

content: Handle <table> elements

9d02ab8

Fixes: zulip#360

content: Handle column text-alignment in <table>

15705ad

gnprice force-pushed the pr-tables branch from f8b1334 to 15705ad Compare November 25, 2024 20:55

gnprice merged commit 15705ad into zulip:main Nov 25, 2024

rajveermalviya deleted the pr-tables branch November 26, 2024 10:57

-        padding: const EdgeInsets.all(4),
+        // Web has 4px padding and 1px border on all sides.
+        // In web, the 1px border grows each cell by 0.5px in all directions.
+        // Our border doesn't affect the layout, it's just painted on,
+        // so we add 0.5px on all sides to match web.
+        padding: const EdgeInsets.all(4 + 0.5),

		static const tableWithDifferentTextAligmentInColumns = ContentExample(
		'table with different text aligment in columns',

-        child: node.nodes.isNotEmpty
-          ? _buildBlockInlineContainer(
+        child: node.nodes.isEmpty
+          ? const SizedBox.shrink()
+          : _buildBlockInlineContainer(

		properties.add(EnumProperty<TableColumnTextAlignment>(
		'textAlignment', textAlignment,

	properties.add(EnumProperty<TableColumnTextAlignment>(
	'textAlignment', textAlignment,
	properties.add(EnumProperty('textAlignment', textAlignment,

	late final int headerColumnCount;
	final int headerColumnCount;

		static const tableWithDifferentTextAlignmentInColumns = ContentExample(
		'table with different text aligment in columns',

	properties.add(EnumProperty(
	'textAlignment', textAlignment,
	properties.add(EnumProperty('textAlignment', textAlignment,

-    ])
-  ]);
+    ]),
+  ]);

		static const tableWithImagesInCells = ContentExample(
		'table with images in cells',

-  static const tableWithoutAnyBodyCellsInMarkdown = ContentExample(
+  // As is, this HTML doesn't look particularly different to our parser.
+  // But if Zulip's table support followed GFM, this would have no <tbody>:
+  //   https://github.github.com/gfm/#example-205
+  //   https://github.com/zulip/zulip-flutter/pull/1031#discussion_r1855931989
+  static const tableWithoutAnyBodyCellsInMarkdown = ContentExample(

		style: widget.style, nodes: widget.nodes);
		style: widget.style, nodes: widget.nodes, textAlign: widget.textAlign);

content: Handle <table> elements #1031

content: Handle <table> elements #1031

Conversation

rajveermalviya commented Oct 28, 2024 • edited Loading

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Nov 4, 2024

chrisbobbe 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

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Nov 15, 2024

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

rajveermalviya commented Nov 21, 2024

chrisbobbe commented Nov 22, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajveermalviya commented Nov 25, 2024

Choose a reason for hiding this 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

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

rajveermalviya commented Nov 25, 2024

gnprice commented Nov 25, 2024

content: Handle `<table>` elements #1031

content: Handle `<table>` elements #1031

rajveermalviya commented Oct 28, 2024 •

edited

Loading

rajveermalviya Nov 21, 2024 •

edited

Loading