Reject PaymentRequest.show() with AbortError DOMException if the user agent's "payment request is showing" boolean is true

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: ben.tian, Assigned: edenchuang)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
mozilla56
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

a year ago
Web platform test for PaymentRequest.show() fails:
https://w3c-test.org/payment-request/payment-request-show-method.https.html

If the user agent's "payment request is showing" boolean is true, then return a promise rejected with an "AbortError" DOMException.

We should reject with AbortError instead of NS_ERROR_FAILURE.
(Reporter)

Updated

a year ago
Blocks: 1318987
(Reporter)

Updated

a year ago
Blocks: 1380546
(Reporter)

Updated

a year ago
No longer blocks: 1318987
(Assignee)

Updated

a year ago
Assignee: nobody → echuang
(Assignee)

Comment 1

a year ago
Created attachment 8888476 [details] [diff] [review]
Bug 1379892 - Reject PaymentRequest.show() with AbortError DOMException if a request is showing. r?baku
Attachment #8888476 - Flags: review?(amarchesini)
Adding ddn, just so we can check whether the docs are correct on this point.
Keywords: dev-doc-needed
Comment on attachment 8888476 [details] [diff] [review]
Bug 1379892 - Reject PaymentRequest.show() with AbortError DOMException if a request is showing. r?baku

Review of attachment 8888476 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/payments/PaymentRequest.cpp
@@ +403,5 @@
>    if (NS_WARN_IF(NS_FAILED(rv))) {
> +    if (rv == NS_ERROR_ABORT) {
> +      promise->MaybeReject(NS_ERROR_DOM_ABORT_ERR);
> +    } else {
> +      promise->MaybeReject(NS_ERROR_FAILURE);

NS_ERROR_FAILURE is a wrong error. Can we use invalid state? or anything else? What does the spec say here?

::: dom/payments/PaymentRequestService.cpp
@@ +327,5 @@
>        }
>        break;
>      }
>      /*
>       *  TODO: Launch/inform payment UI here once the UI module is implemented.

Is this comment still valid?
Attachment #8888476 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 4

a year ago
(In reply to Andrea Marchesini [:baku] from comment #3)
> Comment on attachment 8888476 [details] [diff] [review]
> Bug 1379892 - Reject PaymentRequest.show() with AbortError DOMException if a
> request is showing. r?baku
> 
> Review of attachment 8888476 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/payments/PaymentRequest.cpp
> @@ +403,5 @@
> >    if (NS_WARN_IF(NS_FAILED(rv))) {
> > +    if (rv == NS_ERROR_ABORT) {
> > +      promise->MaybeReject(NS_ERROR_DOM_ABORT_ERR);
> > +    } else {
> > +      promise->MaybeReject(NS_ERROR_FAILURE);
> 
> NS_ERROR_FAILURE is a wrong error. Can we use invalid state? or anything
> else? What does the spec say here?

When PaymentRequestManager::ShowPayment returns NS_ERROR_FAILURE, it can be IPC transfer error or some other internal error.
The spec doesn't describe the situations about internal errors. According to the spec, the state is valid, so returning invalid state error might not the best. I prefer returning NS_ERROR_DOM_NOT_ALLOWED_ERR or NS_ERROR_DOM_UNKNOWN_ERR. How do you think?

> 
> ::: dom/payments/PaymentRequestService.cpp
> @@ +327,5 @@
> >        }
> >        break;
> >      }
> >      /*
> >       *  TODO: Launch/inform payment UI here once the UI module is implemented.
> 
> Is this comment still valid?

Yes, and this comment will be remove after bug 1382092 is resolved.
Flags: needinfo?(amarchesini)
> the spec, the state is valid, so returning invalid state error might not the
> best. I prefer returning NS_ERROR_DOM_NOT_ALLOWED_ERR or
> NS_ERROR_DOM_UNKNOWN_ERR. How do you think?

Better. I prefer NS_ERROR_DOM_NOT_ALLOWED_ERR but this should be covered by spec.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 8

a year ago
Created attachment 8889805 [details] [diff] [review]
Bug 1379892 - Reject PaymentRequest.show() with AbortError DOMException if a request is showing. r=baku
Attachment #8888476 - Attachment is obsolete: true
Attachment #8889805 - Flags: review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 9

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd757326f72b
Reject PaymentRequest.show() with AbortError DOMException if a request is showing. r=baku
Keywords: checkin-needed

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dd757326f72b
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
I've added details of exceptions to this page:

https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest/show#Exceptions

Let me know if that looks OK. Thanks!
Keywords: dev-doc-needed → dev-doc-complete
(Reporter)

Comment 12

a year ago
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #11)
> https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest/
> show#Exceptions
> 
> Let me know if that looks OK. Thanks!

Correction:

In |AbortError| description, "payment request is showing" boolean is true means _another_ payment has already been shown, not this payment. If this payment is showing, call to show() would be rejected with |InvalidStateError| since the request's state is 'interactive' (i.e., the payment request is being presented to the user), not 'created' [2].

[1] https://w3c.github.io/payment-request/#dfn-payment-request-is-showing
[2] step 3 in https://w3c.github.io/payment-request/#show()-method
(In reply to Ben Tian [:btian] from comment #12)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #11)
> > https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest/
> > show#Exceptions
> > 
> > Let me know if that looks OK. Thanks!
> 
> Correction:
> 
> In |AbortError| description, "payment request is showing" boolean is true
> means _another_ payment has already been shown, not this payment. If this
> payment is showing, call to show() would be rejected with
> |InvalidStateError| since the request's state is 'interactive' (i.e., the
> payment request is being presented to the user), not 'created' [2].
> 
> [1] https://w3c.github.io/payment-request/#dfn-payment-request-is-showing
> [2] step 3 in https://w3c.github.io/payment-request/#show()-method

Cool, thanks for the corrections. I've updated the AbortError description, and added an InvalidStateError description.
You need to log in before you can comment on or make changes to this bug.