Skip to content

content: Handle link previews #1049

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 26, 2025

Conversation

rajveermalviya
Copy link
Member

@rajveermalviya rajveermalviya commented Nov 6, 2024

Fixes: #1016

Screenshots
Web Flutter
Screenshot 2025-02-24 at 12 27 55 Screenshot 2025-02-24 at 12 26 56
Screenshot 2025-02-24 at 12 28 55 Screenshot 2025-02-24 at 12 28 33
Web (Small) Flutter (Small)
Screenshot 2025-02-24 at 12 37 54 Screenshot 2025-02-24 at 12 38 23
Screenshot 2025-02-24 at 12 38 13 Screenshot 2025-02-24 at 12 37 19

@rajveermalviya rajveermalviya force-pushed the pr-content-link-previews branch from abdec10 to 8fba3fe Compare November 18, 2024 21:57
@rajveermalviya rajveermalviya marked this pull request as ready for review November 18, 2024 23:14
@rajveermalviya rajveermalviya added the maintainer review PR ready for review by Zulip maintainers label Nov 18, 2024
@chrisbobbe chrisbobbe self-requested a review December 10, 2024 20:33
@chrisbobbe chrisbobbe self-assigned this Dec 10, 2024
Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks like this needs a rebase, so I'll do a more thorough review after that. But here are some comments from a quick skim (in particular I haven't read the parsing code or checked the UI code against web).

padding: const EdgeInsets.symmetric(horizontal: 5),
child: InsetShadowBox(
bottom: 8,
color: messageListTheme.streamMessageBgDefault,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it'll be wrong for DMs, and (in future) for messages where we highlight the background because of @-mentions in the message (#647).

child: Text(node.title!,
style: TextStyle(
fontSize: 1.2 * kBaseFontSize,
color: const HSLColor.fromAHSL(1, 200, 1, 0.4).toColor()))),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a hard-coded color; does it follow web? It needs either a variable in ContentTheme or this comment:

// Web has the same color in light and dark mode.

(Same for any other hard-coded colors.)

Please also post screenshots in light mode; I see screenshots for dark mode already.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the screenshots.

if (isSmallWidth) {
return Container(
decoration: const BoxDecoration(border:
Border(left: BorderSide(color: Color(0xFFEDEDED), width: 3))),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about hard-coded colors; also, I think we more often use lowercase (so 0xffededed instead of 0xFFEDEDED); here and below.

final messageListTheme = MessageListTheme.of(context);
final isSmallWidth = MediaQuery.sizeOf(context).width <= 576;

final dataContainer = Container(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is dataContainer the best name? I see the Container widget being used…what's the "data" and how does that widget "contain" it?

How about building this method's return value with help from a mutable Widget result variable? So here:

Widget result = Container(/* etc. */);

then below,

result = isSmallWidth
  ? Column(/* etc. */, children: [/* etc. */, result])
  : Row(/* etc. */, children: [/* etc. */, result]);

then result = Container(decoration: /* etc. */, child: result);

and finally return result;.

@rajveermalviya rajveermalviya force-pushed the pr-content-link-previews branch 3 times, most recently from 533fa33 to aeabbe6 Compare December 12, 2024 17:45
@rajveermalviya
Copy link
Member Author

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

@chrisbobbe
Copy link
Collaborator

What's a good way for me to test this; do I need to set up a dev server? 🙂 I see link previews are disabled on CZO; I've asked on CZO if there's a reason for that: https://chat.zulip.org/#narrow/channel/9-issues/topic/Link.20previews.20for.20Zulip.20URLs/near/2013846

@gnprice
Copy link
Member

gnprice commented Dec 24, 2024

Dev server, or make a test realm on Zulip Cloud.

Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Comments below.

GestureDetector(
onTap: () => _launchUrl(context, node.hrefUrl),
child: RealmContentNetworkImage(
Uri.parse(node.imageSrcUrl),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about just one codepath for URL-parsing this string, instead of splitting by isSmallWidth?

Also, this will throw an error if parsing fails. Instead of doing that, let's use Uri.tryParse instead, similar to what we do in MessageImage.

if (second.nodes.length > 2) return null;

String? title, description;
for (final node in second.nodes) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is both a title and a description, can they appear in either order? If not—if it's always the title first—how about requiring that? For the code structure, instead of a loop, maybe we could do a switch on the length of second.nodes, and in the 1 case we expect either a title or a description, and in the 2 case we expect a title first and then a description.

Comment on lines 1512 to 1516
final first = divElement.nodes.first;
if (first is! dom.Element) return null;
if (first.localName != 'a') return null;
if (first.className != 'message_embed_image') return null;
if (first.nodes.isNotEmpty) return null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can be a bit more compact in some places by using Dart Patterns; try a regex search for if.*case in this file.

@override
Widget build(BuildContext context) {
final messageListTheme = MessageListTheme.of(context);
final isSmallWidth = MediaQuery.sizeOf(context).width <= 576;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

576 is from the web app, right? And some other explicit width and height values below: 500, 80, 115, etc.

We could comment on each one, saying they come from the web app. But actually, I could imagine future design work where we want to tune these numbers to be different from the web app. In that case such comments would become wrong/misleading if we forgot to update them. So maybe best not.

To memoize the fact that they match web, though (so a reader doesn't have to check each one), let's mention it in the commit message.


return Container(
decoration: const BoxDecoration(
border: Border(left: BorderSide(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BorderDirectional(start:, right?

Comment on lines 879 to 881
// TODO(#647) use different color for highlighted messages
// TODO(#681) use different color for DM messages
color: messageListTheme.streamMessageBgDefault,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right; yeah, I forgot we haven't done #681 yet.

I guess this needs one more TODO I hadn't thought of before:

// TODO(#488) use different color for non-message contexts

Probably the desired effect of that TODO will be to guide the implementation toward a color param rather than a param that's about the aspects of a Zulip message.

? titleAndDescription
: LayoutBuilder(
builder: (context, constraints) => ConstrainedBox(
constraints: BoxConstraints(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a lot about constraints in this code: a Container.constraints, an UnconstrainedBox, a LayoutBuilder, a ConstrainedBox.constraints. I'm not really following it yet; do you think there might be a simpler way to write it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this LayoutBuilder here.

fit: BoxFit.cover,
width: 80,
height: 80,
alignment: Alignment.center)),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't Alignment.center the default; can we leave out this argument?

: LayoutBuilder(
builder: (context, constraints) => ConstrainedBox(
constraints: BoxConstraints(
maxWidth: constraints.maxWidth - 115),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder about dropping this maxWidth - 115 detail, for a few benefits:

  • More of the text can show before it gets clipped
  • Removes the need for LayoutBuilder which isn't great for
    • performance
    • code complexity (e.g. my difficulty in a previous comment)
  • We allow more horizontal space for other paragraph content without issues

Could leave a code comment saying we're not following web in this way.

border: Border(left: BorderSide(
// Web has the same color in light and dark mode.
color: Color(0xffededed), width: 3))),
padding: const EdgeInsets.all(5),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Web also puts 5px bottom margin, a.k.a. --markdown-interelement-space-px, in addition to this. In zulip-flutter do we have something systematic for vertical spacing between block elements, or is each element responsible for adding its own space at the bottom and/or top?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a quick test, looks like the spacing between different widgets is not consistent (or non-existent in some cases). On Web --markdown-interelement-space-px is calculated from line-height (which for me becomes 6.72px instead of 5px), so we'd need something similar for zulip-flutter too. However, it'll need a sweep across all the content widgets therefore making it out-of-scope for this PR.

Should 5px bottom margin as a quick fix, suffice for now?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should 5px bottom margin as a quick fix, suffice for now?

Sure, sounds good

@rajveermalviya rajveermalviya force-pushed the pr-content-link-previews branch 3 times, most recently from 6e37971 to 70ea927 Compare January 21, 2025 19:22
@rajveermalviya
Copy link
Member Author

Thanks for the review @chrisbobbe! Pushed an update, PTAL.

Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks! Just nits below, and I think #1049 (comment) is still open. Otherwise LGTM; marking for Greg's review.

@@ -148,6 +151,7 @@ class ContentTheme extends ThemeExtension<ContentTheme> {
final Color colorTableCellBorder;
final Color colorTableHeaderBackground;
final Color colorThematicBreak;
final Color colorLink;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: keep in alphabetical order (here and elsewhere in class definition)

@@ -1030,7 +1037,7 @@ class _InlineContentBuilder {
_pushRecognizer(recognizer);
final result = _buildNodes(node.nodes,
// Web has the same color in light and dark mode.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this comment; it would be easy for it to become wrong if the light/dark variants start to be different. Also you've already left a comment with the same meaning on the dark variant.

@chrisbobbe chrisbobbe added the integration review Added by maintainers when PR may be ready for integration label Jan 31, 2025
@chrisbobbe chrisbobbe assigned gnprice and unassigned chrisbobbe Jan 31, 2025
@chrisbobbe chrisbobbe requested a review from gnprice January 31, 2025 01:21
@chrisbobbe chrisbobbe removed the maintainer review PR ready for review by Zulip maintainers label Jan 31, 2025
@rajveermalviya rajveermalviya force-pushed the pr-content-link-previews branch 2 times, most recently from 49cf612 to aa4c9ea Compare February 10, 2025 16:56
Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Here's an initial round, just on model/content.dart. Posting this now because about to switch to another task.

// Ref:
// https://ogp.me/
// https://oembed.com/
class LinkPreviewNode extends BlockContentNode {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: let's put this above TableNode and friends; it feels more analogous to embedding images and video

Comment on lines 584 to 586
// Ref:
// https://ogp.me/
// https://oembed.com/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also link to whatever bits of Zulip docs there are for this.

The most on-point item I find in the Help Center is this:
https://zulip.com/help/image-video-and-website-previews#configure-whether-website-previews-are-shown
which at least provides the keyword for what we call it in user-facing text: "website previews". (Which in turn is potentially useful for searching chat.zulip.org for discussion of the feature, or for asking about it.)

Comment on lines 1162 to 1163
'<p><a href="https://pub-14f7b5e1308d42b69c4a46608442a50c.r2.dev/image+title+description.html">https://pub-14f7b5e1308d42b69c4a46608442a50c.r2.dev/image+title+description.html</a></p>\n'
'<div class="message_embed">'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: indentation

Suggested change
'<p><a href="https://pub-14f7b5e1308d42b69c4a46608442a50c.r2.dev/image+title+description.html">https://pub-14f7b5e1308d42b69c4a46608442a50c.r2.dev/image+title+description.html</a></p>\n'
'<div class="message_embed">'
'<p><a href="https://pub-14f7b5e1308d42b69c4a46608442a50c.r2.dev/image+title+description.html">https://pub-14f7b5e1308d42b69c4a46608442a50c.r2.dev/image+title+description.html</a></p>\n'
'<div class="message_embed">'

The div is a sibling of the p, not a child, so the indentation should express that.

Comment on lines 1574 to 1584
case [dom.Element(localName: 'div') && final single]:
switch (single.className) {
case 'message_embed_title':
title = parseTitle(single);
if (title == null) return null;
case 'message_embed_description':
description = parseDescription(single);
if (description == null) return null;
}

default:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this would accept the children being a single div with some other unexpected class. I think that's unintended.

return null;
}

return LinkPreviewNode(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this parser code has (modulo my comment just above) an invariant that it always has at least one of title or description non-null when it decides to construct a LinkPreviewNode.

Is that invariant something you believe holds? It seems like the sort of thing that'd be helpful for reasoning about (the avoidance of) edge cases when displaying these as widgets.

If so, we can add an assert at the constructor to confirm that it holds. That'd make it something the widgets code can count on.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, turns out it is possible for both title and description to be absent. For example, the description is already considered optional but if the website's HTML has neither the <title>…</title> nor <meta property="og:title" … /> then the title will also be missing, resulting in server to generate an empty data-container:

<div class="message_embed">
  <a class="message_embed_image"  ></a>
  <div class="data-container"></div>
</div>

(If the <meta property="og:image" … /> is also missing then server doesn't generate "website preview" message for that link)

I'll update the implementation to support the empty data-container.

Comment on lines 1594 to 1660
} else {
return null;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sure am hoping for Dart to at some point gain an analogue of Rust's let-else, or Swift's guard-let 🙂

@rajveermalviya rajveermalviya force-pushed the pr-content-link-previews branch from aa4c9ea to 193280a Compare February 20, 2025 19:52
@rajveermalviya
Copy link
Member Author

Thanks for the review @gnprice! Pushed an update, PTAL.

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! The revisions look good; just two nits below.

Next I'll look at the remaining parts of the PR.

Comment on lines 1448 to 1453
case []:
// Server generates an empty `<div class="data-container"></div>`
// if website HTML doesn't have both title (derived from
// `og:title` or `<title>…</title>`) and description (derived from
// `og:description`).
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
case []:
// Server generates an empty `<div class="data-container"></div>`
// if website HTML doesn't have both title (derived from
// `og:title` or `<title>…</title>`) and description (derived from
// `og:description`).
break;
case []:
// Server generates an empty `<div class="data-container"></div>`
// if website HTML has neither title (derived from
// `og:title` or `<title>…</title>`) nor description (derived from
// `og:description`).
break;

I'd read "if doesn't have both A and B" as meaning "if not (has A and has B)". But I think that's not what you meant — if that were the case, the single-child case below shouldn't be possible.


String? title, description;
switch (dataContainer.nodes) {
case []:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: either put this case after the other two, or swap the order of the other two — that way the cases of 0, 1, and 2 children go in consecutive order (in one direction or the other)

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, and finished reading the rest of the PR — comments below.

Comment on lines 933 to 935
// But for now we use a static value instead, see discussion:
// https://github.com/zulip/zulip-flutter/pull/1049#discussion_r1915747908
padding: const EdgeInsets.only(bottom: 5),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reasoning makes sense but let's use a value that corresponds to web's new defaults, rather than "dense mode" / web's old defaults.

That way it's most consistent with the rest of our design — our font size and line height are based on web's new defaults.

@@ -1339,6 +1391,112 @@ class _ZulipContentParser {
return EmbedVideoNode(hrefUrl: href, previewImageSrcUrl: imgSrc, debugHtmlNode: debugHtmlNode);
}

static final _websitePreviewImageSrcRegexp = RegExp(r'background-image: url\("(.+)"\)');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I tried opening an ancient example message on CZO, intending to try out how the UI looks… and found that it didn't parse. 🙂.

I think the issue may only be that the argument of url here is lacking quotes. If it's easy to make that old message work by handling that, then it'd be good to do so in this PR while we're thinking about it — we'll want to cover old messages like this eventually anyway.

(Probably cleanest to do so as a separate commit on top, letting the main commit stay focused on the modern case.)

// TODO(#647) use different color for highlighted messages
// TODO(#681) use different color for DM messages
color: MessageListTheme.of(context).streamMessageBgDefault,
child: UnconstrainedBox(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be OverflowBox instead?

This widget's doc suggests that it's not meant for cases where the child will overflow:

/// In debug mode, if the child overflows the container, a warning will be
/// printed on the console, and black and yellow striped areas will appear where
/// the overflow occurs.

whereas that other widget is.

(I haven't read enough to fully understand what the difference is, though.)

child: Text(node.title!,
style: TextStyle(
fontSize: 1.2 * kBaseFontSize,
height: kTextHeightNone,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh curious. Why this "none" value instead of a specific line-height?

This is one bit that looks different between web and this PR in the screenshots — the titles have a bigger line-height on web.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Web uses line-height: normal for this. MDN docs for it says that it can be roughly 1.2. Which seems correct, atleast on Firefox desktop and Firefox mobile for Android:

firefox-fonts-tool.mp4

So, updated to use that value.

Comment on lines 921 to 925
child: RealmContentNetworkImage(
resolvedImageSrcUrl,
fit: BoxFit.cover,
width: 80,
height: 80)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell (I just spent a few minutes digging through the implementation of Image to investigate this), the width and height parameters here don't do anything beyond what wrapping this in a Size widget would do.

It looks like all our other RealmContentNetworkImage widgets take the latter approach — skipping these parameters, and relying on their parents for size — and that seems to work fine.

So let's do that. Then the rest of this widget's details, and its GestureDetector parent, can be deduplicated with the small-width case above.

Widget build(BuildContext context) {
final store = PerAccountStoreWidget.of(context);
final resolvedImageSrcUrl = store.tryResolveUrl(node.imageSrcUrl);
final isSmallWidth = MediaQuery.sizeOf(context).width <= 576;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love using MediaQuery.sizeOf here — it feels like it's inevitably asking the wrong question. (Instead of the width of the entire app's viewport, it'd be much more to the point to specify this design in terms of this widget's own width, which will have been dictated by its parent.)

But this is fine, because I think implementing this design, with the way it flips between wide and tall forms, in that cleaner way would require either (a) LayoutBuilder, which is significantly less clean in other ways, or (b) significantly more work. And the exact design of this feature isn't something I'd want us to spend a lot of time on.

description: null),
]);

static const websitePreviewWithoutTitleAndDescription = ContentExample(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static const websitePreviewWithoutTitleAndDescription = ContentExample(
static const websitePreviewWithoutTitleOrDescription = ContentExample(

await prepare(tester, ContentExample.websitePreviewSmoke.html);
tester.widget(find.byType(WebsitePreview));

await tester.tap(find.text('Zulip — organized team chat'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more quick check we can add to the two of these that have a description: check find.text finds something with the description's text.

(Otherwise e.g. this test case and the websitePreviewWithoutDescription one below are making exactly the same set of checks, I think, which seems incomplete since there are expected differences in behavior.)

Comment on lines 1058 to 1066
tester.widget(find.byType(WebsitePreview));

await tester.tap(find.byType(RealmContentNetworkImage));
check(testBinding.takeLaunchUrlCalls())
.single.equals((url: url, mode: LaunchMode.platformDefault));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The find.byType(WebsitePreview) check can be left out of these, I think — we're checking that there's an image which if you tap it opens the example's URL, and that seems like good confirmation in itself that we didn't just not manage to render the UI for this feature.

The advantage of leaving it out is that it's one fewer thing that has to be changed if we make a refactor to this code (for example even just renaming the widget class), one that isn't meant to affect the UI the user sees.

@rajveermalviya rajveermalviya force-pushed the pr-content-link-previews branch 4 times, most recently from 52af930 to b5480d4 Compare February 24, 2025 08:08
@rajveermalviya
Copy link
Member Author

Thanks for the review @gnprice! Pushed an update, PTAL.

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Generally this all looks good — a few small comments.

@@ -1339,6 +1391,112 @@ class _ZulipContentParser {
return EmbedVideoNode(hrefUrl: href, previewImageSrcUrl: imgSrc, debugHtmlNode: debugHtmlNode);
}

static final _websitePreviewImageSrcRegexp = RegExp(r'background-image: url\("?(.+?)"?\)');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bothers me a bit because it would match if for some reason there was a " on one side but not the other.

We can avoid that with a backreference in the regexp:

Suggested change
static final _websitePreviewImageSrcRegexp = RegExp(r'background-image: url\("?(.+?)"?\)');
static final _websitePreviewImageSrcRegexp = RegExp(r'background-image: url\(("?)(.+?)\1\)');

(and adjusting .group(1) below to .group(2))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(No need for a test for that case, though — it seems awfully unlikely in practice.)

return null;
}

return WebsitePreviewNode(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to pass debugHtmlNode — I noticed that while doing some debugging to follow up on #1049 (comment) 🙂

(The issue turned out to be that I'd done only half of the edits I suggested in the backreference comment above. Should make sure we support debugging, though.)

])
: Row(crossAxisAlignment: CrossAxisAlignment.start, children: [
if (image != null)
AspectRatio(aspectRatio: 1, child: image),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about an 80x80 SizedBox instead? That's simpler to think about, and I think it ends up doing the same thing here.

(If it didn't do the same thing, that would probably be an issue with the AspectRatio version — I think we want the fixed size.)

@rajveermalviya rajveermalviya force-pushed the pr-content-link-previews branch 2 times, most recently from c2c5829 to 30b70ea Compare February 26, 2025 02:46
@rajveermalviya
Copy link
Member Author

Thanks for the review @gnprice! Pushed an update, PTAL.

Implements support for displaying website previews messages,
follows the Web styling, like having different layout for
larger viewports (> 576), and any other constraints that are
empirically present on Web.

Fixes: zulip#1016
In legacy website preview messages, the image URL in
`message_embed_image` element's `style` is formatted as:

  background-image: url(https://example.com/image.png)

In the latest server revision, it is formatted as:

  background-image: url("https://example.com/image.png")

So, fix the regexp to match the URL whether or not it's
enclosed in quotes.
@gnprice gnprice force-pushed the pr-content-link-previews branch from 30b70ea to e0df0ed Compare February 26, 2025 05:02
@gnprice
Copy link
Member

gnprice commented Feb 26, 2025

Thanks! Looks good; merging.

@gnprice gnprice merged commit e0df0ed into zulip:main Feb 26, 2025
1 check passed
@rajveermalviya rajveermalviya deleted the pr-content-link-previews branch February 26, 2025 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle message_embed website previews
3 participants