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

RESOLVED FIXED in mozilla11

Status

()

Toolkit
Form Manager
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla11
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

8.61 KB, patch
Dolske
: review+
zpao
: review+
Away for a while
: feedback+
mounir
: checkin+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
Created attachment 567466 [details] [diff] [review]
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.
Attachment #567466 - Flags: review?(dolske)
Attachment #567466 - Flags: feedback?(ehsan)
(Assignee)

Comment 1

6 years ago
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 :)
Attachment #567466 - Flags: review?(bzbarsky)
(Assignee)

Updated

6 years ago
Assignee: nobody → mounir
Whiteboard: [needs review]
Comment on attachment 567466 [details] [diff] [review]
Patch

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

Comment 3

6 years ago
(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.
Status: NEW → ASSIGNED
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).
Attachment #567466 - Flags: review?(bzbarsky) → review+

Comment 5

6 years ago
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.
Attachment #567466 - Flags: feedback?(ehsan) → feedback+
(Assignee)

Comment 6

6 years ago
(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|.
(Assignee)

Comment 7

6 years ago
Comment on attachment 567466 [details] [diff] [review]
Patch

Paul, feel free to steal Dolske's review if you can do it before him.
Attachment #567466 - Flags: review?(paul)
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.
Attachment #567466 - Flags: review?(paul) → review+
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.
Attachment #567466 - Flags: review?(dolske) → review+
(Assignee)

Updated

6 years ago
Flags: in-testsuite-
Whiteboard: [needs review]
Target Milestone: --- → mozilla11
(Assignee)

Updated

6 years ago
Attachment #567466 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/ba6363fc6448
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Depends on: 704521
You need to log in before you can comment on or make changes to this bug.