Skip to content

Commit

Permalink
Fix #23304 - Only use Confusable during send with ENS (#23582)
Browse files Browse the repository at this point in the history
## *Description**

We should only be using `Confusable` for AddressListItem when the user
is entering an unknown address during the send flow or adding a contact.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/23582?quickstart=1)

## **Related issues**

Fixes: #23304

## **Manual testing steps**

1. Open the new send flow
2. Type `metamask.eth`
3. See red 'm's, noting that they could be a `rn`
4. Type `vitalik.eth`
5. See no red characters since there are no confusable letter
combinations there
6. Look at your contact list
7. Any contact with an `m` should *not* have red text

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

Available in issue

### **After**

<img width="442" alt="SCR-20240319-kcss"
src="https://github.com/MetaMask/metamask-extension/assets/46655/f7993b0c-42f3-4dd2-841a-45487c34ea87">

<img width="437" alt="SCR-20240319-kcpv"
src="https://github.com/MetaMask/metamask-extension/assets/46655/3722cbed-c4b7-4d13-bab3-9a92a5ac01b7">

<img width="424" alt="SCR-20240319-kdao"
src="https://github.com/MetaMask/metamask-extension/assets/46655/b9d50a33-7a29-4b1d-801c-f85c82ce219b">


## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've clearly explained what problem this PR is solving and how it
is solved.
- [ ] I've linked related issues
- [ ] I've included manual testing steps
- [ ] I've included screenshots/recordings if applicable
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [ ] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [ ] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
  • Loading branch information
darkwing authored Mar 22, 2024
1 parent 7ddb0e0 commit 67475ea
Show file tree
Hide file tree
Showing 4 changed files with 114 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,90 @@ exports[`AddressListItem renders the address and label 1`] = `
class="mm-box mm-text address-list-item__label mm-text--body-md-medium mm-text--text-align-left mm-box--padding-0 mm-box--width-full mm-box--color-text-default"
data-testid="address-list-item-label"
>
<span>
metamask.eth
</p>
<div
class="mm-box mm-text mm-text--body-sm mm-text--ellipsis mm-box--color-text-alternative"
data-testid="address-list-item-address"
>
<div>
<div
aria-describedby="tippy-tooltip-1"
class=""
data-original-title="0x0c54FcCd2e384b4BB6f2E405Bf5Cbc15a017AaFb"
data-tooltipped=""
style="display: inline;"
tabindex="0"
>
0x0c54F...7AaFb
</div>
</div>
</div>
</div>
</button>
</div>
`;

exports[`AddressListItem uses a confusable when it should 1`] = `
<div>
<button
class="mm-box address-list-item mm-box--padding-4 mm-box--display-flex mm-box--align-items-center mm-box--width-full mm-box--background-color-transparent"
>
<div
class="mm-box mm-text mm-avatar-base mm-avatar-base--size-sm mm-avatar-account mm-text--body-sm mm-text--text-transform-uppercase mm-box--margin-inline-end-2 mm-box--display-flex mm-box--justify-content-center mm-box--align-items-center mm-box--color-text-default mm-box--background-color-background-alternative mm-box--rounded-full mm-box--border-color-transparent box--border-style-solid box--border-width-1"
>
<div
class="mm-avatar-account__jazzicon"
>
<div
style="border-radius: 50px; overflow: hidden; padding: 0px; margin: 0px; width: 24px; height: 24px; display: inline-block; background: rgb(3, 73, 94);"
>
<svg
height="24"
width="24"
x="0"
y="0"
>
<rect
fill="#F5D800"
height="24"
transform="translate(-1.6948137315966292 7.490045892231985) rotate(238.2 12 12)"
width="24"
x="0"
y="0"
/>
<rect
fill="#1888F2"
height="24"
transform="translate(2.6987555575750655 10.47254609666851) rotate(211.2 12 12)"
width="24"
x="0"
y="0"
/>
<rect
fill="#017E8E"
height="24"
transform="translate(4.1286145552783005 -17.188975454864387) rotate(404.9 12 12)"
width="24"
x="0"
y="0"
/>
</svg>
</div>
</div>
</div>
<div
class="mm-box mm-box--display-flex mm-box--flex-direction-column"
style="overflow: hidden;"
>
<p
class="mm-box mm-text address-list-item__label mm-text--body-md-medium mm-text--text-align-left mm-box--padding-0 mm-box--width-full mm-box--color-text-default"
data-testid="address-list-item-label"
>
<span>
<div
aria-describedby="tippy-tooltip-2"
class=""
data-original-title="'m' is similar to 'rn'."
data-tooltipped=""
style="display: inline;"
Expand All @@ -77,7 +157,7 @@ exports[`AddressListItem renders the address and label 1`] = `
a
<span>
<div
aria-describedby="tippy-tooltip-2"
aria-describedby="tippy-tooltip-3"
class=""
data-original-title="'m' is similar to 'rn'."
data-tooltipped=""
Expand Down Expand Up @@ -105,7 +185,7 @@ exports[`AddressListItem renders the address and label 1`] = `
>
<div>
<div
aria-describedby="tippy-tooltip-3"
aria-describedby="tippy-tooltip-4"
class=""
data-original-title="0x0c54FcCd2e384b4BB6f2E405Bf5Cbc15a017AaFb"
data-tooltipped=""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,12 @@ const SAMPLE_LABEL = 'metamask.eth';

const mockOnClick = jest.fn();

const render = () => {
const render = (label = '', useConfusable = false) => {
return renderWithProvider(
<AddressListItem
address={SAMPLE_ADDRESS}
label={SAMPLE_LABEL}
label={label || SAMPLE_LABEL}
useConfusable={useConfusable}
onClick={mockOnClick}
/>,
configureStore(mockState),
Expand All @@ -30,6 +31,20 @@ describe('AddressListItem', () => {
expect(getByText(shortenAddress(SAMPLE_ADDRESS))).toBeInTheDocument();
});

it('uses a confusable when it should', () => {
const { container } = render('metamask.eth', true);
expect(container).toMatchSnapshot();

expect(document.querySelector('.confusable__point')).toBeInTheDocument();
});

it('does not force red text when unnecessary', () => {
render('metamask.eth');
expect(
document.querySelector('.confusable__point'),
).not.toBeInTheDocument();
});

it('fires onClick when the item is clicked', () => {
render();
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
Expand Down
11 changes: 10 additions & 1 deletion ui/components/multichain/address-list-item/address-list-item.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,24 @@ import Tooltip from '../../ui/tooltip';
type AddressListItemProps = {
address: string;
label: string;
useConfusable?: boolean;
onClick: () => void;
};

export const AddressListItem = ({
address,
label,
useConfusable = false,
onClick,
}: AddressListItemProps) => {
const useBlockie = useSelector(getUseBlockie);
let displayName: string | React.ReactNode = shortenAddress(address);
if (label) {
displayName = label;
if (useConfusable) {
displayName = <Confusable input={label} />;
}
}

return (
<Box
Expand Down Expand Up @@ -74,7 +83,7 @@ export const AddressListItem = ({
className="address-list-item__label"
data-testid="address-list-item-label"
>
{label ? <Confusable input={label} /> : shortenAddress(address)}
{displayName}
</Text>
<Text
variant={TextVariant.bodySm}
Expand Down
5 changes: 4 additions & 1 deletion ui/components/multichain/pages/send/components/recipient.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import { SendPageAddressBook, SendPageRow, SendPageYourAccounts } from '.';
const CONTACTS_TAB_KEY = 'contacts';
const ACCOUNTS_TAB_KEY = 'accounts';

const ENS_RESOLUTION_TYPE = 'ENS resolution';

const renderExplicitAddress = (
address: string,
nickname: string,
Expand All @@ -36,6 +38,7 @@ const renderExplicitAddress = (
<AddressListItem
address={address}
label={nickname}
useConfusable={type === ENS_RESOLUTION_TYPE}
onClick={() => {
dispatch(
addHistoryEntry(
Expand Down Expand Up @@ -85,7 +88,7 @@ export const SendPageRecipient = () => {
contents = renderExplicitAddress(
domainResolution,
addressBookEntryName || userInput,
'ENS resolution',
ENS_RESOLUTION_TYPE,
dispatch,
);
} else {
Expand Down

0 comments on commit 67475ea

Please sign in to comment.