Bug 217365 - [Payment Request] Calling PaymentRequest's show() should consume user activation
Summary: [Payment Request] Calling PaymentRequest's show() should consume user activation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-05 21:58 PDT by Marcos Caceres
Modified: 2021-08-31 16:07 PDT (History)
7 users (show)

See Also:


Attachments
Patch (2.00 KB, patch)
2021-08-22 23:38 PDT, Marcos Caceres
no flags Details | Formatted Diff | Diff
Patch (47.50 KB, patch)
2021-08-25 00:22 PDT, Marcos Caceres
no flags Details | Formatted Diff | Diff
Patch (47.53 KB, patch)
2021-08-25 00:26 PDT, Marcos Caceres
no flags Details | Formatted Diff | Diff
Patch (52.04 KB, patch)
2021-08-25 17:22 PDT, Marcos Caceres
no flags Details | Formatted Diff | Diff
Patch (52.13 KB, patch)
2021-08-26 00:35 PDT, Marcos Caceres
no flags Details | Formatted Diff | Diff
Patch (52.05 KB, patch)
2021-08-31 02:37 PDT, Marcos Caceres
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marcos Caceres 2020-10-05 21:58:28 PDT
See: 
https://github.com/w3c/payment-request/pull/916

Tests incoming via WPT.
Comment 1 Radar WebKit Bug Importer 2020-10-12 21:59:15 PDT
<rdar://problem/70238596>
Comment 2 Marcos Caceres 2021-08-22 23:38:37 PDT
Created attachment 436153 [details]
Patch
Comment 3 Marcos Caceres 2021-08-25 00:22:12 PDT
Created attachment 436374 [details]
Patch
Comment 4 EWS Watchlist 2021-08-25 00:23:12 PDT
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 5 Marcos Caceres 2021-08-25 00:26:49 PDT
Created attachment 436375 [details]
Patch
Comment 6 Darin Adler 2021-08-25 15:05:42 PDT
Comment on attachment 436375 [details]
Patch

Patch needs to update or delete this file:

LayoutTests/platform/ios-wk2/imported/w3c/web-platform-tests/payment-request/show-method-optional-promise-rejects.https-expected.txt
Comment 7 Marcos Caceres 2021-08-25 17:22:10 PDT
Created attachment 436443 [details]
Patch
Comment 8 youenn fablet 2021-08-25 23:59:50 PDT
Comment on attachment 436443 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436443&action=review

> Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:375
> +    if (window && !window->consumeTransientActivation()) {

Should it be:
if (!window || !window->consumeTransientActivation()) {
}

> LayoutTests/imported/w3c/web-platform-tests/payment-request/payment-is-showing.https.html:7
> +<meta name="timeout" content="long" />

I am guessing this is resyncing our copy of WPT tests with upstream.
If so, can you update the change log?
Comment 9 Marcos Caceres 2021-08-26 00:24:42 PDT
Comment on attachment 436443 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436443&action=review

>> Source/WebCore/Modules/paymentrequest/PaymentRequest.cpp:375
>> +    if (window && !window->consumeTransientActivation()) {
> 
> Should it be:
> if (!window || !window->consumeTransientActivation()) {
> }

Yes. You are right. I was unsure if I should treat a nullprt window as a different kind of error. Will update.

>> LayoutTests/imported/w3c/web-platform-tests/payment-request/payment-is-showing.https.html:7
>> +<meta name="timeout" content="long" />
> 
> I am guessing this is resyncing our copy of WPT tests with upstream.
> If so, can you update the change log?

Will do!
Comment 10 Marcos Caceres 2021-08-26 00:35:05 PDT
Created attachment 436477 [details]
Patch
Comment 11 Devin Rousso 2021-08-26 14:29:31 PDT
Comment on attachment 436477 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436477&action=review

r=me as well :)

> LayoutTests/imported/w3c/web-platform-tests/payment-request/payment-request-show-method.https-expected.txt:4
> +FAIL Throws if the promise [[state]] is not 'created'. promise_rejects_dom: function "function () { throw e }" threw object "SecurityError: show() must be triggered by user activation." that is not a DOMException InvalidStateError: property "code" is equal to 18, expected 11

Is there a reason that we're still failing?
Comment 12 Marcos Caceres 2021-08-31 02:05:17 PDT
(In reply to Devin Rousso from comment #11)
> > LayoutTests/imported/w3c/web-platform-tests/payment-request/payment-request-show-method.https-expected.txt:4
> > +FAIL Throws if the promise [[state]] is not 'created'. promise_rejects_dom: function "function () { throw e }" threw object "SecurityError: show() must be triggered by user activation." that is not a DOMException InvalidStateError: property "code" is equal to 18, expected 11
> 
> Is there a reason that we're still failing?

Just returning to this now... investigating the failures now.
Comment 13 Marcos Caceres 2021-08-31 02:37:58 PDT
Created attachment 436859 [details]
Patch
Comment 14 Marcos Caceres 2021-08-31 02:49:31 PDT
(In reply to Marcos Caceres from comment #12)
> (In reply to Devin Rousso from comment #11)
> > Is there a reason that we're still failing?

Bad test. I sent a PR to fix it on the WPT side:
https://github.com/web-platform-tests/wpt/pull/30255
 
And all PASS now! ^_^

> Just returning to this now... investigating the failures now.

Hopefully all good now. 🤞
Comment 15 EWS 2021-08-31 16:07:51 PDT
Committed r281830 (241163@main): <https://commits.webkit.org/241163@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436859 [details].