601209 - Improve interaction between @multiple and @list

Status: NEW

(Toolkit :: Form Manager, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Currently for something like:

<input type=email multiple list=foo>

if the user picks an item in the autocomplete list that replaces the entire value in the input. Similarly, when we filter which autocomplete suggestions to display, we use the entire value of the input, rather than just what has been entered since the last ','.

This largely makes @list useless for fields where @multiple is used. For example the bugzilla cc field couldn't transition to using these HTML5 features even if it wanted to.

To fix this I suggest we treat each comma-separated value separately when @multiple is set on <input type=email>. So when we filter out which suggestions to display, we should just look at string between the ',' before and after the cursor, and if the user picks a value, we should just replace that said string. (i.e. we should support the user editing part of the value that isn't at the end).

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, the spec already calls for this behavior:

http://www.whatwg.org/specs/web-apps/current-work/?slow-browser#attr-input-multiple

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually, I disagree with the blocking-. The combination of @multiple and @list is currently severely broken, to the point that you can't use @list on elements with @multiple.

Since this is broken functionality in a new feature, I really think we need to fix it before release.

Benjamin, if you disagree, please do speak up (or simply blocking- it again).

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: WIP Patch

This is actually working (with whatever the input type is) but I've some troubles with the pattern I choose and <input type='email' multiple> validity.

IOW, when the users select a value in the list, ", " is added so they can select multiple values without type them (only by selecting them in the list). However, "[e-mail], " isn't a valid list of email addresses. But if I don't add ", ", the user will have to add "," to have a popup showing the suggestions. I don't think that's what we want.

Actually, I realize this patch might be buggy if we try to autocomplete on a field with @multiple set. We want the value to be overwritten and it might not be the case.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

hmm.. tricky. We should look at how webpages do this right now.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Also, this patch seems to assume that the user is always editing the last email address?

Comment 6

[Mounir Lamouri (:mounir)]

When you select an email in GMail, it adds ", " after the email you've added. The same for keywords in bugzilla. I guess that might be common.

Do you think asking for "[e-mail], " being a valid email address list would be fine?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

One possible solution, though a somewhat scary one, would be to make .value not return the same value as is in the editor. Change the code at [1] to check if @multiple is set, see if the value ends with ", " (or even ",<whitespace>") and if so remove that from the returned value.

[1] http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLInputElement.cpp#1051

Comment 8

[Mounir Lamouri (:mounir)]

(In reply to comment #7)
> One possible solution, though a somewhat scary one, would be to make .value not
> return the same value as is in the editor. Change the code at [1] to check if
> @multiple is set, see if the value ends with ", " (or even ",<whitespace>") and
> if so remove that from the returned value.

FWIW, I would prefer accepting an email list with the last item being empty as valid.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

My concern with that is that we'd also have to rely on servers and javascript being able to deal with that too.

Comment 10

[Mounir Lamouri (:mounir)]

(In reply to comment #9)
> My concern with that is that we'd also have to rely on servers and javascript
> being able to deal with that too.

I guess we can assume servers already deal with that given that this seems to be how websites already do.
For javascript, the difference will be that after splitting the value, they will have to check that the last one can be the empty string, right?

Comment 11

[Mounir Lamouri (:mounir)]

(In reply to comment #5)
> Also, this patch seems to assume that the user is always editing the last email
> address?

I think we have at least two important UX issues here:

- Do we want to let the users select a value in the list if they come back to a previous entered value in the list. For example, the users type: "foo@, [e-mail],". Should we let them come back to "foo@" and have a UI showing the suggestions? Another solution would be to show something only when the caret is at the end of the field.
GMail doesn't let you do that (there are no suggestions) and bugzilla provides a UI for the last value (which is a buggy behavior).

- What about the autocomplete suggestions? They will be shown with the suggestions from @list but they will behave very differently. Do we want to show them? Maybe we can save every item and show them in the UI instead of showing the entire saved value?
I think autocomplete is disabled in all UI doing what we try to do.

Comment 12

[Alex Faaborg [:faaborg] (Firefox UX)]

(I'm momentarily handling Limi's bugmail while he is out of the office)

>- Do we want to let the users select a value in the list if they come back to a
>previous entered value in the list. For example, the users type: "foo@,
>[e-mail],". Should we let them come back to "foo@" and have a UI showing the
>suggestions? Another solution would be to show something only when the caret is
>at the end of the field.

Yeah, I think the user would expect to be able to change items in the center of the list and still get useful auto-complete feedback.  Several desktop email applications already work this way, so in addition to it being the logical behavior there is also some external consistency.

>GMail doesn't let you do that (there are no suggestions) and bugzilla provides
>a UI for the last value (which is a buggy behavior).

I would assume that they aren't avoiding doing this as much as they are unable to, or they haven't gotten around to it yet.

>- What about the autocomplete suggestions? They will be shown with the
>suggestions from @list but they will behave very differently. Do we want to
>show them? Maybe we can save every item and show them in the UI instead of
>showing the entire saved value?
>I think autocomplete is disabled in all UI doing what we try to do.

I might not understand the full context here.  If the user is editing the middle of the field after faa:

[e-mail], faa| , [e-mail]

The auto complete should be showing any suggestions that begin with that string (and ignoring the items beyond the separator characters.

Comment 13

[Mounir Lamouri (:mounir)]

(In reply to comment #12)
> >- What about the autocomplete suggestions? They will be shown with the
> >suggestions from @list but they will behave very differently. Do we want to
> >show them? Maybe we can save every item and show them in the UI instead of
> >showing the entire saved value?
> >I think autocomplete is disabled in all UI doing what we try to do.
> 
> I might not understand the full context here.  If the user is editing the
> middle of the field after faa:
> 
> [e-mail], faa| , [e-mail]
> 
> The auto complete should be showing any suggestions that begin with that string
> (and ignoring the items beyond the separator characters.

I wasn't clear I think. I was speaking of form history. The classic autocompletion.
If a user submit a form with a <input type='email' multiple name='foo' list='d'> with this value: "[e-mail], [e-mail], [e-mail]". Then the user comes back to the same form, "[e-mail], [e-mail], [e-mail]" will be suggested as a previous entered value but also all suggestions made by the web page. In other words, the user will see:

[e-mail], [e-mail], [e-mail]
-------------------------------------
[e-mail]
[e-mail]
[e-mail]

And if the first value is selected, it will replace the entire field's value but if another is selected, it will only replace the current item.
I'm wondering if that couldn't be a bit confusing?

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, I think that might be confusing. It might be simpler to always replace the current edited token. Or even better, replace the current edited token, but remove any email addresses that already exists in the list.

I think even better would be to be even more clever about the form-history suggestions. We can break up the values into individual addresses and autocomplete one at a time, possibly in some combination with autocompleting the whole list. But I think that's for a separate bug and a later release.

Comment 15

[Mounir Lamouri (:mounir)]

Attachment: Make searchString readonly

Comment 16

[Mounir Lamouri (:mounir)]

Attachment: Split results from form history when multiple is set

There is an issue here: we can't delete these values.
I know this is somewhat another issue but it would be easier to fix that in this bug, otherwise I will have to have a different behavior when multiple is set depending on the selected item in the list and these items are going to be filtered in a specific way when multiple will be set (only using the token the caret is on and not the entire value).

For the moment, one solution would be to add a new field in the form history database to know if the field had the multiple attribute. And, before saving, we would have to split the field's value. So, when getting we would only get clean values.

With bug 598092 and bug 562625 will be fixed, we could even get all saved values (regardless of the multiple attribute) for constrained types like email.

Comment 17

[Mounir Lamouri (:mounir)]

Attachment: Use the current token as a search string when multiple is set

This is more or less working. I've seen two issues:
1. It looks like we get some events before the caret is updated. For example, when moving the selection with the arrows, I get the previous position. But maybe I'm doing something wrong.
2. There is a crash when deleting the entire field's value but I think that might be fixed easily (I probably get a positive selection with an empty string value so when I try to move to the selection with a string iterator, it fails, I should check that.)

Comment 18

[Mounir Lamouri (:mounir)]

Consider all these patches as WIP.

Comment 19

[Mounir Lamouri (:mounir)]

According to Shawn and Gavin we should not push that in Firefox 4 (too late to change the autocomplete code, AFAIUI).
It would be nice to be sure this can be pushed for Firefox 4 given that this is going to take some time (and already did!).

FWIW, I think the basic autocomplete feature should be safe with this patch. This is only adding special cases when the multiple attribute is present.

Comment 20

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Make nsIAutoCompleteController searchString attribute readonly and introduce resetSearchString() method

Comment 21

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Add hasMultiple() to nsIAutoCompleteInput

Comment 22

[Mounir Lamouri (:mounir)]

Attachment: Part 3 - Use mSearchString in InputListAutoComplete instead of field's value

This shouldn't change anything for non-multiple fields but let nsAutoCompleteController to pass the correct search string to InputListAutocomplete.

Comment 23

[Mounir Lamouri (:mounir)]

Attachment: Part 4- Show a UI for each token when multiple is set

I see two issues in the behavior:
- when you move the caret using the arrow keys, it will show the popup with the suggestions because another search have to be done and there is a callback showing the popup when a search has been done but we can't know from this callback what code called the search. A way to fix that would be to only do a new search when we are sure we moved from a token to another. I will try to do that in another patch.
- when you select more than one token, the popup will show suggestions for the token under the beginning of the selection. This might be confusing and not showing suggestions might be better.

Comment 24

[Mounir Lamouri (:mounir)]

Attachment: Part 5 - Don't try to autocomplete when multiple is set

autocomplete with multiple set is hard for UX and other reasons (mSearchString is used for list autocomplete and form history autocomplete). I think the best is to disable that for the moment and enable it as soon as bug 607948 is fixed.

I don't think that's a big deal given that most website with multiple (and providing a UI) disable autocomplete for form history. The only issue is for fields where multiple is set without any UI or list provided. We can assume that's not common enough at the moment (officially multiple can be used on <input type='email'> and <input type='file'> for the moment so it will only affect <input type='email'> which isn't widely used).

Comment 25

[Mounir Lamouri (:mounir)]

Attachment: Part 5 - Don't try to autocomplete when multiple is set

Comment 26

[Mounir Lamouri (:mounir)]

Attachment: Part 6 - (WIP) - Run some event handlers in a runnable to make sure the caret is updated

When nsFormFillController event handlers are called, the caret isn't updated and when trying to compute what is the current selected token, we actually found the previous selected one. Running the event handlers is runnables make sure that the caret is updated before.

This is still work in progress: I need to clean the patch.

Comment 27

[Mounir Lamouri (:mounir)]

Part 1 to 3 are doing minor changes to interfaces. That's surely too late to have that in b7. Could that be moved to b8 or should we just drop this feature?

In addition, Shawn and Gavin were concerned about the changes in the autocomplete code so late in the beta process. I think these changes aren't sensitive for the current behavior. It's mostly refactoring the current code and adding new paths when multiple is set.

Comment 28

[Shawn Wilsher :sdwilsh]

(In reply to comment #27)
> Part 1 to 3 are doing minor changes to interfaces. That's surely too late to
> have that in b7. Could that be moved to b8 or should we just drop this feature?
It needs to happen for b7 or it won't be happening for 2.0.  b7 is our API freeze.

Comment 29

[Mounir Lamouri (:mounir)]

(In reply to comment #28)
> (In reply to comment #27)
> > Part 1 to 3 are doing minor changes to interfaces. That's surely too late to
> > have that in b7. Could that be moved to b8 or should we just drop this feature?
> It needs to happen for b7 or it won't be happening for 2.0.  b7 is our API
> freeze.

I don't think that can happen for b7. The last bug has just been fixed.
However, I thought some interface changes could happen after b7 but would need specific approval. Am I wrong?

If this feature stay as a blocker, we would have to change the API. Otherwise, adding new interfaces...

Comment 30

[Benjamin Smedberg]

I really don't think this should block, and that we probably ought to avoid the risk and the interface changes. We just won't have a complete implementation of HTML5 forms in this release, and we've already said that's ok. cc'ing jst since sicking disagreed in comment #2

Comment 31

[Mounir Lamouri (:mounir)]

Attachment: Part 4- Show a UI for each token when multiple is set

This is adding hasMultiple in autocomplete.xml

Comment 32

[Mounir Lamouri (:mounir)]

Attachment: Part 5 - Don't try to autocomplete when multiple is set

It was the wrong patch.

Comment 33

[Mounir Lamouri (:mounir)]

Attachment: Part 6 - Run some event handlers in a runnable to make sure the caret is updated

Some tests will have to be fixed because now the autocompletion is async.

Comment 34

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Introduce nsIAutoCompleteController_Mozilla_2_0_Branch with resetSearchString() method

The uuid of nsIAutoCompletController isn't updated anymore but an approval might still be required given that searchString setter is know trowing an error.

Comment 35

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Introduce nsIAutoCompleteInput_Mozilla_2_0_Branch with hasMultiple() method

I really doubt this change is a good idea:
diff --git a/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp b/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp
--- a/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp
+++ b/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp
@@ -132,17 +132,17 @@ nsAutoCompleteController::SetInput(nsIAu
     // Stop all searches in case they are async.
     StopSearch();
     ClearResults();
     if (mIsOpen)
       ClosePopup();
     mSearches.Clear();
   }
 
-  mInput = aInput;
+  mInput = do_QueryInterface(aInput);
 
   // Nothing more to do if the input was just being set to null.
   if (!aInput)
     return NS_OK;
 
   nsAutoString newValue;
   aInput->GetTextValue(newValue);
 
But otherwise, I will have to update the uuid of nsIAutoCompleteInput.
If I have autocomplete bindings implementing nsIAutoCompleteInput_Mozilla_2_0_Branch, it might be safe. Will check that.

Comment 36

[Mounir Lamouri (:mounir)]

Attachment: Part 3 - Use mSearchString in InputListAutoComplete instead of field's value

This didn't need uuid update and don't do that now.

Comment 37

[Mounir Lamouri (:mounir)]

Attachment: Part 4- Show a UI for each token when multiple is set

Comment 38

[Mounir Lamouri (:mounir)]

Attachment: Part 5 - Don't try to autocomplete when multiple is set

Comment 39

[Mounir Lamouri (:mounir)]

Attachment: Part 6 - Run some event handlers in a runnable to make sure the caret is updated

Comment 40

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - Introduce nsIAutoCompleteInput_Mozilla_2_0_Branch with hasMultiple() method

I had to change autocomplete widget to implement nsIAutoCompleteInput_Mozilla_2_0_Branch.

Comment 41

[Mounir Lamouri (:mounir)]

Attachment: Part 6 - Run some event handlers in a runnable to make sure the caret is updated

I forgot to refresh this patch before attaching.

Comment 42

[Shawn Wilsher :sdwilsh]

This doesn't block the release of Firefox 4.0 due to reasons in comment 30.

Comment 43

[Justin Dolske [:Dolske]]

Comment on attachment 491536 [details] [diff] [review]
Part 6 - Run some event handlers in a runnable to make sure the caret is updated

(Clearing reviews since we're not doing this for FF4, to get it out of my queue, but do reflag me for after FF4!)

Comment 44

[Matthew N. [:MattN]]

Mounir, do you still intend to work on this?

Comment 45

[Wayne Mery (:wsmwk)]

Mounir writes "I no longer monitor very closely my bugmails and I definitely do not intend to work on this anytime soon."