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

[TextField] Replace autocomplete off with nope #4053

Merged
merged 6 commits into from
Mar 12, 2021

Conversation

dfmcphee
Copy link
Contributor

WHY are these changes introduced?

Currently we use the attribute autocomplete="off" on the <TextField> component when passing in autoComplete={false}.

This causes issues in Chrome because of this bug: https://bugs.chromium.org/p/chromium/issues/detail?id=468153

WHAT is this pull request doing?

This changes the attribute to be autocomplete="nope" which looks strange but does work across browsers.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 10, 2021

🟡 This pull request modifies 9 files and might impact 9 other files. This is an average splash zone for a change, remember to tophat areas that could be affected.

Details:
All files potentially affected (total: 9)
📄 .github/workflows/ci.yml (total: 0)

Files potentially affected (total: 0)

📄 .nvmrc (total: 0)

Files potentially affected (total: 0)

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

📄 dev.yml (total: 0)

Files potentially affected (total: 0)

📄 package.json (total: 0)

Files potentially affected (total: 0)

📄 scripts/accessibility-check.js (total: 0)

Files potentially affected (total: 0)

🧩 src/components/TextField/TextField.tsx (total: 9)

Files potentially affected (total: 9)

🧩 src/components/TextField/tests/TextField.test.tsx (total: 0)

Files potentially affected (total: 0)

📄 yarn.lock (total: 0)

Files potentially affected (total: 0)

@dfmcphee dfmcphee requested review from alex-page and gcornhill March 10, 2021 15:50
Copy link
Contributor

@gcornhill gcornhill left a comment

Choose a reason for hiding this comment

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

Thank you!

@yourpalsonja
Copy link
Contributor

yourpalsonja commented Mar 10, 2021

Just wanted to surface this SO answer, it seems that Chromium "fixed" this and then broke it again.

We'll need to update all autocomplete="off" instances across web that are on form or input elements, too.

})
.join('\n');
};
const {storybookA11yTest} = require('@shopify/storybook-a11y-test');
Copy link
Member

Choose a reason for hiding this comment

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

Unblocked by Shopify/quilt#1764

@alex-page alex-page force-pushed the autocomplete-off-attribute branch from 4d0438f to a21ad68 Compare March 11, 2021 00:58
@dfmcphee dfmcphee force-pushed the autocomplete-off-attribute branch from a21ad68 to 8092455 Compare March 11, 2021 21:36
@utkarshsaxena93
Copy link

utkarshsaxena93 commented Apr 8, 2021

This change is breaking auto complete in the theme editor. Is there a recommended way to fix it? https://screenshot.click/21-04-yqnd4-01tux.mp4

autocomplete: off works correctly for me in Chrome Version 89.0.4389.114 (Official Build) (x86_64)

@dfmcphee
Copy link
Contributor Author

dfmcphee commented Apr 8, 2021

This change is breaking auto complete in the theme editor. Is there a recommended way to fix it? https://screenshot.click/21-04-yqnd4-01tux.mp4

autocomplete: off works correctly for me in Chrome Version 89.0.4389.114 (Official Build) (x86_64)

We saw one other case of this recently. Chrome seems to handle these in very strange ways where sometimes it can depend on the form around it and names and ids on the inputs as well. A work around for now is to use autoComplete="OFF" on the text field.

@utkarshsaxena93
Copy link

utkarshsaxena93 commented Apr 8, 2021

based on https://github.com/Shopify/polaris-react/pull/4053/files#diff-0476232e85245a025ae1fd330da91aae821feefc18fdd5f95d6f0a1e935c5292L520 change it doesn't look like a consumer can do that. Is there another way?

Can we update the code to allow the consumer to pick between off and nope ?

@alex-page
Copy link
Member

alex-page commented Apr 8, 2021

If you use OFF it doesn't hit that if statement. I can make an issue to explore this further though as there seems to be other patterns that are conflicting with how google handles autocomplete.

@utkarshsaxena93
Copy link

utkarshsaxena93 commented Apr 8, 2021

Yes please. it will be cumbersome to update all text fields in the theme editor (and other places where autocomplete may not have broken similar to how it did for the editor but we don't know yet)

@alex-page
Copy link
Member

I don't have time right now but have documented the problem #4107

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants