If changeShippingOption isn't called, the last selected=true option should be selected in the PaymentResponse

RESOLVED FIXED in Firefox 61

Status

()

P1
normal
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: MattN, Assigned: chenyu.chuang)

Tracking

Trunk
mozilla61
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox60 wontfix, firefox61 fixed)

Details

(Whiteboard: [webpayments])

Attachments

(2 attachments, 2 obsolete attachments)

STR:
1) Merchant provides 3 shipping options [a, b, & c] (a & c have `selected:true`).
2) The spec specifies that "If more than one item in the sequence has `selected` set to true, then the user agent selects the last one in the sequence."
so option c should be selected
3) The user doesn't change the shipping option (i.e. leaves the default of c selected) therefore `nsIPaymentRequestService.changeShippingOption` isn't called.
4) The user clicks Pay

Expected result:
The PaymentResponse.shippingOption should have the id of option "c". No shippingoptionchange should have been fired at all.

Actual result:
The PaymentResponse.shippingOption is null.

If the front-end were to call `changeShippingOption` to explicity re-select the already selected option then a `shippingoptionchange` would be fired when it probably shouldn't be. This should be covered by a web platform test.
Flags: in-testsuite?
> This should be covered by a web platform test.

I'll add a test for it.
Sent PR for the WPT: https://github.com/w3c/web-platform-tests/pull/9703

Was a good suggestion! Keep the test requests coming! :D
Flags: needinfo?(mcaceres)
Is the test case already hosted somewhere?
(In reply to Marcos Caceres [:marcosc] from comment #5)
> It is now :) 
> 
> https://w3c-test.org/payment-request/change-shipping-option-select-last-
> manual.https.html

This test passes on m-c trunk without patching. Is that expected due to the reason given in comment 1 or is there something more noteworthy going on?
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1)
> Currently the JS front-end is doing some of this and it should be removed
> when this bug is fixed,
> https://searchfox.org/mozilla-central/rev/
> 14d933246211b02f5be21d2e730a57cf087c6606/toolkit/components/payments/res/
> containers/payment-dialog.js#132-145

Do I understand correctly that we don't currently have a field for the selected option on the IPC stuff at all?
Flags: needinfo?(jaws)
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
That is correct. Currently the selected shipping option is only stored in the frontend.
Flags: needinfo?(jaws)
Would it be better for the front end to not to have the id string of the selected shipping option on nsIPaymentRequest and to instead have a guarantee that only the nsIShippingOption whose id shows up as selected in the Web-facing PaymentRequest has .selected as true?

It's a bit unclear to me what XPCOM view behavior is requested here.
Flags: needinfo?(jaws)
Whiteboard: [webpayments]
(Assignee)

Comment 11

a year ago
I tested with a mochitest in my local, the case in the description did not happen, response.shippingOption is with id c correctly. I tried to test with the web-platform-test in comment 5, payment UI is popup but I can not click the "pay" button. I guess caused by empty payment method, but I can not modify the field. Maybe someone can tell me how to fix the problem, so I can continue the testing with wpt-test.

The TODO mentioned in comment 1 is a different case with the bug description.
The TODO is in the PaymentRequest::UpdatePayment() which is called by PaymentRequestUpdateEvent::updateWith(), if these is no any shippingXXXchanged event fired, the function should never be executed.

The TODO wants to fix the problem that merchant updated the shippingOptions by calling updateWith().
If the updated shippingOptions has duplicate shipping option ids, we should empty the updated shippingOptions like we do in the payment request construction. And we should set the PaymentResponse.shippingOption to as the last selected one in the updated shippingOtpions.
(Assignee)

Comment 12

a year ago
The attached is the mochitest I wrote according to bug description.
Although current nightly build can pass it, it still helps to stabilize our codebase.
Eden, if you're able to take this, I'll let you assign it to yourself.
Assignee: hsivonen → nobody
Status: ASSIGNED → NEW
Priority: P1 → P2
(Assignee)

Comment 14

a year ago
Andrew, of course, I am happy to take this bug.

There is a tiny problem, I can not take it by myself, there is no "take" button for me.
I am not sure if there is any problem with my bugzilla account registration, but you can assign it to me. :)
Assignee: nobody → chenyu.chuang
Status: NEW → ASSIGNED
Priority: P2 → P1
(Assignee)

Comment 15

a year ago
I finally can press the "pay" button in the https://w3c-test.org/payment-request/change-shipping-option-select-last-manual.https.html. But I still got "PASS" result.

Matt, could you double confirm if you can reproduce the problem?
(In reply to EdenChuang(ChenYu Chuang) from comment #11)
> I tested with a mochitest in my local, the case in the description did not
> happen, response.shippingOption is with id c correctly. I tried to test with
> the web-platform-test in comment 5, payment UI is popup but I can not click
> the "pay" button. I guess caused by empty payment method, but I can not
> modify the field. Maybe someone can tell me how to fix the problem, so I can
> continue the testing with wpt-test.

In order to reproduce this issue you need to comment out this block:
https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/toolkit/components/payments/res/containers/payment-dialog.js#138-156

I see that the WPT test does pass if you only click Pay when the dialog opens but you will notice that the UI then shows that no shipping option is selected. Part of the point of this bug is that IMO the front-end shouldn't have to re-implement the spec algorithm implemented in the DOM code to determine which shipping option is selected. There are two ways I can see to address this:

a) It would be best for the front-end if nsIPaymentDetails.shippingOptions[i].selected reflected the result of running the spec algorithm.
b) Otherwise, we could make it work if PaymentRequest.shippingOption reflected the result and was exposed on nsIPaymentRequest.

> The TODO mentioned in comment 1 is a different case with the bug description.
> The TODO is in the PaymentRequest::UpdatePayment() which is called by
> PaymentRequestUpdateEvent::updateWith(), if these is no any
> shippingXXXchanged event fired, the function should never be executed.
> 
> The TODO wants to fix the problem that merchant updated the shippingOptions
> by calling updateWith().
> If the updated shippingOptions has duplicate shipping option ids, we should
> empty the updated shippingOptions like we do in the payment request
> construction. And we should set the PaymentResponse.shippingOption to as the
> last selected one in the updated shippingOtpions.

That sounds like a bug to be fixed as well, do you plan to fix that in this same bug or a different one?
Flags: needinfo?(MattN+bmo) → needinfo?(chenyu.chuang)
(Assignee)

Comment 17

a year ago
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #16)
> (In reply to EdenChuang(ChenYu Chuang) from comment #11)
> > I tested with a mochitest in my local, the case in the description did not
> > happen, response.shippingOption is with id c correctly. I tried to test with
> > the web-platform-test in comment 5, payment UI is popup but I can not click
> > the "pay" button. I guess caused by empty payment method, but I can not
> > modify the field. Maybe someone can tell me how to fix the problem, so I can
> > continue the testing with wpt-test.
> 
> In order to reproduce this issue you need to comment out this block:
> https://searchfox.org/mozilla-central/rev/
> a0665934fa05158a5a943d4c8b277465910c029c/toolkit/components/payments/res/
> containers/payment-dialog.js#138-156
> 
> I see that the WPT test does pass if you only click Pay when the dialog
> opens but you will notice that the UI then shows that no shipping option is
> selected. Part of the point of this bug is that IMO the front-end shouldn't
> have to re-implement the spec algorithm implemented in the DOM code to
> determine which shipping option is selected. There are two ways I can see to
> address this:
> 
> a) It would be best for the front-end if
> nsIPaymentDetails.shippingOptions[i].selected reflected the result of
> running the spec algorithm.
> b) Otherwise, we could make it work if PaymentRequest.shippingOption
> reflected the result and was exposed on nsIPaymentRequest.
> 

I will implement the b) add a new attribute PaymentRequest.shippingOption to reflected the selected one.

> > The TODO mentioned in comment 1 is a different case with the bug description.
> > The TODO is in the PaymentRequest::UpdatePayment() which is called by
> > PaymentRequestUpdateEvent::updateWith(), if these is no any
> > shippingXXXchanged event fired, the function should never be executed.
> > 
> > The TODO wants to fix the problem that merchant updated the shippingOptions
> > by calling updateWith().
> > If the updated shippingOptions has duplicate shipping option ids, we should
> > empty the updated shippingOptions like we do in the payment request
> > construction. And we should set the PaymentResponse.shippingOption to as the
> > last selected one in the updated shippingOtpions.
> 
> That sounds like a bug to be fixed as well, do you plan to fix that in this
> same bug or a different one?

I implemented it in bug 1441709.
Flags: needinfo?(chenyu.chuang)
(Assignee)

Comment 19

a year ago
Attachment #8961083 - Attachment is obsolete: true
Attachment #8966366 - Flags: review?(amarchesini)
Attachment #8966365 - Flags: review?(amarchesini) → review+
Attachment #8966366 - Flags: review?(amarchesini) → review+

Comment 21

11 months ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b12f74a216d
Add a new readonly attribute nsIPaymentRequest.shippingOption. r=baku.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f07f19c30db9
mochitest for nsIPaymentRequest.shippingOption. r=baku

Comment 22

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8b12f74a216d
https://hg.mozilla.org/mozilla-central/rev/f07f19c30db9
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
status-firefox60: affected → wontfix
Flags: in-testsuite? → in-testsuite+
Depends on: 1478029
See Also: bug 1478029
You need to log in before you can comment on or make changes to this bug.