1388203 - Autofill misdetects extra fields on page

Status: NEW

(Toolkit :: Form Autofill, enhancement, P3)

Description

[Justin Dolske [:Dolske]]

I wanted to modify a hotel reservation on this page: https://gc.synxis.com/rez.aspx?hotel=62038&template=instasite&shell=hestia&start=searchres (via http://www.olympicinn.com/ --> modify/cancel reservation).

When I focus the first email box (under "Search by Confirmation Number") and press down-arrow, the dropdown shows and suggests it "Also autofills address, name, company, phone."

This is confusing, because there are no apparent inputs for anything than an email address and confirmation number.

MattN and I took a quick look to see if it was maybe finding something hidden on the page, this is the input Firefox thinks is for a phone number:

<input id="V115_C1_LocateCustomerCntrl_SecurityPoint3TextBox" class="BEInputText" name="V115$C1$LocateCustomerCntrl$SecurityPoint3TextBox" maxlength="40" type="text"> 

I'm not sure why that would be detected as such.

I'd expect Firefox to only be filling in the email address, and nothing more.

Comment 1

[Matthew N. [:MattN]]

Sean, can you take a look at this when you have a chance?

Marking as MVP for now.

Comment 2

[Sean Lee [:seanlee][:weilonge]]

Attachment: InvisibleForm

Hi MattN,

If "display: none" CSS rule in `#V115$C1$LocateCustomerCntrl$SecurityPoint1TextBox` is uncheck, the invisible form includes the extra fields is shown. That's what "Also autofill" mentioned.
These fields should not be considered 

I tried to detect visibility with `window.getComputedStyle(inputElement).display == "none"`, but "display" style of `inputElement` is actually "inline" rather than "none". "none" is actually for parent div, so we need to check its ancestors like what `findLabelElements` did.
(btw, window.getComputedStyle could be slow as well I guess.)

May I have your suggestion for the alternative solutions? Probably it is more accurate from Layout perspective?

Comment 3

[Luke Chang [:lchang] (inactive)]

(In reply to Sean Lee [:seanlee][:weilonge] from comment #2)
> I tried to detect visibility with
> `window.getComputedStyle(inputElement).display == "none"`, but "display"
> style of `inputElement` is actually "inline" rather than "none". "none" is
> actually for parent div, so we need to check its ancestors like what
> `findLabelElements` did.
> (btw, window.getComputedStyle could be slow as well I guess.)

I used to detect the visibility of an element by testing "offsetWidth==0" and "offsetHeight==0". It can deal with most cases. Not sure if it works on inputs as well though.

Comment 4

[Sean Lee [:seanlee][:weilonge]]

(In reply to Sean Lee [:seanlee][:weilonge] from comment #2)
> If "display: none" CSS rule in
> `#V115$C1$LocateCustomerCntrl$SecurityPoint1TextBox` is uncheck, the
The form ID should be `#V115_C1_LocateCustomerCntrl_ForgotPasswordPanel`. Sorry for confusing.

Comment 5

[Matthew N. [:MattN]]

I would first like to understand why the heuristics are detecting those fields as address fields. Could you explain which parts of the regexes match for the irrelevant fields. I think we should look at that first since taking visibility into account is going to be expensive and potentially error-prone.

Comment 6

[Sean Lee [:seanlee][:weilonge]]

(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #5)
> I would first like to understand why the heuristics are detecting those
> fields as address fields. Could you explain which parts of the regexes match
> for the irrelevant fields. I think we should look at that first since taking
> visibility into account is going to be expensive and potentially error-prone.

The first issue is that the heuristics won't skip the invisible elements.
These extra fields are all hidden by setting "display: none" on their parent `#V115_C1_LocateCustomerCntrl_ForgotPasswordPanel`. 

The labels of these extra fields match to these regexps (e.g. Last Name, City, Phone, see attachment 8895110 [details])
The second issue is that "Confirmation Number" matches to de-DE rule "firma" of "organization" regexp[1].

BTW, offsetWidth and offsetHeight can be used to detect the hidden elements. (Thanks, Luke!)
I added this snippet at the beginning of FormAutofillUtils.isFieldEligibleForAutofill():
```JS
if (element.offsetWidth == 0 || element.offsetHeight == 0) {
  return false;
}
```
It fixed the mismatch hidden element issue, but the related xpcshell test are all broken. (e.g. test_isFieldEligibleForAutofill.js and other heuristics test.)

[1] http://searchfox.org/mozilla-central/rev/0f16d437cce97733c6678d29982a6bcad49f817b/browser/extensions/formautofill/content/heuristicsRegexp.js#53

Comment 7

[Luke Chang [:lchang] (inactive)]

(In reply to Sean Lee [:seanlee][:weilonge] from comment #6)
> BTW, offsetWidth and offsetHeight can be used to detect the hidden elements.

Glad to hear it works. However, I'm quite afraid it affects the performance a lot as it does cause a sync reflow. Could you try if "getBoundsWithoutFlushing" works, too?

Comment 8

[Sean Lee [:seanlee][:weilonge]]

Hey MattN, could you take a look the comment 6? I wish I've anwsered your concern. Thank you.

Comment 9

[Matthew N. [:MattN]]

(In reply to Sean Lee [:seanlee][:weilonge] from comment #6)
> (In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking
> you) from comment #5)
> > I would first like to understand why the heuristics are detecting those
> > fields as address fields. Could you explain which parts of the regexes match
> > for the irrelevant fields. I think we should look at that first since taking
> > visibility into account is going to be expensive and potentially error-prone.
> 
> The first issue is that the heuristics won't skip the invisible elements.
> These extra fields are all hidden by setting "display: none" on their parent
> `#V115_C1_LocateCustomerCntrl_ForgotPasswordPanel`. 

Well we don't take visibility into account for password manager either since it's slow to calculate and isn't always desired.

It seems like it could also be solved by splitting the forms into separate sections.

Does Chromium have the same bug?

> The labels of these extra fields match to these regexps (e.g. Last Name,
> City, Phone, see attachment 8895110 [details])
> The second issue is that "Confirmation Number" matches to de-DE rule "firma"
> of "organization" regexp[1].

Hmm… confirmation numbers are fairly common for reservations so it would be good to understand if our label matching is too aggressive compared to Chromium in this case.

> It fixed the mismatch hidden element issue, but the related xpcshell test
> are all broken. (e.g. test_isFieldEligibleForAutofill.js and other
> heuristics test.)

Right, that makes sense since they're not displayed so I would like to avoid handling visibility unless we really need to.

If Chromium has this same problem then I don't think it should be a blocker.

Comment 10

[Sean Lee [:seanlee][:weilonge]]

(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #9)
> 
> Well we don't take visibility into account for password manager either since
> it's slow to calculate and isn't always desired.
> 
> It seems like it could also be solved by splitting the forms into separate
> sections.
> 
> Does Chromium have the same bug?
> 

Chromium doesn't fill the invisible elements. (BTW, our phishing warning makes this issue more obvious.)

> Hmm… confirmation numbers are fairly common for reservations so it would be
> good to understand if our label matching is too aggressive compared to
> Chromium in this case.
> 
I found Chromium has the same issue to determine the label "Reservation or Itinerary Confirmation" as "COMPANY_NAME" aka "organization".

> Right, that makes sense since they're not displayed so I would like to avoid
> handling visibility unless we really need to.
> 
> If Chromium has this same problem then I don't think it should be a blocker.

I think the "confirmation numbers" issue can not be a blocker but "visibility" issue.
IMHO, I guess Chromium does not use a strong "visibility" algorithm to detect the elements are visible or not. Otherwise, they won't have the phishing fields issue. However, this assumption still needs to be confirmed by reading Chromium's code.

Comment 11

[Luke Chang [:lchang] (inactive)]

Make this bug not block address autofill MVP.