From 957a733838ac8254e6197b00755992fddde8886d Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 25 Jan 2019 11:33:09 -0500 Subject: [PATCH] Yet more link preview refinements. --- .../Cells/OWSQuotedMessageView.m | 4 ++- .../ConversationInputToolbar.m | 15 +++++++++ Signal/src/views/LinkPreviewView.swift | 2 +- Signal/src/views/QuotedReplyPreview.swift | 4 ++- .../Interactions/OWSLinkPreview.swift | 33 +++++++++++-------- .../tests/Messages/OWSLinkPreviewTest.swift | 28 ++++++++-------- 6 files changed, 57 insertions(+), 29 deletions(-) diff --git a/Signal/src/ViewControllers/ConversationView/Cells/OWSQuotedMessageView.m b/Signal/src/ViewControllers/ConversationView/Cells/OWSQuotedMessageView.m index ea1cb80a1..fe9798308 100644 --- a/Signal/src/ViewControllers/ConversationView/Cells/OWSQuotedMessageView.m +++ b/Signal/src/ViewControllers/ConversationView/Cells/OWSQuotedMessageView.m @@ -1,5 +1,5 @@ // -// Copyright (c) 2018 Open Whisper Systems. All rights reserved. +// Copyright (c) 2019 Open Whisper Systems. All rights reserved. // #import "OWSQuotedMessageView.h" @@ -195,6 +195,8 @@ const CGFloat kRemotelySourcedContentRowSpacing = 3; [innerBubbleView autoPinTrailingToSuperviewMarginWithInset:self.bubbleHMargin]; [innerBubbleView autoPinTopToSuperviewMargin]; [innerBubbleView autoPinBottomToSuperviewMargin]; + [innerBubbleView setContentHuggingHorizontalLow]; + [innerBubbleView setCompressionResistanceHorizontalLow]; UIStackView *hStackView = [UIStackView new]; hStackView.axis = UILayoutConstraintAxisHorizontal; diff --git a/Signal/src/ViewControllers/ConversationView/ConversationInputToolbar.m b/Signal/src/ViewControllers/ConversationView/ConversationInputToolbar.m index cf684f8ec..6131298b1 100644 --- a/Signal/src/ViewControllers/ConversationView/ConversationInputToolbar.m +++ b/Signal/src/ViewControllers/ConversationView/ConversationInputToolbar.m @@ -170,13 +170,24 @@ const CGFloat kMaxTextViewHeight = 98; _quotedReplyWrapper = [UIView containerView]; self.quotedReplyWrapper.hidden = YES; + [self.quotedReplyWrapper setContentHuggingHorizontalLow]; + [self.quotedReplyWrapper setCompressionResistanceHorizontalLow]; _linkPreviewWrapper = [UIView containerView]; self.linkPreviewWrapper.hidden = YES; + [self.linkPreviewWrapper setContentHuggingHorizontalLow]; + [self.linkPreviewWrapper setCompressionResistanceHorizontalLow]; _vStack = [[UIStackView alloc] initWithArrangedSubviews:@[ self.quotedReplyWrapper, self.linkPreviewWrapper, self.inputTextView ]]; self.vStack.axis = UILayoutConstraintAxisVertical; + [self.vStack setContentHuggingHorizontalLow]; + [self.vStack setCompressionResistanceHorizontalLow]; + + for (UIView *button in @[ self.attachmentButton, self.voiceMemoButton, self.sendButton ]) { + [button setContentHuggingHorizontalHigh]; + [button setCompressionResistanceHorizontalHigh]; + } _hStack = [[UIStackView alloc] initWithArrangedSubviews:@[ self.attachmentButton, self.vStack, self.voiceMemoButton, self.sendButton ]]; @@ -189,6 +200,8 @@ const CGFloat kMaxTextViewHeight = 98; [self addSubview:self.hStack]; [self.hStack autoPinEdgeToSuperviewEdge:ALEdgeTop]; [self.hStack autoPinEdgeToSuperviewSafeArea:ALEdgeBottom]; + [self.hStack setContentHuggingHorizontalLow]; + [self.hStack setCompressionResistanceHorizontalLow]; // See comments on updateContentLayout:. if (@available(iOS 11, *)) { @@ -285,6 +298,8 @@ const CGFloat kMaxTextViewHeight = 98; QuotedReplyPreview *quotedMessagePreview = [[QuotedReplyPreview alloc] initWithQuotedReply:quotedReply conversationStyle:self.conversationStyle]; quotedMessagePreview.delegate = self; + [quotedMessagePreview setContentHuggingHorizontalLow]; + [quotedMessagePreview setCompressionResistanceHorizontalLow]; self.quotedReplyWrapper.hidden = NO; self.quotedReplyWrapper.layoutMargins = UIEdgeInsetsMake(self.quotedMessageTopMargin, 0, 0, 0); diff --git a/Signal/src/views/LinkPreviewView.swift b/Signal/src/views/LinkPreviewView.swift index 53cde1861..f5395683c 100644 --- a/Signal/src/views/LinkPreviewView.swift +++ b/Signal/src/views/LinkPreviewView.swift @@ -163,7 +163,7 @@ public class LinkPreviewSent: NSObject, LinkPreviewState { public func displayDomain() -> String? { guard let displayDomain = linkPreview.displayDomain() else { - owsFailDebug("Missing display domain") + Logger.error("Missing display domain") return nil } return displayDomain diff --git a/Signal/src/views/QuotedReplyPreview.swift b/Signal/src/views/QuotedReplyPreview.swift index 0c9d9f979..c806b7b2a 100644 --- a/Signal/src/views/QuotedReplyPreview.swift +++ b/Signal/src/views/QuotedReplyPreview.swift @@ -1,5 +1,5 @@ // -// Copyright (c) 2018 Open Whisper Systems. All rights reserved. +// Copyright (c) 2019 Open Whisper Systems. All rights reserved. // import Foundation @@ -46,6 +46,8 @@ class QuotedReplyPreview: UIView { // sizes changes). let quotedMessageView = OWSQuotedMessageView(forPreview: quotedReply, conversationStyle: conversationStyle) self.quotedMessageView = quotedMessageView + quotedMessageView.setContentHuggingHorizontalLow() + quotedMessageView.setCompressionResistanceHorizontalLow() quotedMessageView.backgroundColor = .clear diff --git a/SignalServiceKit/src/Messages/Interactions/OWSLinkPreview.swift b/SignalServiceKit/src/Messages/Interactions/OWSLinkPreview.swift index a964258a9..784ba87d0 100644 --- a/SignalServiceKit/src/Messages/Interactions/OWSLinkPreview.swift +++ b/SignalServiceKit/src/Messages/Interactions/OWSLinkPreview.swift @@ -289,20 +289,23 @@ public class OWSLinkPreview: MTLModel { // MARK: - Whitelists + // For link domains, we require an exact match - no subdomains allowed. + // + // Note that order matters in this whitelist since the logic for determining + // how to render link preview domains in displayDomain(...) uses the first match. + // We should list TLDs first and subdomains later. private static let linkDomainWhitelist = [ // YouTube "youtube.com", "www.youtube.com", "m.youtube.com", "youtu.be", - "youtube-nocookie.com", - "www.youtube-nocookie.com", // Reddit "reddit.com", "www.reddit.com", "m.reddit.com", - "redd.it", + // NOTE: We don't use redd.it. // Imgur // @@ -311,7 +314,7 @@ public class OWSLinkPreview: MTLModel { // For example, you can access "user/member" pages: https://sillygoose2.imgur.com/ // A different member page can be accessed without a subdomain: https://imgur.com/user/SillyGoose2 // - // I'm not sure we need to support these subdomains; they don't appear to be core functionality? + // I'm not sure we need to support these subdomains; they don't appear to be core functionality. "imgur.com", "www.imgur.com", "m.imgur.com", @@ -319,17 +322,21 @@ public class OWSLinkPreview: MTLModel { // Instagram "instagram.com", "www.instagram.com", - "m.instagram.com", - - // Giphy - "giphy.com", - "www.giphy.com", - "media.giphy.com", - "gph.is" + "m.instagram.com" ] + // For media domains, we DO NOT require an exact match - subdomains are allowed. private static let mediaDomainWhitelist = [ + // YouTube "ytimg.com", + + // Reddit + "redd.it", + + // Imgur + "imgur.com", + + // Instagram "cdninstagram.com" ] @@ -355,7 +362,7 @@ public class OWSLinkPreview: MTLModel { guard let result = whitelistedDomain(forUrl: url, domainWhitelist: OWSLinkPreview.linkDomainWhitelist, allowSubdomains: false) else { - owsFailDebug("Missing domain.") + Logger.error("Missing domain.") return nil } return result @@ -377,7 +384,7 @@ public class OWSLinkPreview: MTLModel { return false } return whitelistedDomain(forUrl: url, - domainWhitelist: OWSLinkPreview.linkDomainWhitelist + OWSLinkPreview.mediaDomainWhitelist, + domainWhitelist: OWSLinkPreview.mediaDomainWhitelist, allowSubdomains: true) != nil } diff --git a/SignalServiceKit/tests/Messages/OWSLinkPreviewTest.swift b/SignalServiceKit/tests/Messages/OWSLinkPreviewTest.swift index f8b6e02fd..979dbaca8 100644 --- a/SignalServiceKit/tests/Messages/OWSLinkPreviewTest.swift +++ b/SignalServiceKit/tests/Messages/OWSLinkPreviewTest.swift @@ -96,8 +96,8 @@ class OWSLinkPreviewTest: SSKBaseTestSwift { // Case shouldn't matter. XCTAssertTrue(OWSLinkPreview.isValidLinkUrl("https://WWW.YOUTUBE.COM/watch?v=tP-Ipsat90c")) - // Allow arbitrary subdomains. - XCTAssertTrue(OWSLinkPreview.isValidMediaUrl("https://some.random.subdomain.youtube.com/watch?v=tP-Ipsat90c")) + // Don't allow arbitrary subdomains. + XCTAssertFalse(OWSLinkPreview.isValidMediaUrl("https://some.random.subdomain.youtube.com/watch?v=tP-Ipsat90c")) // Don't allow HTTP, only HTTPS XCTAssertFalse(OWSLinkPreview.isValidLinkUrl("http://youtube.com/watch?v=tP-Ipsat90c")) @@ -136,24 +136,26 @@ class OWSLinkPreviewTest: SSKBaseTestSwift { } func testIsValidMediaUrl() { - XCTAssertTrue(OWSLinkPreview.isValidMediaUrl("https://www.youtube.com/watch?v=tP-Ipsat90c")) - XCTAssertTrue(OWSLinkPreview.isValidMediaUrl("https://youtube.com/watch?v=tP-Ipsat90c")) + // Only allow domains on the media whitelist. + XCTAssertFalse(OWSLinkPreview.isValidMediaUrl("https://www.youtube.com/watch?v=tP-Ipsat90c")) + XCTAssertFalse(OWSLinkPreview.isValidMediaUrl("https://youtube.com/watch?v=tP-Ipsat90c")) // Allow arbitrary subdomains. - XCTAssertTrue(OWSLinkPreview.isValidMediaUrl("https://some.random.subdomain.youtube.com/watch?v=tP-Ipsat90c")) + XCTAssertTrue(OWSLinkPreview.isValidMediaUrl("https://ytimg.com/something")) + XCTAssertTrue(OWSLinkPreview.isValidMediaUrl("https://something.ytimg.com/something")) // Don't allow HTTP, only HTTPS - XCTAssertFalse(OWSLinkPreview.isValidMediaUrl("http://youtube.com/watch?v=tP-Ipsat90c")) - XCTAssertFalse(OWSLinkPreview.isValidMediaUrl("mailto://youtube.com/watch?v=tP-Ipsat90c")) - XCTAssertFalse(OWSLinkPreview.isValidMediaUrl("ftp://youtube.com/watch?v=tP-Ipsat90c")) + XCTAssertFalse(OWSLinkPreview.isValidMediaUrl("http://ytimg.com/watch?v=tP-Ipsat90c")) + XCTAssertFalse(OWSLinkPreview.isValidMediaUrl("mailto://ytimg.com/watch?v=tP-Ipsat90c")) + XCTAssertFalse(OWSLinkPreview.isValidMediaUrl("ftp://ytimg.com/watch?v=tP-Ipsat90c")) // Don't allow similar domains. - XCTAssertFalse(OWSLinkPreview.isValidMediaUrl("https://xyoutube.com/watch?v=tP-Ipsat90c")) + XCTAssertFalse(OWSLinkPreview.isValidMediaUrl("https://xytimg.com/watch?v=tP-Ipsat90c")) XCTAssertFalse(OWSLinkPreview.isValidMediaUrl("https://youtubex.com/watch?v=tP-Ipsat90c")) - XCTAssertFalse(OWSLinkPreview.isValidMediaUrl("https://youtube.comx/watch?v=tP-Ipsat90c")) - XCTAssertFalse(OWSLinkPreview.isValidMediaUrl("https://www.xyoutube.com/watch?v=tP-Ipsat90c")) - XCTAssertFalse(OWSLinkPreview.isValidMediaUrl("https://www.youtubex.com/watch?v=tP-Ipsat90c")) - XCTAssertFalse(OWSLinkPreview.isValidMediaUrl("https://www.youtube.comx/watch?v=tP-Ipsat90c")) + XCTAssertFalse(OWSLinkPreview.isValidMediaUrl("https://ytimg.comx/watch?v=tP-Ipsat90c")) + XCTAssertFalse(OWSLinkPreview.isValidMediaUrl("https://www.xytimg.com/watch?v=tP-Ipsat90c")) + XCTAssertFalse(OWSLinkPreview.isValidMediaUrl("https://www.ytimgx.com/watch?v=tP-Ipsat90c")) + XCTAssertFalse(OWSLinkPreview.isValidMediaUrl("https://www.ytimg.comx/watch?v=tP-Ipsat90c")) // Allow media domains. XCTAssertTrue(OWSLinkPreview.isValidMediaUrl("https://i.ytimg.com/vi/tP-Ipsat90c/maxresdefault.jpg"))