Add tag muted-text BaseHTMLEngineProvider to render html errors #5550

aldo-expensify · 2021-09-28T00:26:27Z

Details

The new error text:

You have already set up this bank account on Expensify. If you'd like to add it again, please delete it here.

Comes from Web-Secure backend and contains a link, so I'll be sending it using HTML. See related Web-Secure PR

Added new tag muted-text in BaseHTMLEngineProvider, this tag is only for setting the style mutedTextLabel when we find the tag. The idea is that if we receive an html error from the back, we wrap it in <muted-text> and render it using RenderHTML.

Also, we currently render all <a> with external urls in href as AnchorForCommentsOnly. I introduced a custom tag <comment> to wrap comment's html, and use it to detect if we are rendering a <a> within a comment or not. If we are rendering a <a> outside of a comment, like the error coming from the back end, we render it using a <Text> because AnchorForCommentsOnly:

Adds a context menu that is not present in links in other parts of the app
Doesn't align well with the rest of the text in Android

Fixed Issues

$ #5626
https://github.com/Expensify/Expensify/issues/177017

Tests / QA

Dev: make sure you have the changes from this PR in you workspace

Create a new account in OldDot, verify the email
Still in OldDot, add an open VBA using data from this SO (3). Add VBA can be found in: Settings > Account > Payments > Add Verified Bank Account
Log in with the same account in NewDot
Create a workspace and start the add VBA process
Choose to add manually the bank account, input Routing Number: 011401533 and Account number: 1111222233331111
Click "Save & continue"
You should see the following error above the "Save & Continue" button:

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

When we get have an html error to display, we can wrap the error in <muted-text> to make it have our 'mutedTextLabel' styling.

flodnv · 2021-09-28T11:13:40Z

src/libs/actions/BankAccounts.js

@@ -751,7 +753,8 @@ function setupWithdrawalAccount(data) {
                }
            } else {
                if (response.jsonCode === 666 || response.jsonCode === 404) {
-                    error = response.message;
+                    error = response.message || response.htmlMessage;


I am not sure I understand this change? In general we prefer the HTML message first (on OldDot)

Jag96 · 2021-09-28T20:44:06Z

src/libs/actions/BankAccounts.js

-function showBankAccountErrorModal(errorModalMessage = null) {
-    Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {isErrorModalVisible: true, errorModalMessage});
+function showBankAccountErrorModal(errorModalMessage = null, isErrorModalMessageHtml = false) {
+    Onyx.merge(ONYXKEYS.REIMBURSEMENT_ACCOUNT, {isErrorModalVisible: true, errorModalMessage, isErrorModalMessageHtml});


Just to confirm here, if we call merge the value of isErrorModalMessageHtml will be overwritten right?

I understand that is the case

src/libs/actions/BankAccounts.js

joelbettner · 2021-09-29T18:02:48Z

src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js

@@ -231,6 +235,7 @@ const renderers = {
    code: CodeRenderer,
    img: ImgRenderer,
    edited: withLocalize(EditedRenderer),
+    'muted-text': TNodeChildrenRenderer,


Just so I'm clear on why we are doing this, please correct me if I'm wrong...

From reading about it, I think a TNodeChildrenRenderer element is designed so it will render ONLY that element and its children when some data within the TNodeChildrenRenderer element changes. This prevents us from re-rendering a bunch of things unnecessarily. Is that correct?

Also, when we use it, why are we nesting it inside a <RenderHTML rather than a <Text?

About TNodeChildrenRenderer... I actually didn't read the documentation about it, I just saw other usages like here:

App/src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js

Lines 69 to 111 in e3e02c9

function AnchorRenderer({tnode, key, style}) {

const htmlAttribs = tnode.attributes;

// An auth token is needed to download Expensify chat attachments

const isAttachment = Boolean(htmlAttribs['data-expensify-source']);

const fileName = lodashGet(tnode, 'domNode.children[0].data', '');

const parentStyle = lodashGet(tnode, 'parent.styles.nativeTextRet', {});

const internalExpensifyPath = (htmlAttribs.href.startsWith(CONST.NEW_EXPENSIFY_URL) && htmlAttribs.href.replace(CONST.NEW_EXPENSIFY_URL, ''))

|| (htmlAttribs.href.startsWith(CONST.STAGING_NEW_EXPENSIFY_URL) && htmlAttribs.href.replace(CONST.STAGING_NEW_EXPENSIFY_URL, ''));

// If we are handling a New Expensify link then we will assume this should be opened by the app internally. This ensures that the links are opened internally via react-navigation

// instead of in a new tab or with a page refresh (which is the default behavior of an anchor tag)

if (internalExpensifyPath) {

return (

<Text

style={styles.link}

onPress={() => Navigation.navigate(internalExpensifyPath)}

>

<TNodeChildrenRenderer tnode={tnode} />

</Text>

);

}

return (

<AnchorForCommentsOnly

href={htmlAttribs.href}

isAuthTokenRequired={isAttachment}

// Unless otherwise specified open all links in

// a new window. On Desktop this means that we will

// skip the default Save As... download prompt

// and defer to whatever browser the user has.

// eslint-disable-next-line react/jsx-props-no-multi-spaces

target={htmlAttribs.target || '_blank'}

rel={htmlAttribs.rel || 'noopener noreferrer'}

style={{...style, ...parentStyle}}

key={key}

fileName={fileName}

>

<TNodeChildrenRenderer tnode={tnode} />

</AnchorForCommentsOnly>

);

}

I just needed something that rendered the rest of the html as it would by default, but allowed me to use the react native styles mutedTextLabel. I did the test of removing 'muted-text': TNodeChildrenRenderer, and it works the same, seems like it is not doing anything useful.

The code

'muted-text': defaultHTMLElementModels.p.extend({ tagName: 'muted-text', mixedUAStyles: styles.mutedTextLabel, }),

I would expect is would make the element muted-text render as a <p> with our mutedTextLabel styles, but in reality this is rendering it as a <div>.

Using defaultHTMLElementModels.div.extend broke the layout, that's why I ended up using p, which for me made sense as a parent element of a message.

I'll do some testing on an Android device to confirm this is not working only in web.

Also, when we use it, why are we nesting it inside a <RenderHTML rather than a <Text?

Because the <Text doesn't render html

joelbettner

I did the test of removing 'muted-text': TNodeChildrenRenderer, and it works the same, seems like it is not doing anything useful.

Yeah...if it is not doing anything then lets get rid of it.

aldo-expensify · 2021-09-29T19:16:58Z

Update:

Got rid of 'muted-text': TNodeChildrenRenderer

Putting this back on WIP for a bit because in Android the link (here) in the error looks miss-aligned 🥲 ... I'll see if there is anything I can do about that!

aldo-expensify · 2021-09-29T20:52:36Z

The miss-aligned "here" link in the error comes from using a <PressableWithSecondaryInteraction to render the anchor tag:

App/src/components/AnchorForCommentsOnly/BaseAnchorForCommentsOnly/index.native.js

Lines 39 to 60 in 56374e8

    
               <PressableWithSecondaryInteraction 
        
                   onSecondaryInteraction={ 
        
               (event) => { 
        
                   showContextMenu( 
        
                       CONTEXT_MENU_TYPES.LINK, 
        
                       event, 
        
                       href, 
        
                       lodashGet(linkRef, 'current'), 
        
                   ); 
        
               } 
        
           } 
        
                   onPress={() => Linking.openURL(href)} 
        
               > 
        
                   <Text 
        
                       ref={el => linkRef = el} 
        
                       style={StyleSheet.flatten(style)} 
        
               // eslint-disable-next-line react/jsx-props-no-spreading 
        
                       {...props} 
        
                   > 
        
                       {children} 
        
                   </Text> 
        
               </PressableWithSecondaryInteraction>

We end up using this pressable because any anchor <a> which href is not an internal NewDot link gets rendered as an AnchorForCommentsOnly:

App/src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js

Lines 92 to 110 in 56374e8

    
           return ( 
        
               <AnchorForCommentsOnly 
        
                   href={htmlAttribs.href} 
        
                   isAuthTokenRequired={isAttachment} 
        
                   // Unless otherwise specified open all links in 
        
                   // a new window. On Desktop this means that we will 
        
                   // skip the default Save As... download prompt 
        
                   // and defer to whatever browser the user has. 
        
                   // eslint-disable-next-line react/jsx-props-no-multi-spaces 
        
                   target={htmlAttribs.target || '_blank'} 
        
                   rel={htmlAttribs.rel || 'noopener noreferrer'} 
        
                   style={{...style, ...parentStyle}} 
        
                   key={key} 
        
                   fileName={fileName} 
        
               > 
        
                   <TNodeChildrenRenderer tnode={tnode} /> 
        
               </AnchorForCommentsOnly> 
        
           );

BaseHTMLEngineProvider.js is registering AnchorRenderer for <a> here:

App/src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js

Lines 229 to 231 in 56374e8

    
           const renderers = { 
        
               a: AnchorRenderer, 
        
               code: CodeRenderer,

Possible solutions I see:

This is not an achor in a "comment", so it is probably incorrect to end up rendering this using AnchorForCommentsOnly. We could improve our checks here and make sure we render it using a Text
Fix the rendering of <AnchorForCommentsOnly so they don't look miss-aligned when inline with text.
Do both

aldo-expensify · 2021-09-29T20:55:31Z

I see that the miss-aligned link is also happening in the chat for Android:

I'll try to do this

This is not an achor in a "comment", so it is probably incorrect to end up rendering this using AnchorForCommentsOnly. We could improve our checks here and make sure we render it using a Text

And fixing the miss-aligned <AnchorForCommentsOnly wont be part of this PR since this is unrelated and was happening before. I'll create a new GH issue for that!

In our HTMLEngineProvider, we are always handling <a> tags using AnchorRenderer. This renderer will always end up using AnchorForCommentsOnly if the href of the <a> is not an internal link. We should not use AnchorForCommentsOnly if this <a> is not within the context of a comment in a chat.

aldo-expensify · 2021-09-30T00:06:40Z

Update: to avoid using AnchorForCommentsOnly in contexts that were not comments, we needed someone to recognize that. The cleanest way I found, was this:

Add new boolean prop comment to the RenderHtml component. If the comment prop is true, we wrap the html in a <div> with a special attribute data-comment.
From AnchorRenderer, check the ancestor nodes until we find or not find the special attribute. Finding it means that we are rendering inside a comment and we can use AnchorForCommentsOnly
If we don't find it, we default to use a more "default" way of rendering a link:

<Text
    style={styles.link}
    onPress={() => Navigation.navigate(internalExpensifyPath)}
>
    <TNodeChildrenRenderer tnode={tnode} />
</Text>

Result:

Update: well, thinking a bit more about this, it doesn't feel so right that the RenderHtml component knows about "comments". I would still lean to similar solutions, but maybe the prop in RenderHtml could be more "generic"... like context, and you can pass just a string as a flag that would be added to the html in a similar way...

aldo-expensify · 2021-09-30T00:56:43Z

The miss-alignment link issue was reported here already: #5319

Chatted with @MitchExpensify about it, we will create a new clean GH issue. If this PR goes through, the only miss alignment remaining will be in links sent on the chat.

To make it more consistent with <muted-text> tag solution, I added a <comment> tag to be able to recognize we are rendering a link within a comment. RenderHtml doesn't know about comments again.

aldo-expensify · 2021-09-30T18:21:54Z

Sorry, closed by mistake...

Update:

Refactored detection of link in comment

To make it more consistent with tag solution,
I added a tag to be able to recognize we are rendering
a link within a comment.

RenderHtml doesn't know about comments again.

src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js

src/libs/actions/BankAccounts.js

src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js

src/pages/ReimbursementAccount/ReimbursementAccountForm.js

src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js

…port-html-error

aldo-expensify · 2021-10-06T19:06:51Z

Update: Resolved conflicts.

Luke9389

Lookin' good Aldo. Ignore my single comment

src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js

Jag96

Tested on web and this LGTM. I noticed that if you click the error message link on the mobile app, you are taken to the sign-in screen on mobile web for OldDot and after signing in the redirect doesn't happen. I don't think that is part of this PR/issue, but just mentioning it in case.

aldo-expensify · 2021-10-13T17:16:46Z

@flodnv friendly review reminder :)

flodnv · 2021-10-13T17:23:01Z

Sorry about that, I did not intend to be a reviewer here. Feel free to merge if all is well!

OSBotify · 2021-10-13T17:42:35Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2021-10-15T06:40:19Z

🚀 Deployed to staging by @Jag96 in version: 1.1.7-25 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2021-10-23T00:42:41Z

🚀 Deployed to production by @roryabraham in version: 1.1.8-9 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

Add tag muted-text BaseHTMLEngineProvider

d816738

When we get have an html error to display, we can wrap the error in <muted-text> to make it have our 'mutedTextLabel' styling.

aldo-expensify self-assigned this Sep 28, 2021

flodnv reviewed Sep 28, 2021

View reviewed changes

Prioritize backend error message

1a55303

Jag96 reviewed Sep 28, 2021

View reviewed changes

Add comment for htmlMessage priority over message

fccac00

aldo-expensify marked this pull request as ready for review September 29, 2021 00:08

aldo-expensify requested a review from a team as a code owner September 29, 2021 00:08

MelvinBot requested review from joelbettner and removed request for a team September 29, 2021 00:08

joelbettner reviewed Sep 29, 2021

View reviewed changes

joelbettner suggested changes Sep 29, 2021

View reviewed changes

Remove useless code - custom renderer

c8faa40

aldo-expensify changed the title ~~Add tag muted-text BaseHTMLEngineProvider to render html errors~~ [WIP] Add tag muted-text BaseHTMLEngineProvider to render html errors Sep 29, 2021

aldo-expensify changed the title ~~[WIP] Add tag muted-text BaseHTMLEngineProvider to render html errors~~ Add tag muted-text BaseHTMLEngineProvider to render html errors Sep 30, 2021

Refactored detection of link in comment

667be37

To make it more consistent with <muted-text> tag solution, I added a <comment> tag to be able to recognize we are rendering a link within a comment. RenderHtml doesn't know about comments again.

aldo-expensify closed this Sep 30, 2021

aldo-expensify reopened this Sep 30, 2021

Luke9389 reviewed Oct 1, 2021

View reviewed changes

aldo-expensify added 2 commits October 1, 2021 10:57

Update variable name

383004a

Revert unnecessary styling change

91f996f

Jag96 reviewed Oct 4, 2021

View reviewed changes

src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js Outdated Show resolved Hide resolved

src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js Outdated Show resolved Hide resolved

Merge branch 'main' of github.com:Expensify/App into aldo_vba-add-sup…

d221bf2

…port-html-error

aldo-expensify added 3 commits October 5, 2021 15:52

Added comment on using Text for <a>

682e5b4

Leave default renderer for tag comment

1620af7

Merge branch 'main' / Resolve conflicts

c12931b

joelbettner approved these changes Oct 6, 2021

View reviewed changes

Luke9389 approved these changes Oct 6, 2021

View reviewed changes

src/components/HTMLEngineProvider/BaseHTMLEngineProvider.js Show resolved Hide resolved

Jag96 approved these changes Oct 12, 2021

View reviewed changes

Jag96 merged commit 23bcfc9 into main Oct 13, 2021

Jag96 deleted the aldo_vba-add-support-html-error branch October 13, 2021 17:41

OSBotify mentioned this pull request Oct 15, 2021

Deploy Checklist: New Expensify 2021-10-15 #5879

Closed

	function AnchorRenderer({tnode, key, style}) {
	const htmlAttribs = tnode.attributes;

	// An auth token is needed to download Expensify chat attachments
	const isAttachment = Boolean(htmlAttribs['data-expensify-source']);
	const fileName = lodashGet(tnode, 'domNode.children[0].data', '');
	const parentStyle = lodashGet(tnode, 'parent.styles.nativeTextRet', {});
	const internalExpensifyPath = (htmlAttribs.href.startsWith(CONST.NEW_EXPENSIFY_URL) && htmlAttribs.href.replace(CONST.NEW_EXPENSIFY_URL, ''))
	\|\| (htmlAttribs.href.startsWith(CONST.STAGING_NEW_EXPENSIFY_URL) && htmlAttribs.href.replace(CONST.STAGING_NEW_EXPENSIFY_URL, ''));

	// If we are handling a New Expensify link then we will assume this should be opened by the app internally. This ensures that the links are opened internally via react-navigation
	// instead of in a new tab or with a page refresh (which is the default behavior of an anchor tag)
	if (internalExpensifyPath) {
	return (
	<Text
	style={styles.link}
	onPress={() => Navigation.navigate(internalExpensifyPath)}
	>
	<TNodeChildrenRenderer tnode={tnode} />
	</Text>
	);
	}

	return (
	<AnchorForCommentsOnly
	href={htmlAttribs.href}
	isAuthTokenRequired={isAttachment}

	// Unless otherwise specified open all links in
	// a new window. On Desktop this means that we will
	// skip the default Save As... download prompt
	// and defer to whatever browser the user has.
	// eslint-disable-next-line react/jsx-props-no-multi-spaces
	target={htmlAttribs.target \|\| '_blank'}
	rel={htmlAttribs.rel \|\| 'noopener noreferrer'}
	style={{...style, ...parentStyle}}
	key={key}
	fileName={fileName}
	>
	<TNodeChildrenRenderer tnode={tnode} />
	</AnchorForCommentsOnly>
	);
	}

Add tag muted-text BaseHTMLEngineProvider to render html errors #5550

Add tag muted-text BaseHTMLEngineProvider to render html errors #5550

Uh oh!

Conversation

aldo-expensify commented Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests / QA

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

flodnv Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

Jag96 Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

aldo-expensify Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joelbettner Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

aldo-expensify Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

joelbettner left a comment

Choose a reason for hiding this comment

Uh oh!

aldo-expensify commented Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aldo-expensify commented Sep 29, 2021

Uh oh!

aldo-expensify commented Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aldo-expensify commented Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aldo-expensify commented Sep 30, 2021

Uh oh!

aldo-expensify commented Sep 30, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aldo-expensify commented Oct 6, 2021

Uh oh!

Luke9389 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Jag96 left a comment

Choose a reason for hiding this comment

Uh oh!

aldo-expensify commented Oct 13, 2021

Uh oh!

flodnv commented Oct 13, 2021

Uh oh!

OSBotify commented Oct 13, 2021

Uh oh!

OSBotify commented Oct 15, 2021

Uh oh!

OSBotify commented Oct 23, 2021

Uh oh!

Uh oh!

aldo-expensify commented Sep 28, 2021 •

edited

Loading

aldo-expensify commented Sep 29, 2021 •

edited

Loading

aldo-expensify commented Sep 29, 2021 •

edited

Loading

aldo-expensify commented Sep 30, 2021 •

edited

Loading