[Form] Check if form.elements is iterable before destructuring

[dbajpeyi]

Reviewer: @shakyShane
Asana: https://app.asana.com/0/1205996472158114/1207750680043135/f

Description

Some sites potentially can override the form.elements behavior, making it not iteratable, causing autofill breakage.

Steps to test

1. npm run build
2. Add to android's package.json and npm install,
3. Run android studio/other IDE to run emulator
4. Go to usa.experian.com/login/index
5. Autofill should work normally (saves new password, shows key icon, autofills and triggers auto-submit)

[shakyShane]

This would exclude those sites with an override form.elements though? perhaps it's still array-like in those cases and we could just do a manual thing like this (which would be faster too 😂)

const formElements = [];
for (let i = 0; i < this.form.elements.length; i++) {
  const el = this.form.elements[i];
  if (el.matches(selector)) {
    formElements.push(el);
  }
}

Also might be worth checking if Array.from() solves the site in question too - (you'll need to check the spec, as to what .from accepts)

either way, if this pops up again, we should probably just make a small helper, like array() that takes array-like things and packages them up into a regular array, so that map/filter etc work as expected.

You could also make it accept null/undefined too, returning an empty array. so this entire block might look like:

const formElements = array(this.form?.elements).filter(el => el.matches(selector))

[GioSensation]

Let’s not get ahead of ourselves 😅. There’s no evidence of this being a widespread issue. We’ve only found one site and their override is not iterable, if I remember correctly. Until we have reason to introduce the extra complexity I’d be inclined to keep things simple.

[dbajpeyi]

Sorry, I still wanted to do run some tests before opening for review, but my android setup is broken (getting help in android channel).

I would not generalize it too far yet with the custom array solution, I think your first suggestion makes sense in the else part.

[shakyShane]

Of course, I was just giving an alternative solution, as I said 'if this pops up again' :)

I didn't want to use 'Symbol.iterator in Object(this.form.elements)' just because it would exclude the form - is that the intention?

if so, you can discard my comments of course :)

[dbajpeyi]

Symbol.iterator in Object(this.form.elements) in this case will exclude the extra shadow checks and some nice goodies we get e.g safeguard against broken markups (based on the comments, haven't tested that), but it still falls back to querySelectorAll (in the else part) which ends up working for this specific site luckily:

android-form-elements.mp4

Of course it'd suck if we had shadow inputs here, but we're lucky :D

If we see more of these, we should figure out a way to abstract out the form.elements totally agree! But that's implementation dependent on what has been done to form.elements. Although I think it's a ridiculous thing to do to make a collection non-iterable. Not sure how often this would happen.

[shakyShane]

Thanks!