Closed Bug 1440041 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: MattN, Assigned: chenyu.chuang)

References

Details

(Whiteboard: [webpayments])

Attachments

(2 files, 2 obsolete files)

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
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)
Flags: needinfo?(jaws) → needinfo?(MattN+bmo)
Whiteboard: [webpayments]
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.
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
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
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)
(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)
Attachment #8961083 - Attachment is obsolete: true
Attachment #8966366 - Flags: review?(amarchesini)
Attachment #8966365 - Flags: review?(amarchesini) → review+
Attachment #8966366 - Flags: review?(amarchesini) → review+
Blocks: 1454129
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
https://hg.mozilla.org/mozilla-central/rev/8b12f74a216d
https://hg.mozilla.org/mozilla-central/rev/f07f19c30db9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Flags: in-testsuite? → in-testsuite+
See Also: → 1478029
Depends on: 1478029
See Also: 1478029
You need to log in before you can comment on or make changes to this bug.