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

cli: do not set AutofillShowTypePredictions #12038

Merged
merged 2 commits into from
Feb 10, 2021

Conversation

connorjclark
Copy link
Collaborator

fixes #11985

@connorjclark connorjclark requested a review from a team as a code owner February 2, 2021 19:43
@connorjclark connorjclark requested review from adamraine and removed request for a team February 2, 2021 19:43
@google-cla google-cla bot added the cla: yes label Feb 2, 2021
@brendankenny
Copy link
Member

should all traces of it be removed?

like

const launchedChrome = await ChromeLauncher.launch({
chromeFlags: ['--enable-features=AutofillShowTypePredictions'],
});

and

// TODO(paulirish): restore when we stop using --enable-features=AutofillShowTypePredictions

and

// Autofill information that is injected into the snippet via AutofillShowTypePredictions
// TODO(paulirish): Don't clean title attribute from all elements if it's unnecessary
const autoFillIgnoreAttrs = ['autofill-information', 'autofill-prediction', 'title'];

and anything in those PRs that doesn't just match a AutofillShowTypePredictions search?

@connorjclark
Copy link
Collaborator Author

should all traces of it be removed?

No, I don't think so.

like

const launchedChrome = await ChromeLauncher.launch({
chromeFlags: ['--enable-features=AutofillShowTypePredictions'],
});

There's still a smoke test for this experimental audit.

// TODO(paulirish): restore when we stop using --enable-features=AutofillShowTypePredictions

same

// Autofill information that is injected into the snippet via AutofillShowTypePredictions
// TODO(paulirish): Don't clean title attribute from all elements if it's unnecessary
const autoFillIgnoreAttrs = ['autofill-information', 'autofill-prediction', 'title'];

and anything in those PRs that doesn't just match a AutofillShowTypePredictions search?

still supporting experimental usage, should anyone care..

We could remove more but this is the minimal to have any impact. Anything else might be churn (removed but brought back later).

@connorjclark
Copy link
Collaborator Author

connorjclark commented Feb 2, 2021

Ahhhh I assumed the chrome flags being set in the smoke test runner would apply to all, but that is just bundle.js. The normal run of smoke tests uses the regular chrome launching...

Add the flag to cli.js, or just remove/comment out form smoke tests?

EDIT: I commented out the smoke tests.

@connorjclark connorjclark merged commit 4bb4f4b into master Feb 10, 2021
@connorjclark connorjclark deleted the cli-flags-no-autocomplete-preds branch February 10, 2021 17:54
@brendankenny
Copy link
Member

still supporting experimental usage, should anyone care..

FWIW historically we've been in this exact position with an unsupported feature hidden by a flag that no one owns like two or three times now and it always ends up the same way.

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

Successfully merging this pull request may close these issues.

--enable-features=AutofillShowTypePredictions messes with accessibility results
4 participants