Improve interaction between @multiple and @list

NEW
Unassigned

Status

()

8 years ago
6 months ago

People

(Reporter: sicking, Unassigned)

Tracking

(Depends on: 2 bugs)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 .x+, status2.0 wontfix)

Details

Attachments

(6 attachments, 16 obsolete attachments)

6.87 KB, patch
Details | Diff | Splinter Review
2.27 KB, patch
Details | Diff | Splinter Review
14.56 KB, patch
Details | Diff | Splinter Review
1.49 KB, patch
Details | Diff | Splinter Review
7.34 KB, patch
Details | Diff | Splinter Review
7.73 KB, patch
Details | Diff | Splinter Review
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).
Component: DOM → Autocomplete
OS: Mac OS X → All
Product: Core → Toolkit
QA Contact: general → autocomplete
Hardware: x86 → All
blocking2.0: --- → ?

Updated

8 years ago
blocking2.0: ? → -
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).
blocking2.0: - → betaN+
Summary: Improvie interaction between @multiple and @list for <input type=email> → Improve interaction between @multiple and @list for <input type=email>
Summary: Improve interaction between @multiple and @list for <input type=email> → Improve interaction between @multiple and @list
Created attachment 484374 [details] [diff] [review]
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, "foo@foo.com, " 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.
hmm.. tricky. We should look at how webpages do this right now.
Also, this patch seems to assume that the user is always editing the last email address?
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 "foo@foo.com, " being a valid email address list would be fine?
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/﷒0
(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.
My concern with that is that we'd also have to rely on servers and javascript being able to deal with that too.
(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?
Depends on: 606170
Depends on: 606196
(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@, foo@bar.com,". 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.
(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@,
>foo@bar.com,". 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:

mounir.lamouri@gmail.com, faa| , jonas@sicking.cc

The auto complete should be showing any suggestions that begin with that string (and ignoring the items beyond the separator characters.
(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:
> 
> mounir.lamouri@gmail.com, faa| , jonas@sicking.cc
> 
> 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: "foo@foo.com, foo@bar.com, bar@bar.com". Then the user comes back to the same form, "foo@foo.com, foo@bar.com, bar@bar.com" will be suggested as a previous entered value but also all suggestions made by the web page. In other words, the user will see:

foo@foo.com, foo@bar.com, bar@bar.com
-------------------------------------
foo@foo.com
something@foo.com
myemail@foo.com

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?
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.
Created attachment 486389 [details] [diff] [review]
Make searchString readonly
Created attachment 486390 [details] [diff] [review]
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.
Created attachment 486392 [details] [diff] [review]
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.)
Attachment #484374 - Attachment is obsolete: true
Consider all these patches as WIP.
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.
Depends on: 607948
Created attachment 487550 [details] [diff] [review]
Part 1 - Make nsIAutoCompleteController searchString attribute readonly and introduce resetSearchString() method
Attachment #486389 - Attachment is obsolete: true
Attachment #487550 - Flags: review?(dolske)
Created attachment 487551 [details] [diff] [review]
Part 2 - Add hasMultiple() to nsIAutoCompleteInput
Attachment #487551 - Flags: review?(dolske)
Attachment #487550 - Attachment is patch: true
Attachment #487550 - Attachment mime type: application/octet-stream → text/plain
Created attachment 487552 [details] [diff] [review]
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.
Attachment #486392 - Attachment is obsolete: true
Attachment #487552 - Flags: review?(dolske)
Created attachment 487553 [details] [diff] [review]
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.
Attachment #487553 - Flags: review?(dolske)
Created attachment 487556 [details] [diff] [review]
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).
Attachment #487556 - Flags: review?(dolske)
Created attachment 487558 [details] [diff] [review]
Part 5 - Don't try to autocomplete when multiple is set
Attachment #487558 - Flags: review?(dolske)
Attachment #487556 - Attachment is obsolete: true
Attachment #487556 - Flags: review?(dolske)
Created attachment 487559 [details] [diff] [review]
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.
Attachment #486390 - Attachment is obsolete: true
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.
Status: NEW → ASSIGNED
(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.
(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

8 years ago
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
Created attachment 487895 [details] [diff] [review]
Part 4- Show a UI for each token when multiple is set

This is adding hasMultiple in autocomplete.xml
Attachment #487553 - Attachment is obsolete: true
Attachment #487895 - Flags: review?(dolske)
Attachment #487553 - Flags: review?(dolske)
Created attachment 487897 [details] [diff] [review]
Part 5 - Don't try to autocomplete when multiple is set

It was the wrong patch.
Attachment #487558 - Attachment is obsolete: true
Attachment #487897 - Flags: review?(dolske)
Attachment #487558 - Flags: review?(dolske)
Created attachment 487898 [details] [diff] [review]
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.
Attachment #487559 - Attachment is obsolete: true
Attachment #487898 - Flags: review?(dolske)
Created attachment 491359 [details] [diff] [review]
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.
Attachment #487550 - Attachment is obsolete: true
Attachment #491359 - Flags: review?(dolske)
Attachment #487550 - Flags: review?(dolske)
Created attachment 491363 [details] [diff] [review]
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.
Attachment #487551 - Attachment is obsolete: true
Attachment #491363 - Flags: review?(dolske)
Attachment #487551 - Flags: review?(dolske)
Created attachment 491364 [details] [diff] [review]
Part 3 - Use mSearchString in InputListAutoComplete instead of field's value

This didn't need uuid update and don't do that now.
Attachment #487552 - Attachment is obsolete: true
Attachment #491364 - Flags: review?(dolske)
Attachment #487552 - Flags: review?(dolske)
Created attachment 491365 [details] [diff] [review]
Part 4- Show a UI for each token when multiple is set
Attachment #487895 - Attachment is obsolete: true
Attachment #491365 - Flags: review?(dolske)
Attachment #487895 - Flags: review?(dolske)
Created attachment 491367 [details] [diff] [review]
Part 5 - Don't try to autocomplete when multiple is set
Attachment #487897 - Attachment is obsolete: true
Attachment #491367 - Flags: review?(dolske)
Attachment #487897 - Flags: review?(dolske)
Created attachment 491368 [details] [diff] [review]
Part 6 - Run some event handlers in a runnable to make sure the caret is updated
Attachment #487898 - Attachment is obsolete: true
Attachment #491368 - Flags: review?(dolske)
Attachment #487898 - Flags: review?(dolske)
Whiteboard: [needs review]
Created attachment 491535 [details] [diff] [review]
Part 2 - Introduce nsIAutoCompleteInput_Mozilla_2_0_Branch with hasMultiple() method

I had to change autocomplete widget to implement nsIAutoCompleteInput_Mozilla_2_0_Branch.
Attachment #491363 - Attachment is obsolete: true
Attachment #491535 - Flags: review?(dolske)
Attachment #491363 - Flags: review?(dolske)
Created attachment 491536 [details] [diff] [review]
Part 6 - Run some event handlers in a runnable to make sure the caret is updated

I forgot to refresh this patch before attaching.
Attachment #491368 - Attachment is obsolete: true
Attachment #491536 - Flags: review?(dolske)
Attachment #491368 - Flags: review?(dolske)
This doesn't block the release of Firefox 4.0 due to reasons in comment 30.
blocking2.0: betaN+ → -

Updated

8 years ago
blocking2.0: - → .x
status2.0: --- → wontfix
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!)
Attachment #491536 - Flags: review?(dolske)
Attachment #491535 - Flags: review?(dolske)
Attachment #491367 - Flags: review?(dolske)
Attachment #491365 - Flags: review?(dolske)
Attachment #491364 - Flags: review?(dolske)
Attachment #491359 - Flags: review?(dolske)
Whiteboard: [needs review]
Mounir, do you still intend to work on this?
Flags: needinfo?(mounir)

Comment 45

4 years ago
Mounir writes "I no longer monitor very closely my bugmails and I definitely do not intend to work on this anytime soon."
Assignee: mounir → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(mounir)

Updated

6 months ago
Component: Autocomplete → Form Manager
You need to log in before you can comment on or make changes to this bug.