1442078 - PaymentRequest.show() must only be triggered by user activation

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, enhancement, P1)

Description

[Marcos Caceres [:marcosc]]

Calling show() without being triggered by user activation needs to throw, as per spec.

Comment 1

[Matthew N. [:MattN]]

This can be implemented with EventStateManager::IsHandlingUserInput

Comment 2

[Jonathan Guillotte-Blouin [:jonathanGB]]

Attachment: Bug 1442078 - throw "SecurityError" if `show` is not triggered by user activation.

Review commit: https://reviewboard.mozilla.org/r/226350/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226350/

Comment 3

[Jonathan Guillotte-Blouin [:jonathanGB]]

Attachment: Bug 1442078 - Modify DOM tests that were broken by the change to the "show" specs.

Review commit: https://reviewboard.mozilla.org/r/226352/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226352/

Comment 4

[Jonathan Guillotte-Blouin [:jonathanGB]]

Attachment: Bug 1442078 - Modify UI tests that were broken by the change to the "show" specs.

Review commit: https://reviewboard.mozilla.org/r/226354/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226354/

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8957428 [details]
Bug 1442078 - Modify UI tests that were broken by the change to the "show" specs.

https://reviewboard.mozilla.org/r/226354/#review232244

Thanks Jonathan!

::: toolkit/components/payments/test/PaymentTestUtils.jsm:86
(Diff revision 1)
> +      content.QueryInterface(Ci.nsIInterfaceRequestor)
> +             .getInterface(Ci.nsIDOMWindowUtils)
> +             .setHandlingUserInput(true);
> +
>        const rq = new content.PaymentRequest(methodData, details, options);
>        content.rq = rq; // assign it so we can retrieve it later
>        content.showPromise = rq.show();

I think we should reset this after show() is called… not sure if that will break more tests but it may be worth it. Can you give that a try?

Comment 6

[Andrea Marchesini [:baku]]

Comment on attachment 8957426 [details]
Bug 1442078 - throw "SecurityError" if `show` is not triggered by user activation.

https://reviewboard.mozilla.org/r/226350/#review232256

Please, add a test.

Comment 7

[Jonathan Guillotte-Blouin [:jonathanGB]]

Comment on attachment 8957427 [details]
Bug 1442078 - Modify DOM tests that were broken by the change to the "show" specs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226352/diff/1-2/

Comment 8

[Jonathan Guillotte-Blouin [:jonathanGB]]

Comment on attachment 8957428 [details]
Bug 1442078 - Modify UI tests that were broken by the change to the "show" specs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226354/diff/1-2/

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8957428 [details]
Bug 1442078 - Modify UI tests that were broken by the change to the "show" specs.

https://reviewboard.mozilla.org/r/226354/#review232436

Thanks

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8957428 [details]
Bug 1442078 - Modify UI tests that were broken by the change to the "show" specs.

https://reviewboard.mozilla.org/r/226354/#review232438

::: toolkit/components/payments/test/PaymentTestUtils.jsm:86
(Diff revision 2)
> +      const handle = content.QueryInterface(Ci.nsIInterfaceRequestor)
> +                            .getInterface(Ci.nsIDOMWindowUtils)
> +                            .setHandlingUserInput(true);
> +

To make the test more accurate, technically this should go directly above the `rq.show()` call.

Comment 11

[Jonathan Guillotte-Blouin [:jonathanGB]]

Comment on attachment 8957428 [details]
Bug 1442078 - Modify UI tests that were broken by the change to the "show" specs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226354/diff/2-3/

Comment 12

[Andrea Marchesini [:baku]]

Comment on attachment 8957427 [details]
Bug 1442078 - Modify DOM tests that were broken by the change to the "show" specs.

https://reviewboard.mozilla.org/r/226352/#review232580

Comment 13

[Jonathan Guillotte-Blouin [:jonathanGB]]

Comment on attachment 8957427 [details]
Bug 1442078 - Modify DOM tests that were broken by the change to the "show" specs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226352/diff/2-3/

Comment 14

[Jonathan Guillotte-Blouin [:jonathanGB]]

Comment on attachment 8957428 [details]
Bug 1442078 - Modify UI tests that were broken by the change to the "show" specs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226354/diff/3-4/

Comment 15

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 75437fefd4a91ce5af2fe7b49b6ac7a8dbd14baa -d 5b4fefc4bb90: rebasing 452208:75437fefd4a9 "Bug 1442078 - throw "SecurityError" if `show` is not triggered by user activation. r=baku"
merging dom/payments/PaymentRequest.cpp
rebasing 452209:4cfe935ec290 "Bug 1442078 - Modify DOM tests that were broken by the change to the "show" specs. r=baku"
merging dom/payments/test/ShowPaymentChromeScript.js
merging dom/payments/test/test_showPayment.html
warning: conflicts while merging dom/payments/test/test_showPayment.html! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 16

[Cristina Coroiu [:ccoroiu]]

There are conflicts while landing this patch, :jonathanGB Can you please have a look?

Comment 17

[Jonathan Guillotte-Blouin [:jonathanGB]]

Comment on attachment 8957426 [details]
Bug 1442078 - throw "SecurityError" if `show` is not triggered by user activation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226350/diff/1-2/

Comment 18

[Jonathan Guillotte-Blouin [:jonathanGB]]

Comment on attachment 8957427 [details]
Bug 1442078 - Modify DOM tests that were broken by the change to the "show" specs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226352/diff/3-4/

Comment 19

[Jonathan Guillotte-Blouin [:jonathanGB]]

Comment on attachment 8957428 [details]
Bug 1442078 - Modify UI tests that were broken by the change to the "show" specs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226354/diff/4-5/

Comment 20

[Jonathan Guillotte-Blouin [:jonathanGB]]

Resolved the conflicts :ccoroiu !

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6b8e0261486
throw "SecurityError" if `show` is not triggered by user activation. r=baku
https://hg.mozilla.org/integration/autoland/rev/dcc9329ec127
Modify DOM tests that were broken by the change to the "show" specs. r=baku
https://hg.mozilla.org/integration/autoland/rev/457b8e3d5efa
Modify UI tests that were broken by the change to the "show" specs. r=MattN

Comment 22

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/f6b8e0261486
https://hg.mozilla.org/mozilla-central/rev/dcc9329ec127
https://hg.mozilla.org/mozilla-central/rev/457b8e3d5efa

Comment 23

[Marcos Caceres [:marcosc]]

Testing in Nightly, "triggered by user activation" doesn't seem to survive `postMessage()`? 

For this to be usable by Stripe, and to be compatible with Chrome, it has to be possible to `postMessage()` from a user gesture, and then have a script in the iframe pick it up.

I've created a web platform test: 
https://github.com/w3c/web-platform-tests/pull/10090

Comment 24

[Matthew N. [:MattN]]

Marcos, once code has landed, we generally don't re-open bugs unless that code is backed out. It makes it hard to track what version code landed in otherwise. Follow-up bugs should be filed for follow-up issues.

Also, that issue isn't specific to PaymentRequest… Jonathan implemented the Gecko standard method of checking for user interaction so if we want to loosen that it should be considered for other uses of IsHandlingUserInput too. Can you file a follow-up Core::DOM bug that blocks this one and mark it with [webpayments]? If you prefer I file it then needinfo me please. Thanks

Comment 25

[Marcos Caceres [:marcosc]]

(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #24)
> Marcos, once code has landed, we generally don't re-open bugs unless that
> code is backed out.

No probs.

> It makes it hard to track what version code landed in
> otherwise. Follow-up bugs should be filed for follow-up issues.
> 
> Also, that issue isn't specific to PaymentRequest… Jonathan implemented the
> Gecko standard method of checking for user interaction so if we want to
> loosen that it should be considered for other uses of IsHandlingUserInput
> too. Can you file a follow-up Core::DOM bug that blocks this one and mark it
> with [webpayments]? If you prefer I file it then needinfo me please. Thanks

Sounds good. Let's follow up in a new bug. I'll file it tomorrow, unless you want to do it... we might need to drag bz in. If we can't propagate the trigger, that might be kinda bad.

Comment 26

[Marcos Caceres [:marcosc]]

File new bug: 
https://bugzilla.mozilla.org/show_bug.cgi?id=1447178

Interested folks should follow along there.

Comment 27

[Eric Shepherd [:sheppy]]

Docs already cover this.