-
Notifications
You must be signed in to change notification settings - Fork 53
Add an autofill module #706
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yoavweiss Does this proposal depend on the user agent (the browser) supporting autofill? What if the browser doesn't support this feature? Is it dependent on the autofill portion of the HTML Living Standard? Is it expected that the property names in the autofill store for testing
map must match the autofill field name tokens? If not, what are the restrictions on the property names, if any?
@jimevans - I thought that using a "should" language in "The [=user agent=] should [=autofill=] |element| and |element|'s [=form owner=], while taking into account the contents of |store|" would be enough to ensure that user agents that don't support autofill would still be compliant. I'm happy to add an explicit condition to that effect.
It does. I guess I didn't make this dependency explicit. I'll take a stab at that. |
It looks like the preview has not been regenerated for some reason (I am still seeing the save command). |
@OrKoN I've addressed all your comments, would you be able to give it another pass? Thanks in advance! |
/cc @galich - I'd love your thoughts on this :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, the approach LGTM but I wonder what @jgraham @whimboo @gsnedders think.
Hi @OrKoN , coming back from a 2-weeks vacation, I wanted to check in and see where we stand with this PR - is this ok to be merged, so we can move on to the implementation phase? Thanks in advance for a quick update! |
@martinLechnerShopify we need the approval from other browser vendors as well to merge this |
@OrKoN thanks, is there some kind of regular "sync" or a timeline when other browser vendors are usually commenting/approving? I'd like to make sure we can push this over the finish line together in a reasonable timeframe. |
The meeting happens on the second Wednesday of the month. @AutomatedTester is the chair and the organizer of the meeting. @yoavweiss already presented in the proposal in one of the previous meetings https://www.w3.org/2024/05/08-webdriver-minutes.html |
Thanks all for the TPAC discussion. The presentation link above is the wrong one tho. Here's the actual one. |
So did the design here get any updates in response to the TPAC discussion? IIRC correctly a concern in that session was that this API ends up both specifying the autofill data and also requesting that data to be filled, so it was unclear that it was leaving enough of the original browser codepaths to be meaningful. In particular it wouldn't allow checking that the expected data is saved/provided by the data given a prior user interaction e.g. a form submission. |
@jgraham sorry for the delayed response as I was involved with a few high prio projects at Shopify. Thanks for getting back to us! As to your points, if I recall TPAC discussions correctly, I think we said there is value in having both the explicit tests on filling correctly (i.e. the proposal we currently have), and the actual data saving part (or even better, a combination of both). This would leave us with 3 possible kinds of autofill tests:
Do you consider 1 and 2 valuable in itself, do you think it only makes sense to go with 3, or do you see all 3 options viable? Also, looking a bit more closely into option #3, I am thinking that the test flow would be like:
wdyt? |
I think testing the filling part (no. 1) is the most important as there are many ways data might end up being saved into the autofill storage (updated over time, profile sync etc). The web authors probably care most if a specific form(s) can be auto-filled on their website (given it is in storage) and not so much if browser's autofill storage can be populated using their website specifically. Also, not every user agent might be saving data from forms and given that this storage part is largely non-standard, focusing on the filling part seems to be most beneficial. The spec that includes the saving part should also define how the data is stored and for how long (probably should not survive the WebDriver session), how the autofill options are presented to the client and how they can be selected, how sensitive data like credit cards could be saved and loaded. |
I agree with @OrKoN 's assessment actually. @jgraham would you be OK to approach this in a phased launch? I.e. starting with the filling spec specifically, getting this over the finish line, and then add the joint saving + filling part? I don't think they are mutually exclusive, but speaking from a strictly web developer perspective, I also consider the filling part as the most important one. Would you be OK with this approach? |
@jgraham any thoughts on the above? Would you be OK with a phased approach? |
This reverts commit 2b63c0e.
|
||
The [=remote end steps=] with |session| and |command parameters| are: | ||
|
||
1. Let |navigable id| be the value of the <code>navigable</code> field of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1. Let |navigable id| be the value of the <code>navigable</code> field of | |
1. Let |navigable id| be the value of the <code>context</code> field of |
1. If user agent cannot autofill |field|, return an [=error=] with | ||
[=error code=] [=unsupported operation=]. | ||
|
||
1. The user agent should [=autofill=] |element| and |element|'s [=form | ||
owner=], while taking into account the contents of |field|. | ||
|
||
1. Return [=success=] with data null. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1. If user agent cannot autofill |field|, return an [=error=] with | |
[=error code=] [=unsupported operation=]. | |
1. The user agent should [=autofill=] |element| and |element|'s [=form | |
owner=], while taking into account the contents of |field|. | |
1. Return [=success=] with data null. | |
1. Run implementation-specific steps to [=autofill=] |element| and |element|'s [=form owner=], while taking into account the contents of |field|. If this is not possible return [=error=] with error code [=unsupported operation=]. | |
1. Return [=success=] with data null. |
value: text | ||
} | ||
|
||
autofill.setAddress = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe each command should have its own section
} | ||
|
||
autofill.setAddress = ( | ||
method: "autofill.setAddress", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused by this API:
autofill.trigger
has a single paramelement
withfield {name, value}
. Does it mean it triggers the autofill only on a single field? What about multi-field addresses?- What is the point of
autofill.setAddress
, if the only way to trigger that autofill isautofill.trigger
, which providesvalues
?
This is a PR similar to w3c/webdriver#1797, but on the BiDi side.
It adds a
save
andtrigger
autofill commands./cc @sadym-chromium
Preview | Diff