| Summary: | [Apple Pay] use the first item in `shippingOptions` even when it's not `selected` | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||
| Component: | New Bugs | Assignee: | Devin Rousso <hi> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | aestes, ews-watchlist, hi, joepeck, keith_miller, mark.lam, mkwst, msaboff, saam, tzagallo, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Devin Rousso
2021-01-21 10:42:09 PST
Created attachment 418072 [details]
Patch
Comment on attachment 418072 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418072&action=review > LayoutTests/http/tests/paymentrequest/updateWith-shippingOptions.https.html:102 > + // assert_equals(request.shippingOption, detailsUpdate.shippingOptions.find((shippingOption) => shippingOption.selected).id, "selected shipping option should change"); My preference would be to let this assertion fail and update the test expectation (rather than commenting it out). Do you think we should log to the console when a merchant sets `selected` to true on something other than the first shipping option, letting them know that the selection won't be honored in the Apple Pay UI? Comment on attachment 418072 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418072&action=review (In reply to Andy Estes from comment #3) > Do you think we should log to the console when a merchant sets `selected` to true on something other than the first shipping option, letting them know that the selection won't be honored in the Apple Pay UI? Good idea! >> LayoutTests/http/tests/paymentrequest/updateWith-shippingOptions.https.html:102 >> + // assert_equals(request.shippingOption, detailsUpdate.shippingOptions.find((shippingOption) => shippingOption.selected).id, "selected shipping option should change"); > > My preference would be to let this assertion fail and update the test expectation (rather than commenting it out). I would agree, but AFAIK `assert_equals` actually throws an error, meaning that any `assert_equals` after it won't execute. Do you know of any ways to deal with this (e.g. have each test log instead of silent-pass-and-throw-on-failure)? Comment on attachment 418072 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=418072&action=review >>> LayoutTests/http/tests/paymentrequest/updateWith-shippingOptions.https.html:102 >>> + // assert_equals(request.shippingOption, detailsUpdate.shippingOptions.find((shippingOption) => shippingOption.selected).id, "selected shipping option should change"); >> >> My preference would be to let this assertion fail and update the test expectation (rather than commenting it out). > > I would agree, but AFAIK `assert_equals` actually throws an error, meaning that any `assert_equals` after it won't execute. Do you know of any ways to deal with this (e.g. have each test log instead of silent-pass-and-throw-on-failure)? Good point. You could move this assert into a new `user_activation_test` to isolate it from the others. Created attachment 418098 [details]
Patch
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features. Committed r271735: <https://trac.webkit.org/changeset/271735> All reviewed patches have been landed. Closing bug and clearing flags on attachment 418098 [details]. |