Skip to content
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

fix image from opening in both a new tab and app modal #52013

Merged
merged 15 commits into from
Nov 18, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/ROUTES.ts
Original file line number Diff line number Diff line change
Expand Up @@ -310,11 +310,13 @@ const ROUTES = {
},
ATTACHMENTS: {
route: 'attachment',
getRoute: (reportID: string, type: ValueOf<typeof CONST.ATTACHMENT_TYPE>, url: string, accountID?: number, isAuthTokenRequired?: boolean) => {
getRoute: (reportID: string, type: ValueOf<typeof CONST.ATTACHMENT_TYPE>, url: string, accountID?: number, isAuthTokenRequired?: boolean, imageLink?: string) => {
const reportParam = reportID ? `&reportID=${reportID}` : '';
const accountParam = accountID ? `&accountID=${accountID}` : '';
const authTokenParam = isAuthTokenRequired ? '&isAuthTokenRequired=true' : '';
return `attachment?source=${encodeURIComponent(url)}&type=${type}${reportParam}${accountParam}${authTokenParam}` as const;
const imageLinkParam = imageLink ? `&imageLink=${imageLink}` : '';

return `attachment?source=${encodeURIComponent(url)}&type=${type}${reportParam}${accountParam}${authTokenParam}${imageLinkParam}` as const;
},
},
REPORT_PARTICIPANTS: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import type {BaseAnchorForCommentsOnlyProps, LinkProps} from './types';
/*
* This is a default anchor component for regular links.
*/
function BaseAnchorForCommentsOnly({onPressIn, onPressOut, href = '', rel = '', target = '', children = null, style, onPress, ...rest}: BaseAnchorForCommentsOnlyProps) {
function BaseAnchorForCommentsOnly({onPressIn, onPressOut, href = '', rel = '', target = '', children = null, style, onPress, isLinkHasImage, ...rest}: BaseAnchorForCommentsOnlyProps) {
const styles = useThemeStyles();
const StyleUtils = useStyleUtils();
const linkRef = useRef<RNText>(null);
Expand Down Expand Up @@ -62,7 +62,7 @@ function BaseAnchorForCommentsOnly({onPressIn, onPressOut, href = '', rel = '',
role={CONST.ROLE.LINK}
accessibilityLabel={href}
>
<Tooltip text={href}>
<Tooltip text={!isLinkHasImage ? href : undefined}>
huult marked this conversation as resolved.
Show resolved Hide resolved
<Text
ref={linkRef}
style={StyleSheet.flatten([style, defaultTextStyle])}
Expand All @@ -71,7 +71,7 @@ function BaseAnchorForCommentsOnly({onPressIn, onPressOut, href = '', rel = '',
rel,
target: isEmail || !linkProps.href ? '_self' : target,
}}
href={href}
href={!isLinkHasImage ? href : undefined}
suppressHighlighting
// Add testID so it gets selected as an anchor tag by SelectionScraper
testID="a"
Expand Down
3 changes: 3 additions & 0 deletions src/components/AnchorForCommentsOnly/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ type AnchorForCommentsOnlyProps = ChildrenProps & {

/** Press handler for the link, when not passed, default href is used to create a link like behaviour */
onPress?: () => void;

/** Indicates whether an image is wrapped in an anchor (`<a>`) tag with an `href` link */
isLinkHasImage?: boolean;
huult marked this conversation as resolved.
Show resolved Hide resolved
};

type BaseAnchorForCommentsOnlyProps = AnchorForCommentsOnlyProps & {
Expand Down
22 changes: 22 additions & 0 deletions src/components/AttachmentModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,8 @@ type AttachmentModalProps = {
canEditReceipt?: boolean;

shouldDisableSendButton?: boolean;

imageLink?: string;
};

function AttachmentModal({
Expand Down Expand Up @@ -161,6 +163,7 @@ function AttachmentModal({
type = undefined,
accountID = undefined,
shouldDisableSendButton = false,
imageLink = '',
}: AttachmentModalProps) {
const styles = useThemeStyles();
const StyleUtils = useStyleUtils();
Expand All @@ -185,6 +188,7 @@ function AttachmentModal({
const parentReportAction = ReportActionsUtils.getReportAction(report?.parentReportID ?? '-1', report?.parentReportActionID ?? '-1');
const transactionID = ReportActionsUtils.isMoneyRequestAction(parentReportAction) ? ReportActionsUtils.getOriginalMessage(parentReportAction)?.IOUTransactionID ?? '-1' : '-1';
const [transaction] = useOnyx(`${ONYXKEYS.COLLECTION.TRANSACTION}${transactionID}`);
const [attachmentCarouselImageLink, setAttachmentCarouselImageLink] = useState('');

const [file, setFile] = useState<FileObject | undefined>(
originalFileName
Expand All @@ -211,6 +215,7 @@ function AttachmentModal({
setFile(attachment.file);
setIsAuthTokenRequiredState(attachment.isAuthTokenRequired ?? false);
onCarouselAttachmentChange(attachment);
setAttachmentCarouselImageLink(attachment?.imageLink ?? '');
},
[onCarouselAttachmentChange],
);
Expand Down Expand Up @@ -482,6 +487,22 @@ function AttachmentModal({

const submitRef = useRef<View | HTMLElement>(null);

const getSubTitleLink = () => {
huult marked this conversation as resolved.
Show resolved Hide resolved
if (shouldShowNotFoundPage) {
return '';
}

if (!isEmptyObject(report) && !isReceiptAttachment) {
return attachmentCarouselImageLink;
}

if (!isAuthTokenRequired && imageLink) {
return imageLink;
}

return '';
};

return (
<>
<Modal
Expand Down Expand Up @@ -527,6 +548,7 @@ function AttachmentModal({
threeDotsAnchorPosition={styles.threeDotsPopoverOffsetAttachmentModal(windowWidth)}
threeDotsMenuItems={threeDotsMenuItems}
shouldOverlayDots
subTitleLink={getSubTitleLink()}
/>
<View style={styles.imageModalImageCenterContainer}>
{isLoading && <FullScreenLoadingIndicator />}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,13 @@ function extractAttachments(
// and navigating back (<) shows the image preceding the first instance, not the selected duplicate's position.
const uniqueSources = new Set();

let currentImageLink = '';

const htmlParser = new HtmlParser({
onopentag: (name, attribs) => {
if (name === 'a' && attribs.href) {
currentImageLink = attribs.href;
}
if (name === 'video') {
const source = tryResolveUrlFromApiRoot(attribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE]);
if (uniqueSources.has(source)) {
Expand Down Expand Up @@ -81,9 +86,17 @@ function extractAttachments(
file: {name: fileName, width, height},
isReceipt: false,
hasBeenFlagged: attribs['data-flagged'] === 'true',
imageLink: currentImageLink,
});
}
},
onclosetag: (name) => {
if (!(name === 'a') || !currentImageLink) {
huult marked this conversation as resolved.
Show resolved Hide resolved
return;
}

currentImageLink = '';
},
});

if (type === CONST.ATTACHMENT_TYPE.NOTE) {
Expand Down
2 changes: 2 additions & 0 deletions src/components/Attachments/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ type Attachment = {
isReceipt?: boolean;

duration?: number;

imageLink?: string;
};

export type {AttachmentSource, Attachment};
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ function AnchorRenderer({tnode, style, key}: AnchorRendererProps) {
const internalNewExpensifyPath = Link.getInternalNewExpensifyPath(attrHref);
const internalExpensifyPath = Link.getInternalExpensifyPath(attrHref);
const isVideo = attrHref && Str.isVideo(attrHref);
const isLinkHasImage = tnode.tagName === 'a' && tnode.children.some((child) => child.tagName === 'img');

const isDeleted = HTMLEngineUtils.isDeletedNode(tnode);
const textDecorationLineStyle = isDeleted ? styles.underlineLineThrough : {};
Expand Down Expand Up @@ -73,6 +74,7 @@ function AnchorRenderer({tnode, style, key}: AnchorRendererProps) {
key={key}
// Only pass the press handler for internal links. For public links or whitelisted internal links fallback to default link handling
onPress={internalNewExpensifyPath || internalExpensifyPath ? () => Link.openLink(attrHref, environmentURL, isAttachment) : undefined}
isLinkHasImage={isLinkHasImage}
>
<TNodeChildrenRenderer
tnode={tnode}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@ function ImageRenderer({tnode}: ImageRendererProps) {
return;
}

const route = ROUTES.ATTACHMENTS?.getRoute(reportID ?? '-1', type, source, accountID, isAttachmentOrReceipt);
const imageLink = tnode.parent?.attributes?.href;
const route = ROUTES.ATTACHMENTS?.getRoute(reportID ?? '-1', type, source, accountID, isAttachmentOrReceipt, imageLink);
Navigation.navigate(route);
}}
onLongPress={(event) => {
Expand Down
24 changes: 22 additions & 2 deletions src/components/Header.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import type {ReactNode} from 'react';
import React, {useMemo} from 'react';
import type {StyleProp, TextStyle, ViewStyle} from 'react-native';
import {View} from 'react-native';
import {Linking, View} from 'react-native';
import useThemeStyles from '@hooks/useThemeStyles';
import EnvironmentBadge from './EnvironmentBadge';
import Text from './Text';
import TextLink from './TextLink';

type HeaderProps = {
/** Title of the Header */
Expand All @@ -21,9 +22,12 @@ type HeaderProps = {

/** Additional header container styles */
containerStyles?: StyleProp<ViewStyle>;

/** The URL link associated with the attachment's subtitle, if available */
subTitleLink?: string;
};

function Header({title = '', subtitle = '', textStyles = [], containerStyles = [], shouldShowEnvironmentBadge = false}: HeaderProps) {
function Header({title = '', subtitle = '', textStyles = [], containerStyles = [], shouldShowEnvironmentBadge = false, subTitleLink = ''}: HeaderProps) {
const styles = useThemeStyles();
const renderedSubtitle = useMemo(
() => (
Expand All @@ -43,6 +47,21 @@ function Header({title = '', subtitle = '', textStyles = [], containerStyles = [
),
[subtitle, styles],
);

const renderedSubTitleLink = () => {
huult marked this conversation as resolved.
Show resolved Hide resolved
return (
<Text numberOfLines={1}>
<TextLink
huult marked this conversation as resolved.
Show resolved Hide resolved
onPress={() => {
Linking.openURL(subTitleLink);
}}
>
{subTitleLink}
</TextLink>
</Text>
);
};

return (
<View style={[styles.flex1, styles.flexRow, containerStyles]}>
<View style={styles.mw100}>
Expand All @@ -57,6 +76,7 @@ function Header({title = '', subtitle = '', textStyles = [], containerStyles = [
)
: title}
{renderedSubtitle}
{subTitleLink && renderedSubTitleLink()}
</View>
{shouldShowEnvironmentBadge && <EnvironmentBadge />}
</View>
Expand Down
3 changes: 3 additions & 0 deletions src/components/HeaderWithBackButton/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ function HeaderWithBackButton({
shouldDisplaySearchRouter = false,
progressBarPercentage,
style,
subTitleLink = '',
}: HeaderWithBackButtonProps) {
const theme = useTheme();
const styles = useThemeStyles();
Expand Down Expand Up @@ -108,10 +109,12 @@ function HeaderWithBackButton({
title={title}
subtitle={stepCounter ? translate('stepCounter', stepCounter) : subtitle}
textStyles={[titleColor ? StyleUtils.getTextColorStyle(titleColor) : {}, isCentralPaneSettings && styles.textHeadlineH2]}
subTitleLink={subTitleLink}
/>
);
}, [
StyleUtils,
subTitleLink,
isCentralPaneSettings,
policy,
progressBarPercentage,
Expand Down
3 changes: 3 additions & 0 deletions src/components/HeaderWithBackButton/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,9 @@ type HeaderWithBackButtonProps = Partial<ChildrenProps> & {

/** Additional styles to add to the component */
style?: StyleProp<ViewStyle>;

/** The URL link associated with the attachment's subtitle, if available */
subTitleLink?: string;
};

export type {ThreeDotsMenuItem};
Expand Down
1 change: 1 addition & 0 deletions src/libs/Navigation/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1549,6 +1549,7 @@ type AuthScreensParamList = CentralPaneScreensParamList &
type: ValueOf<typeof CONST.ATTACHMENT_TYPE>;
accountID: string;
isAuthTokenRequired?: string;
imageLink?: string;
};
[SCREENS.PROFILE_AVATAR]: {
accountID: string;
Expand Down
6 changes: 4 additions & 2 deletions src/pages/home/report/ReportAttachments.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ function ReportAttachments({route}: ReportAttachmentsProps) {
const type = route.params.type;
const accountID = route.params.accountID;
const isAuthTokenRequired = route.params.isAuthTokenRequired;
const imageLink = route.params.imageLink;
const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID || -1}`);
const [isLoadingApp] = useOnyx(ONYXKEYS.IS_LOADING_APP);

Expand All @@ -26,10 +27,10 @@ function ReportAttachments({route}: ReportAttachmentsProps) {

const onCarouselAttachmentChange = useCallback(
(attachment: Attachment) => {
const routeToNavigate = ROUTES.ATTACHMENTS.getRoute(reportID, type, String(attachment.source), Number(accountID));
const routeToNavigate = ROUTES.ATTACHMENTS.getRoute(reportID, type, String(attachment.source), Number(accountID), attachment.isAuthTokenRequired, attachment.imageLink);
Navigation.navigate(routeToNavigate);
},
[reportID, accountID, type],
[reportID, type, accountID],
);

return (
Expand All @@ -48,6 +49,7 @@ function ReportAttachments({route}: ReportAttachmentsProps) {
onCarouselAttachmentChange={onCarouselAttachmentChange}
shouldShowNotFoundPage={!isLoadingApp && type !== CONST.ATTACHMENT_TYPE.SEARCH && !report?.reportID}
isAuthTokenRequired={!!isAuthTokenRequired}
imageLink={imageLink ?? ''}
/>
);
}
Expand Down
Loading