| Summary: | HTMLFormElement#requestAutocomplete() should require a user action | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Elliott Sprehn <esprehn> | ||||||||||||||||
| Component: | Forms | Assignee: | Dan Beam <dbeam> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | abarth, dglazkov, esprehn, estade, fishd, ian, isherman, jamesr, japhet, mifenton, ojan, tkent, tkent+wkapi, webkit.review.bot | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | All | ||||||||||||||||||
| OS: | All | ||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||
| Bug Blocks: | 100560 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
Should be an easy patch. If I'm just passing along this info to the embedder (so it can fail if it'd like), I guess I have to wait for the code to land in Chrome before I can test firing an autocompleteerror on a non-user gesture. Also, is there a way to simulate user gestures in layout tests? Hey Ian and Adam, This bug is about requiring HTMLFormElement#requestAutocomplete() be based on a user action, else dispatching an error. Is this behavior going to be specified (meaning all WebKit implementations will need to behave the same) or will it be up to each embedder/browser to choose this behavior? Whether it's standardized or not affects my patch, that's why I'm asking. Created attachment 177887 [details]
Patch
(In reply to comment #3) > Hey Ian and Adam, > > This bug is about requiring HTMLFormElement#requestAutocomplete() be based on a user action, else dispatching an error. Is this behavior going to be specified (meaning all WebKit implementations will need to behave the same) or will it be up to each embedder/browser to choose this behavior? > > Whether it's standardized or not affects my patch, that's why I'm asking. As the person writing the preliminary spec I was going to require it for all implementations. Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI. Created attachment 177888 [details]
changelog typo
esprehn@: I only see 2 differentiations on whether something is a user action or not in this document (http://dev.w3.org/html5/spec/single-page.html) 1) http://dev.w3.org/html5/spec/single-page.html#history-notes """ In addition, a user agent could ignore calls to pushState() that are invoked on a timer, or from event listeners that are not triggered in response to a clear user action, or that are invoked in rapid succession. """ 2) http://dev.w3.org/html5/spec/single-page.html#context-menus """ The context information of the event must be initialized to the same values as the last MouseEvent user interaction event that was fired as part of the gesture that that was interpreted as a request for the context menu. """ It'd seem only #1 is applicable, and it's only a recommendation (if that). However, there's a first time for everything, :). Created attachment 177894 [details]
if specified impl
Comment on attachment 177894 [details] if specified impl View in context: https://bugs.webkit.org/attachment.cgi?id=177894&action=review > Source/WebCore/ChangeLog:11 > + No new tests (OOPS!). OOPS! You had better mention that you updated form-request-autocomplete.html. Comment on attachment 177894 [details] if specified impl View in context: https://bugs.webkit.org/attachment.cgi?id=177894&action=review >> Source/WebCore/ChangeLog:11 >> + No new tests (OOPS!). > > OOPS! > You had better mention that you updated form-request-autocomplete.html. Yeah, I don't know why the scripts aren't picking this up automatically... will update now. (In reply to comment #8) > esprehn@: I only see 2 differentiations on whether something is a user action or not in this document (http://dev.w3.org/html5/spec/single-page.html) http://www.whatwg.org/specs/web-apps/current-work/#allowed-to-show-a-pop-up window.open() is indeed spec'ed to require a trusted event. See also DOMWindow::allowPopUp which has the user gesture code baked into webkit, not into the embedder. Created attachment 177896 [details]
if unspecified impl
Created attachment 177898 [details]
if specified impl
(In reply to comment #13) > Created an attachment (id=177896) [details] > if unspecified impl I talked with esprehn@ and we agreed that we'd want to spec the safer, simpler to implement version of this API. So, I've obsoleted the unspecified implementation in favor of the specified one. tkent@: if I could steal a minute of your time it'd be awesome to get a [very short] review on the non-obsolete patch (https://bugs.webkit.org/attachment.cgi?id=177898&action=review) now that I'm done juggling this bug's attachments, :P. Comment on attachment 177898 [details] if specified impl View in context: https://bugs.webkit.org/attachment.cgi?id=177898&action=review > Source/WebCore/ChangeLog:14 > + * fast/forms/form-request-autocomplete.html: > + > + Added a test to ensure that dispatching a call to HTMLFormElement#requestAutocomplete() in a setTimeout() (which will > + never be a user gesture) always fails. nit: This file is not a part of WebCore. So you don't need to follow the file list style here. "fast/forms/form-request-autocomplete.html is updated." is enough. > As the person writing the preliminary spec I was going to require it for all implementations.
Makes sense.
I think it makes sense that it happen that way, but in practice the spec won't require it, since it's UI. But acting as if it does is appropriate. Created attachment 177925 [details]
Patch for landing
Comment on attachment 177898 [details] if specified impl View in context: https://bugs.webkit.org/attachment.cgi?id=177898&action=review >> Source/WebCore/ChangeLog:14 >> + never be a user gesture) always fails. > > nit: This file is not a part of WebCore. So you don't need to follow the file list style here. > "fast/forms/form-request-autocomplete.html is updated." is enough. Done. Comment on attachment 177925 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=177925&action=review > Source/WebCore/ChangeLog:11 > + * fast/forms/form-request-autocomplete.html is updated. "* " is confusing. It looks like a part of WebCore files. Created attachment 177931 [details]
Patch for landing
Comment on attachment 177925 [details] Patch for landing Clearing flags on attachment: 177925 Committed r136800: <http://trac.webkit.org/changeset/136800> All reviewed patches have been landed. Closing bug. Comment on attachment 177925 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=177925&action=review >> Source/WebCore/ChangeLog:11 >> + * fast/forms/form-request-autocomplete.html is updated. > > "* " is confusing. It looks like a part of WebCore files. Sorry, removed. Comment on attachment 177931 [details]
Patch for landing
OMG
(In reply to comment #24) > All reviewed patches have been landed. Closing bug. Whoops, too late. I'll remember next time. I understand now, looking at the patchset (http://trac.webkit.org/changeset/136800), why * matters. (In reply to comment #26) > (From update of attachment 177931 [details]) > OMG Yes, bad timing :(. Is there anything else I need to do for this bug? Comment on attachment 177925 [details]
Patch for landing
this was the committed patch
|
requestAutocomplete should require a user action and should dispatch a failure event when triggered from a non user action: ex. setTimeout(function() { form.requestAutocomplete(); // this should always fail. }, 0);