695014 - nsFormFillController should not watch input elements which have no list attribute and disabled autocomplete

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch

As mentioned by bz in bug 598819 comment 2, the autocomplete code is run even if autocomplete is disabled. It has been done very likely because of datalist/list support (at least the last related change come from that, I didn't check more deeply).

This patch check @autocomplete and @list to watch an input element.
The only downside I see is that changing @autocomplete and @list while the element is focused will have no impact on the fact that the list will be displayed or not. Though, re-focusing will make the change happen.
Currently, going from autocomplete disabled to enabled works as expected but not the opposite so it's likely not a use case we care a lot about.

We could open a follow-up to listen to attribute changes on the element, thus being able to change dynamically our autocomplete behavior.

Comment 1

[Mounir Lamouri (:mounir)]

Comment on attachment 567466 [details] [diff] [review]
Patch

Boris, could you review the nsContentUtils change? A general feedback on the patch would also be appreciated :)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 567466 [details] [diff] [review]
Patch

So autocomplete="foopy" is equivalent to "off"?  Where does the code checking that live?

Comment 3

[Mounir Lamouri (:mounir)]

(In reply to Boris Zbarsky (:bz) from comment #2)
> Comment on attachment 567466 [details] [diff] [review] [diff] [details] [review]
> Patch
> 
> So autocomplete="foopy" is equivalent to "off"?  Where does the code
> checking that live?

autocomplete IDL attribute is an enum. For input, three values are allowed ("", "on" and "off) and the default value is "". For form, two values are allowed ("on" and "off") and the default value is "on".

My code is directly checking the IDL attribute so only those values can be returned.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 567466 [details] [diff] [review]
Patch

Ah, right.  For some reason I read that as GetAttr.  ;)

r=me, but it might be worth documenting the allowed enum values here (and perhaps asserting if an unexpected value is seen).

Comment 5

[(no longer active)]

Comment on attachment 567466 [details] [diff] [review]
Patch

Review of attachment 567466 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsContentUtils.cpp
@@ +647,5 @@
>    return rv;
>  }
>  
> +bool
> +nsContentUtils::IsAutocompleteEnabled(nsIDOMHTMLInputElement* aInput)

Does the autocomplete attribute apply to all types of input elements?  If not, this method needs to check the type as well.

Comment 6

[Mounir Lamouri (:mounir)]

(In reply to Ehsan Akhgari [:ehsan] from comment #5)
> Does the autocomplete attribute apply to all types of input elements?  If
> not, this method needs to check the type as well.

Except buttons, checkbox, radiobox, file input and hidden input, autocomplete apply to all all types. Here, the real check is whether the input element is a text field or not and this is done with |IsSingleLineTextControl|.

Comment 7

[Mounir Lamouri (:mounir)]

Comment on attachment 567466 [details] [diff] [review]
Patch

Paul, feel free to steal Dolske's review if you can do it before him.

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 567466 [details] [diff] [review]
Patch

Review of attachment 567466 [details] [diff] [review]:
-----------------------------------------------------------------

I'd feel better if Dolske gave this a quick look over too since I haven't actually touched nsFormFillController, but it seems fine. I wish we had more comments throughout... and tests :/

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #0)
> We could open a follow-up to listen to attribute changes on the element,
> thus being able to change dynamically our autocomplete behavior.

I think it would be good to have it on file, even if it's not a priority. I would really like to see us follow through on fixing these small details.

Comment 9

[Justin Dolske [:Dolske]]

Comment on attachment 567466 [details] [diff] [review]
Patch

Review of attachment 567466 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me, although please add the mFocusedInput check noted below just to be extra safe. Also, ehsan's prior comment about checking the input type in IsAutocompleteEnabled() seems like a good idea to me too!

::: toolkit/components/satchel/nsFormFillController.cpp
@@ -572,5 @@
>                                           mFocusedInput,
>                                           getter_AddRefs(result));
>    } else {
>      nsCOMPtr<nsIAutoCompleteResult> formHistoryResult;
> -    if (!IsInputAutoCompleteOff()) {

The one interesting thing this function did previously but not with this patch is to ensure mFocusedInput != null here. This code has been a bit sloppy about that, I've no idea if it can actually be null here or if that's handled safely.

Instead of investing time to check, just wrap this with an |if (mFocusedInput)| check.

@@ +758,5 @@
>  
>    bool isReadOnly = false;
>    input->GetReadOnly(&isReadOnly);
>  
> +  bool autocomplete = nsContentUtils::IsAutocompleteEnabled(input);

FWIW, I'd like to convert this whole file to JS at some point, so will end up not using the C++ function you moved. Does anything else want to use that? Doesn't matter either way to me, though. We'll deal with it whenever it's ported.

Comment 10

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/ba6363fc6448