content: Support spoilers! #503

chrisbobbe · 2024-02-02T01:51:38Z

TODO tests, but here's my code so far.

Fixes: #358

gnprice · 2024-02-05T23:51:12Z

Thanks! This direction looks good. I like how the open/close animations are very smooth.

The two prep commits toward #497 look good. I've just sent #505 incorporating those and finishing up the rest of #497.

chrisbobbe · 2024-02-08T05:33:51Z

Thanks for the review! Revision pushed, this time with content-parsing tests; PTAL. Widget tests still TODO.

gnprice

Thanks! The tests generally look good too; a few comments below.

gnprice · 2024-02-08T21:46:49Z

lib/model/content.dart

+  List<DiagnosticsNode> debugDescribeChildren() {
+    return [
+      _SpoilerHeaderDiagnosticableNode(header).toDiagnosticsNode(),
+      _SpoilerContentDiagnosticableNode(content).toDiagnosticsNode(),


This bit didn't feel satisfying — it feels like we should be able to express directly here a label "header" for the header and "content" for the content, rather than just list them positionally — so I looked at the debugDescribeChildren doc to see what alternatives I could find.

That led me to [RenderTable.debugDescribeChildren] , which pointed me at the name parameter to toDiagnosticsNode. I just tried that out and it works nicely.

I'll push a commit to the tip of this PR branch that makes that change. Probably the ideal branch structure is

that commit first, applying to ListNode;

then this SpoilerNode commit, with the rest of my new commit squashed into it.

That commit is:
cd0ed9c content [nfc]: Use toDiagnosticsNode(name: ...)

Awesome; glad to have this more satisfying way. Thanks!

gnprice · 2024-02-08T22:10:50Z

test/model/content_test.dart

+        header: [ParagraphNode(
+          links: [LinkNode(url: 'https://chat.zulip.org/', nodes: [TextNode('czo')])],
+          nodes: [


Suggested change

header: [ParagraphNode(

links: [LinkNode(url: 'https://chat.zulip.org/', nodes: [TextNode('czo')])],

nodes: [

header: [

ParagraphNode(links: null, nodes: [

This convenience is in a comment on equalsNode, which is the engine of these parsing tests. It should probably be in model/content_test.dart itself so as to be more visible:

// In [expected], for the `links` field of [ParagraphNode] or // any other [BlockInlineContainerNode] subclass, use `null`. // This field will be ignored in [expected], and instead the // field's value in [actual] will be checked for accuracy against // the [BlockInlineContainerNode.nodes] field on the same node. void equalsNode(ContentNode expected) {

gnprice · 2024-02-08T22:17:53Z

test/model/content_test.dart

+    testParse('with rich header and content',
+      // ```spoiler **bold** [czo](https://chat.zulip.org/)\n*italic* [zulip](https://zulip.com/)\n```


Did you find a way to provoke the header into being a list of more than one BlockContentNode? That'd be fun to also test, if so.

If not, then a test case where the header is like 1. * ## details, showing that it's not necessarily a paragraph, would be good. (I guess more generally: it'd be good to demonstrate not-a-paragraph, either as the only BlockContentNode or not.)

Did you find a way to provoke the header into being a list of more than one BlockContentNode? That'd be fun to also test, if so.

No, try as I might, sadly I did not manage to do this. 😛

No wait! I did! Here: https://chat.zulip.org/#narrow/stream/7-test-here/topic/content/near/1735802

Here's the HTML (formatted for easy reading, with an online HTML-formatting tool):

<div class="spoiler-block"> <div class="spoiler-header"> <p><a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3">image</a></p> <div class="message_inline_image"><a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3" title="image"><img src="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3"></a></div> </div> <div class="spoiler-content" aria-hidden="true"> <p>hello world</p> </div> </div>

The div.spoiler-header has a p child and a div child.

But that includes an image, and I don't think we can build our content widget for image previews, MessageImage, in a testContentSmoke call. That's because MessageImage needs PerAccountStoreWidget.of and InheritedMessage.of, and testContentSmoke doesn't give support for those. When we think about those dependencies, we should have this issue in mind:

content: Support Zulip content outside messages (even outside per-account contexts) #488

Cool.

Yeah, it's fine that testContentSmoke won't handle that; it's just a shortcut to enable a lot of those tests to be one line each. Instead you can write a test that's similar to what you'd get by inlining testContentSmoke and prepareContentBare, but that provides a few more enclosing widgets in the pumpWidget call.

(And if we find ourselves with a lot of tests like that doing similar things, we can abstract those into further helpers similar to prepareContentBare and/or testContentSmoke, or perhaps have the latter take an optional flag.)

gnprice · 2024-02-09T02:47:25Z

I recommend rebasing atop #511, which I just sent, and converting the content-parsing tests into the new ContentExample form from that PR — that should help simplify writing the widget tests. In particular it means you can get widget smoke tests very compactly, by reusing the examples from content parsing.

You'll also want some interaction tests, though (for showing and hiding on tap), and those might not benefit from ContentExample; see the comment in widgets/content_test.dart added at the tip of #511. But when it comes to those, the examples can be kept very simple, with the header and content made of just plain text. I think all the wild and wooly other stuff that can go in the headers doesn't affect the show/hide behavior.

(Hmm I guess if there's a link in the header, it'd be nice to check that tapping the link works and isn't pre-empted by the header's gesture detector. But that can still be pretty concise, along the lines of the HTML examples in the "LinkNode interactions" tests.)

chrisbobbe · 2024-02-24T03:20:29Z

OK, revision pushed!

The feature is still imperfect:

I haven't tested the screen-reader interface and I suspect it will need special care with Semantics
We need to think more about the direction in which the spoiler opens, and how that interacts with the message-list sliver's GrowthDirection.

but we can leave that for later work. I can file issues, unless one or both of these falls under an umbrella issue that you have a vision for.

gnprice

Thanks! All looks good, with just small comments below.

gnprice · 2024-02-26T20:53:24Z

lib/model/content.dart

@@ -226,33 +241,35 @@ class ListNode extends BlockContentNode {
  @override
  List<DiagnosticsNode> debugDescribeChildren() {
    return items
-      .map((nodes) => _ListItemDiagnosticableNode(nodes).toDiagnosticsNode())
+      .mapIndexed((i, nodes) =>
+        _BlockContentListNode(nodes).toDiagnosticsNode(name: 'item $i'))


content [nfc]: Use toDiagnosticsNode(name: ...)

This commit should probably still have me as the author :-)

(you can use git commit --amend --author=… to adjust)

Ah indeed; thanks for catching this!

gnprice · 2024-02-26T21:06:55Z

test/widgets/content_test.dart

+      testWidgets('tap image', (tester) async {
+        final pushedRoutes = await prepareContent(tester, example.html);
+
+        await tester.tapAt(tester.getCenter(find.byType(RealmContentNetworkImage)));


nit: can simplify to:

Suggested change

await tester.tapAt(tester.getCenter(find.byType(RealmContentNetworkImage)));

await tester.tap(find.byType(RealmContentNetworkImage));

gnprice · 2024-02-26T21:08:57Z

test/widgets/page_checks.dart

@@ -9,3 +9,7 @@ extension WidgetRouteChecks on Subject<WidgetRoute> {
 extension AccountPageRouteMixinChecks on Subject<AccountPageRouteMixin> {
  Subject<int> get accountId => has((x) => x.accountId, 'accountId');
 }
+
+extension PageRouteChecks on Subject<PageRoute> {


nit: let's keep the checks extensions for Flutter upstream classes gathered in flutter_checks.dart

gnprice · 2024-02-26T21:14:54Z

test/model/content_test.dart

+      header: [ListNode(ListStyle.ordered, [
+          [ListNode(ListStyle.unordered, [
+            [HeadingNode(level: HeadingLevel.h2, links: null, nodes: [
+              TextNode('hello'),
+            ])]
+          ])],
+        ])],


nit: two-space indents, here and below

Suggested change

header: [ListNode(ListStyle.ordered, [

[ListNode(ListStyle.unordered, [

[HeadingNode(level: HeadingLevel.h2, links: null, nodes: [

TextNode('hello'),

])]

])],

])],

header: [ListNode(ListStyle.ordered, [

[ListNode(ListStyle.unordered, [

[HeadingNode(level: HeadingLevel.h2, links: null, nodes: [

TextNode('hello'),

])]

])],

])],

gnprice · 2024-02-26T21:18:50Z

test/model/content_test.dart

+    '```spoiler [image](https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3)\nhello world\n```',
+    '<div class="spoiler-block"><div class="spoiler-header">\n'
+      '<p><a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3">image</a></p>\n'
+      '<div class="message_inline_image"><a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3" title="image"><img src="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3"></a></div></div><div class="spoiler-content" aria-hidden="true">\n'


nit: this line finishes an element it didn't begin (the div.spoiler-header), so should have a newline before starting the next:

Suggested change

'<div class="message_inline_image"><a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3" title="image"><img src="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3"></a></div></div><div class="spoiler-content" aria-hidden="true">\n'

'<div class="message_inline_image"><a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3" title="image"><img src="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3"></a></div></div>'

'<div class="spoiler-content" aria-hidden="true">\n'

(Ideally the indentation would help show the nesting of elements, but this part is easy and an improvement regardless)

Yeah, I wasn't sure how to compromise between being helpful about element nesting and not stretching out the code too much over a lot of lines. Like if it were OK to take a lot of lines, we could do

'<div class="spoiler-block">' '<div class="spoiler-header">\n' '<p>' '<a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3">image</a>' '</p>\n' '<div class="message_inline_image">' '<a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3" title="image">' '<img src="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3">' '</a>' '</div>' '</div>' '<div class="spoiler-content" aria-hidden="true">\n' '<p>hello world</p>\n' '</div>' '</div>\n',

or something like that, right?

Yeah. And like you suggested, going all the way there would be a lot more lines and probably not worth the trade-off in making it hard to see several of these on the screen at once.

gnprice · 2024-02-26T22:05:56Z

Oh, and forgot that I'd meant to reply to these:

The feature is still imperfect:

I haven't tested the screen-reader interface and I suspect it will need special care with Semantics

We need to think more about the direction in which the spoiler opens, and how that interacts with the message-list sliver's GrowthDirection.

but we can leave that for later work. I can file issues, unless one or both of these falls under an umbrella issue that you have a vision for.

Yeah, please go ahead and file an issue for the second one. The first one is probably best handled as part of a future sweep through our semantics and screen-reader behavior; I'll go file an issue for that and include this as an item to remember to check when doing that.

chrisbobbe · 2024-02-26T22:47:30Z

Sure: #536

chrisbobbe · 2024-02-26T22:50:33Z

Thanks for the review! Revision pushed.

gnprice · 2024-02-26T22:57:00Z

Thanks! Looks good; merging.

This produces slightly nicer output, by indicating the index for each item. For example, after introducing an error in one test, the expected and actual values it prints look like this: Actual: <ZulipContent └─ListNode │ unordered │ ├─item 0: BlockContentNode list │ └─ParagraphNode │ │ was implicit │ │ │ └─TextNode │ "something" │ └─item 1: BlockContentNode list └─ParagraphNode │ was implicit │ └─TextNode "another"> instead of like this: Actual: <ZulipContent └─ListNode │ unordered │ ├─list item │ └─ParagraphNode │ │ was implicit │ │ │ └─TextNode │ "something" │ └─list item └─ParagraphNode │ was implicit │ └─TextNode "another"> This also means less code as we start adding more nodes that have this kind of structure; it avoids having to make a new subclass of DiagnosticableTree for each one.

When we support spoilers, soon, we'll want to translate the default header text ("Spoiler") into the user's language. It seems safe to assume that Zulip content will always have `ZulipLocalizations.of` support wherever it appears in the widget tree. The language will be a client setting with a default value. There are certain other assumptions that `prepareContentBare` will probably want to avoid making, though, including: - that the content is rendered in a particular message - that the content is rendered in a message at all (zulip#488) - that the content is rendered in a per-account context (zulip#488) But we'll think more about that later, when we start using testContentSmoke in more places.

Fixes: zulip#358

chrisbobbe added the a-content Parsing and rendering Zulip HTML content, notably message contents label Feb 2, 2024

chrisbobbe requested a review from gnprice February 2, 2024 01:51

gnprice mentioned this pull request Feb 5, 2024

Cut inefficient CssClassSet out of content parsing #505

Merged

gnprice marked this pull request as draft February 5, 2024 23:51

chrisbobbe force-pushed the wip-spoilers branch from c296615 to f1f111a Compare February 8, 2024 05:33

gnprice reviewed Feb 8, 2024

View reviewed changes

chrisbobbe force-pushed the wip-spoilers branch 2 times, most recently from 8bad174 to 62fffc5 Compare February 24, 2024 03:17

chrisbobbe marked this pull request as ready for review February 24, 2024 03:17

chrisbobbe changed the title ~~wip content: Support spoilers!~~ content: Support spoilers! Feb 24, 2024

gnprice reviewed Feb 26, 2024

View reviewed changes

chrisbobbe mentioned this pull request Feb 26, 2024

msglist: Spoilers should always grow downward when they open #536

Open

chrisbobbe force-pushed the wip-spoilers branch from 62fffc5 to 17c8ccc Compare February 26, 2024 22:50

gnprice and others added 3 commits February 26, 2024 14:57

content: Handle spoilers

0be474a

Fixes: zulip#358

gnprice force-pushed the wip-spoilers branch from 17c8ccc to 0be474a Compare February 26, 2024 22:57

gnprice merged commit 0be474a into zulip:main Feb 26, 2024
1 check passed

chrisbobbe deleted the wip-spoilers branch February 26, 2024 23:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

content: Support spoilers! #503

content: Support spoilers! #503

chrisbobbe commented Feb 2, 2024 •

edited

Loading

gnprice commented Feb 5, 2024

chrisbobbe commented Feb 8, 2024

gnprice left a comment

gnprice Feb 8, 2024

gnprice Feb 8, 2024

chrisbobbe Feb 12, 2024

gnprice Feb 8, 2024

gnprice Feb 8, 2024

chrisbobbe Feb 12, 2024

chrisbobbe Feb 12, 2024 •

edited

Loading

chrisbobbe Feb 12, 2024

gnprice Feb 12, 2024

gnprice commented Feb 9, 2024

chrisbobbe commented Feb 24, 2024 •

edited

Loading

gnprice left a comment

gnprice Feb 26, 2024

chrisbobbe Feb 26, 2024

gnprice Feb 26, 2024

gnprice Feb 26, 2024

gnprice Feb 26, 2024

gnprice Feb 26, 2024

chrisbobbe Feb 26, 2024 •

edited

Loading

gnprice Feb 26, 2024

gnprice commented Feb 26, 2024

chrisbobbe commented Feb 26, 2024

chrisbobbe commented Feb 26, 2024

gnprice commented Feb 26, 2024

		testParse('with rich header and content',
		// ```spoiler bold [czo](https://chat.zulip.org/)\nitalic [zulip](https://zulip.com/)\n```

	await tester.tapAt(tester.getCenter(find.byType(RealmContentNetworkImage)));
	await tester.tap(find.byType(RealmContentNetworkImage));

	'<div class="message_inline_image"><a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3" title="image"><img src="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3"></a></div></div><div class="spoiler-content" aria-hidden="true">\n'
	'<div class="message_inline_image"><a href="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3" title="image"><img src="https://chat.zulip.org/user_avatars/2/realm/icon.png?version=3"></a></div></div>'
	'<div class="spoiler-content" aria-hidden="true">\n'

content: Support spoilers! #503

content: Support spoilers! #503

Conversation

chrisbobbe commented Feb 2, 2024 • edited Loading

gnprice commented Feb 5, 2024

chrisbobbe commented Feb 8, 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

chrisbobbe Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Feb 9, 2024

chrisbobbe commented Feb 24, 2024 • edited Loading

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

chrisbobbe Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Feb 26, 2024

chrisbobbe commented Feb 26, 2024

chrisbobbe commented Feb 26, 2024

gnprice commented Feb 26, 2024

chrisbobbe commented Feb 2, 2024 •

edited

Loading

chrisbobbe Feb 12, 2024 •

edited

Loading

chrisbobbe commented Feb 24, 2024 •

edited

Loading

chrisbobbe Feb 26, 2024 •

edited

Loading