Closed Bug 1442078 Opened 3 years ago Closed 3 years ago

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

Categories

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

Product:
Core
Component:
DOM: Web Payments
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: marcosc, Assigned: jonathanGB)

References

(Depends on 1 open bug, Blocks 1 open bug,
URL
)

Details

(Keywords: dev-doc-complete, Whiteboard: [webpayments])

Attachments

(3 files)

Bug 1442078 - throw "SecurityError" if `show` is not triggered by user activation.
59 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1442078 - Modify DOM tests that were broken by the change to the "show" specs.
59 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1442078 - Modify UI tests that were broken by the change to the "show" specs.
59 bytes, text/x-review-board-request
MattN
: review+
Details 
Calling show() without being triggered by user activation needs to throw, as per spec.
Blocks: paymentrequest
URL: https://github.com/w3c/payment-reques...
Priority: -- → P1
This can be implemented with EventStateManager::IsHandlingUserInput
Assignee: nobody → jonathan.guillotte.blouin
Status: NEW → ASSIGNED
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?
Attachment #8957428 - Flags: review?(MattN+bmo)
Attachment #8957427 - Flags: review?(amarchesini)
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.
Attachment #8957426 - Flags: review?(amarchesini) → review+
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
Attachment #8957428 - Flags: review?(MattN+bmo) → review+
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 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
Attachment #8957427 - Flags: review?(amarchesini) → review+
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)
There are conflicts while landing this patch, :jonathanGB Can you please have a look?
Flags: needinfo?(jonathan.guillotte.blouin)
Flags: needinfo?(jonathan.guillotte.blouin)
Keywords: checkin-needed
Resolved the conflicts :ccoroiu !
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
Whiteboard: [webpayments]
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Priority: P1 → --
Whiteboard: [webpayments]
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
Status: REOPENED → RESOLVED
Closed: 3 years ago → 3 years ago
Priority: -- → P1
Resolution: --- → FIXED
Whiteboard: [webpayments]
(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.
File new bug: 
https://bugzilla.mozilla.org/show_bug.cgi?id=1447178

Interested folks should follow along there.
Depends on: 1447178
Depends on: 1491996
Docs already cover this.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.