Bug 32261 - Add ability to specify suggested autocomplete value in HTMLInputElement
Summary: Add ability to specify suggested autocomplete value in HTMLInputElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: GoogleBug
Depends on:
Blocks: 32533
  Show dependency treegraph
 
Reported: 2009-12-07 18:14 PST by Zelidrag Hornung
Modified: 2009-12-16 11:07 PST (History)
10 users (show)

See Also:


Attachments
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement and related RenderTextControlSingleLine classes. (14.69 KB, patch)
2009-12-09 16:43 PST, Zelidrag Hornung
no flags Details | Formatted Diff | Diff
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement / RenderTextControlSingleLine (15.24 KB, patch)
2009-12-09 16:50 PST, Zelidrag Hornung
darin: review-
Details | Formatted Diff | Diff
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement / RenderTextControlSingleLine (5.55 KB, patch)
2009-12-14 21:15 PST, Zelidrag Hornung
no flags Details | Formatted Diff | Diff
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement / RenderTextControlSingleLine (5.09 KB, patch)
2009-12-15 23:57 PST, Zelidrag Hornung
abarth: review-
Details | Formatted Diff | Diff
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement / RenderTextControlSingleLine (5.10 KB, patch)
2009-12-16 00:04 PST, Zelidrag Hornung
darin: review-
Details | Formatted Diff | Diff
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement / RenderTextControlSingleLine (deleted)
2009-12-16 09:42 PST, Zelidrag Hornung
darin: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement / RenderTextControlSingleLine (deleted)
2009-12-16 10:27 PST, Zelidrag Hornung
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zelidrag Hornung 2009-12-07 18:14:33 PST
Currently, autocomplete has no way of just visually changing a value of HTMLInputElement without exposing this value to other parts of the system (i.e. Javascript).
  
I suggest changing HTMLInputElement by adding a new field, suggested value, that could be used for rendering purposes if this element is in autocomplete suggestion mode. This suggestion value might become element's value only if user accepts the autocomplete suggestion (i.e. pressing enter on the menu item or focusing to another field).
Comment 1 Zelidrag Hornung 2009-12-09 16:43:10 PST
Created attachment 44580 [details]
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement and related RenderTextControlSingleLine classes.
Comment 2 WebKit Review Bot 2009-12-09 16:46:54 PST
style-queue ran check-webkit-style on attachment 44580 [details] without any errors.
Comment 3 Zelidrag Hornung 2009-12-09 16:50:21 PST
Created attachment 44581 [details]
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement / RenderTextControlSingleLine
Comment 4 WebKit Review Bot 2009-12-09 16:52:16 PST
style-queue ran check-webkit-style on attachment 44581 [details] without any errors.
Comment 5 Ojan Vafai 2009-12-09 17:18:56 PST
Have you run this by chrome's UI leads? Why do we want this behavior of not
exposing the changed value to JavaScript?
Comment 6 Glen Murphy 2009-12-09 17:25:08 PST
Zel ran it by us - we don't want the value exposed to the page so that a page can't figure out what terms you're using elsewhere.

e.g. I go to a site and do a search for "sapiens" - I don't want the owner of the page  to see the form value first fill out with "sexy undies" as I type "s", then "salacious books" as I type "sa", etc. The values for autocomplete come from inputs across the browser, not just searches on that page.
Comment 7 Oliver Hunt 2009-12-09 19:15:26 PST
Can this be removed from the review queue until people have decided that they actually want it -- also exposing this to the dom means it should be standardised (and so brought up on the appropriate w3 mailing list).
Comment 8 Ben Goodger 2009-12-09 19:22:18 PST
Chrome UI leads would appreciate this behavior for the reason Glen describes.
Comment 9 Oliver Hunt 2009-12-09 19:34:52 PST
(In reply to comment #8)
> Chrome UI leads would appreciate this behavior for the reason Glen describes.

This sounds like you're wanting to add API to allow a site to suggest values to input, but i haven't seen anything about this on the mailing lists i'm a member of.
Comment 10 Dimitri Glazkov (Google) 2009-12-09 19:50:33 PST
There are no additional DOM APIs. Please see https://bugs.webkit.org/show_bug.cgi?id=32255 for more discussion.
Comment 11 Zelidrag Hornung 2009-12-09 20:59:20 PST
First, there is not about the site being able to suggest the value - it's about hiding the autocomplete suggestions from the site.  There are no DOM API changes here. The newly added methods should be accessible from webkit's internals only.  Please check the repro steps in the following bug to really see the nature of what we are really trying to prevent there:

https://bugs.webkit.org/show_bug.cgi?id=32255
Comment 12 Zelidrag Hornung 2009-12-09 21:14:44 PST
..adding Darin on the thread since he is familiar with the original issue behind this bug (https://bugs.webkit.org/show_bug.cgi?id=32255)
Comment 13 Ojan Vafai 2009-12-09 22:04:29 PST
Zelidrag, I think some of the confusion here is that the title of the bug and your initial description of it makes it sound like this should be exposing something to javascript.

Now that I understand that it's not actually exposing a new property to JS, I'm convinced by Glen's argument that this is necessary as a user-privacy feature.
Comment 14 John Sullivan 2009-12-10 07:17:39 PST
There are at least a couple of problems with this approach.

From the user-privacy standpoint, if it's unacceptable to let the web page see the autocompleted value while the user is typing in the field, then allowing the web page to see the value when focus is lost from the field might be equally unacceptable. The user could hit Tab or click elsewhere before noticing the autocompleted value, and now it is exposed to the page. Worse, the page could focus another element via JavaScript and have the value exposed without any user action.

From the UI standpoint, it seems problematic that the page might behave differently though look exactly the same depending on whether the text in the focused field was typed or auto-completed.
Comment 15 Zelidrag Hornung 2009-12-10 10:12:23 PST
From John's comments, there are two issues that need to be answered here:

a) Accepting autocomplete suggestion on focus lost. The current implementation
proposal in the attachment will (for Chromium only) indeed turn suggestion into
the input element's value on focus lost. I am open to suggestion about what
makes more sense than that, but IMO this seemed like the most natural behavior
from UX perspective. Alternatively, we could potentially require users to press
enter to explicitly accept the suggestion or identify focus lost cases where
that transition won't happen. It is also true that the site could trigger focus
change and get the suggestion turned into the element's value, but the site's
JS code has no way of knowing if a particular filed is in the suggestion mode
at all. So, it would really be a crap shooting for a site to get such value. We
could still stem this issue by not transferring the value at least in a case
when JS call caused the focus lost.

b) UI inconsistencies. The field will actually look different, to the certain
degree, in suggest mode case - the added/suggested part of the value will
actually have a text selection there. Once the value is accepted, the selection
goes away. We could further mitigate page look/behaving inconsistency here by
modifying rendering. That will most likely be browser specific. For Chromium,
we could simply use a different coloring scheme, similar what we do for
autofill.
Comment 16 Darin Adler 2009-12-11 14:27:51 PST
Comment on attachment 44581 [details]
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement / RenderTextControlSingleLine

How about using a suggested value of null to mean suggest and thus eliminating the separate enterSuggestMode function and m_suggestMode?

I don't think you need a leaveSuggestMode function. The client can call setValue(suggestedValue) and then setSuggestedValue(String()) or just setSuggestedValue(String()). If you think setSuggestedValue(String()) is too ugly you could make a convenience function member called clearSuggestedValue.

Ideally you'd factor things so more logic was shared with setValue. Since there is no regression test for this that's particularly important. It's likely people would forget to update setSuggestedValue as they change setValue.

The name suggestMode is not a good name for a function or data member that's a boolean. A good name would be something that completes the phrase "element <xxx>", and "element suggest mode" does not work for that. I think hasSuggestion or hasSuggestedValue would be good. Or you could omit this function and say !suggestedValue().isNull() at the call sites.

We frown on the use of booleans for function arguments if the caller will be passing a constant to a function like leaveSuggestMode. Instead we use enums. See ReferrerPolicy, for example. But I suggest just removing the function entirely.

The PopupMenu changes should be in a separate patch. Even if for you they are part of the same task, they are not directly related.
Comment 17 Zelidrag Hornung 2009-12-14 21:15:23 PST
Created attachment 44840 [details]
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement / RenderTextControlSingleLine

Based on Darin's comments of my previous patch, I have 1) removed enter/leaveSuggestMode functions, 2) reset suggested value in setValue(), and 3) moved PopupMenu changes into a separate patch.
Comment 18 WebKit Review Bot 2009-12-14 21:18:21 PST
style-queue ran check-webkit-style on attachment 44840 [details] without any errors.
Comment 19 Darin Adler 2009-12-15 14:24:43 PST
Comment on attachment 44840 [details]
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement / RenderTextControlSingleLine

> +    String suggestedValue() const { return m_suggestedValue; }

I know that the value() function doesn't do it this way, but you can save a tiny bit of reference count churn by making the return type be const String& instead of String.

I believe this will break the WML build, because you're adding a new virtual function that WMLInputElement doesn't implement.

I really wish there was a way to expose and test this new feature, but since the whole idea is to hide it from JavaScript, I can't think of any better way do things.

r=me
Comment 20 Zelidrag Hornung 2009-12-15 23:57:12 PST
Created attachment 44947 [details]
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement / RenderTextControlSingleLine

This addressed two comments from Darin about the previous CL:

1. suggestedValue() getter now defined as:
const String& suggestedValue() const;

2. suggestedValue() getter is added to WMLInputElement class.
Comment 21 WebKit Review Bot 2009-12-15 23:58:16 PST
Attachment 44947 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/wml/WMLInputElement.cpp:128:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1
Comment 22 Adam Barth 2009-12-16 00:03:57 PST
Comment on attachment 44947 [details]
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement / RenderTextControlSingleLine

No ChangeLog.  Please use four space indent instead of 2.
Comment 23 Zelidrag Hornung 2009-12-16 00:04:06 PST
Created attachment 44950 [details]
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement / RenderTextControlSingleLine

This addressed two comments from Darin about the previous CL:

1. suggestedValue() getter now defined as:
const String& suggestedValue() const;

2. suggestedValue() getter is added to WMLInputElement class.
Comment 24 Adam Barth 2009-12-16 00:04:27 PST
Comment on attachment 44840 [details]
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement / RenderTextControlSingleLine

Clearing r+ flag from obsolete patch.
Comment 25 WebKit Review Bot 2009-12-16 00:09:18 PST
style-queue ran check-webkit-style on attachment 44950 [details] without any errors.
Comment 26 Darin Adler 2009-12-16 09:24:30 PST
Comment on attachment 44950 [details]
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement / RenderTextControlSingleLine

Patch needs a ChangeLog entry. Setting review- because there is no change log in this patch.
Comment 27 Zelidrag Hornung 2009-12-16 09:42:38 PST
Created attachment 44985 [details]
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement / RenderTextControlSingleLine

This addressed two comments from Darin about the previous CLs:
1. suggestedValue() getter now defined as:
const String& suggestedValue() const;
2. suggestedValue() getter is added to WMLInputElement class.
3. Added mysteriously missing ChangeLog file.
Comment 28 WebKit Review Bot 2009-12-16 09:46:32 PST
style-queue ran check-webkit-style on attachment 44985 [details] without any errors.
Comment 29 Darin Adler 2009-12-16 09:47:02 PST
Comment on attachment 44985 [details]
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement / RenderTextControlSingleLine

> +        No new tests. This new methods are not exposed yet. It's use will be
> +        platform specific. Chromium specific changes are separated based on
> +        Darin Adler's comments and they are coming soon.

I think there was a misunderstanding. I suggested separating PopupMenu changes, not separating Chromium-specific changes. If there are Chromium-specific changes that relate directly to this patch, I could see them going in at the same time.

r=me
Comment 30 Zelidrag Hornung 2009-12-16 09:56:50 PST
Darin, I will put PopupMenu changes together with Chromium-specific stuff on the same patch. I will send this one as soon as this one gets checked in.
Comment 31 Dimitri Glazkov (Google) 2009-12-16 10:01:11 PST
Comment on attachment 44985 [details]
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement / RenderTextControlSingleLine

Let cq do its dirty work.
Comment 32 WebKit Commit Bot 2009-12-16 10:05:01 PST
Comment on attachment 44985 [details]
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement / RenderTextControlSingleLine

Rejecting patch 44985 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit']" exit_code: 1
Last 500 characters of output:
s/CommitQueue/WebCore/html/HTMLParser.cpp -o /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/i386/HTMLParser.o
** BUILD FAILED **

The following build commands failed:
WebCore:
	Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/i386/HTMLInputElement.o /Users/eseidel/Projects/CommitQueue/WebCore/html/HTMLInputElement.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://webkit-commit-queue.appspot.com/results/127614
Comment 33 Zelidrag Hornung 2009-12-16 10:27:39 PST
Created attachment 44990 [details]
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement / RenderTextControlSingleLine

- fixed String& compile warning from the previous patch, not really sure how my compiler could have missed it
Comment 34 WebKit Review Bot 2009-12-16 10:28:44 PST
style-queue ran check-webkit-style on attachment 44990 [details] without any errors.
Comment 35 WebKit Commit Bot 2009-12-16 10:43:35 PST
Comment on attachment 44990 [details]
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement / RenderTextControlSingleLine

Clearing flags on attachment: 44990

Committed r52204: <http://trac.webkit.org/changeset/52204>
Comment 36 WebKit Commit Bot 2009-12-16 10:43:42 PST
All reviewed patches have been landed.  Closing bug.