Description
Zelidrag Hornung
2009-12-07 18:14:33 PST
Created attachment 44580 [details]
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement and related RenderTextControlSingleLine classes.
style-queue ran check-webkit-style on attachment 44580 [details] without any errors.
Created attachment 44581 [details]
Added ability to separate autocomplete suggestion previewing from accepting of the suggested value in HTMLInputElement / RenderTextControlSingleLine
style-queue ran check-webkit-style on attachment 44581 [details] without any errors.
Have you run this by chrome's UI leads? Why do we want this behavior of not exposing the changed value to JavaScript? 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. 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). Chrome UI leads would appreciate this behavior for the reason Glen describes. (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. There are no additional DOM APIs. Please see https://bugs.webkit.org/show_bug.cgi?id=32255 for more discussion. 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 ..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) 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. 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. 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 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.
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.
style-queue ran check-webkit-style on attachment 44840 [details] without any errors.
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 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.
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 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.
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 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.
style-queue ran check-webkit-style on attachment 44950 [details] without any errors.
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.
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.
style-queue ran check-webkit-style on attachment 44985 [details] without any errors.
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 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 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 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 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
style-queue ran check-webkit-style on attachment 44990 [details] without any errors.
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> All reviewed patches have been landed. Closing bug. |