core(inputs): refactor form-elements gatherer #13662
Conversation
| @@ -72,8 +72,7 @@ const smokeTests = [ | |||
| errorsExpiredSsl, | |||
| errorsIframeExpiredSsl, | |||
| errorsInfiniteLoop, | |||
| // TODO: restore when --enable-features=AutofillShowTypePredictions is not needed. | |||
| // formsAutoComplete, | |||
| formsAutoComplete, | |||
There was a problem hiding this comment.
I've enabled the smoke test, but commented out the stuff that rely on the predictions being available.
| const failingFormsData = []; | ||
| const warnings = []; | ||
| let foundPrediction = false; | ||
| for (const form of forms) { |
There was a problem hiding this comment.
just whitespace changes, I didn't modify the logic.
| autocomplete: { | ||
| property: 'cc-exp-month', | ||
| attribute: 'cc-exp-month', | ||
| prediction: null, |
There was a problem hiding this comment.
all predictions are now null. lets add a comment.. somewhere in this file... to clarify that they'd be populated if the flag were used.
There was a problem hiding this comment.
There's a comment below that mentions the flag
There was a problem hiding this comment.
i saw that. but it's for the section you commented out. :)
i don't think the comment on line 467 is gonna help give context to whats happening on line 70..
so i was suggesting an additional comment.
| parentFormIndex: parentFormEl ? | ||
| [...formElToArtifact.keys()].indexOf(parentFormEl) : | ||
| undefined, | ||
| labelIndicies: [...inputEl.labels || []].map((labelEl) => { | ||
| return [...labelElToArtifact.keys()].indexOf(labelEl); | ||
| }), |
There was a problem hiding this comment.
Nit: For readability, can you compute these on their own lines
| @@ -0,0 +1,111 @@ | |||
| /** | |||
| * @license Copyright 2020 The Lighthouse Authors. All Rights Reserved. | |||
There was a problem hiding this comment.
| * @license Copyright 2020 The Lighthouse Authors. All Rights Reserved. | |
| * @license Copyright 2022 The Lighthouse Authors. All Rights Reserved. |
There was a problem hiding this comment.
it's a file rename. kinda.
There was a problem hiding this comment.
There are enough changes to consider this a new file imo, but I'll leave it up to you
types/artifacts.d.ts
Outdated
| interface FormElement { | ||
| id: string; | ||
| name: string; | ||
| autocomplete: string; | ||
| node: NodeDetails | null; |
There was a problem hiding this comment.
Is there a situation where this is null?
Refactors + renames the form-elements gatherer.
Instead of structuring the input elements inside form objects, place each input/label/form object into top-level properties and use indices to connect an input element with a form/labels.
It is still possible to query the artifact for "formless inputs" or "inputless labels" (if we ever wish to).