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)
Core
DOM: Web Payments
Tracking
()
RESOLVED
FIXED
mozilla61
People
(Reporter: MattN, Assigned: chenyu.chuang)
References
Details
(Whiteboard: [webpayments])
Attachments
(2 files, 2 obsolete files)
17.27 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
23.58 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
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?
Comment 1•6 years ago
|
||
This is referenced in the DOM implementation as a TODO, https://searchfox.org/mozilla-central/rev/14d933246211b02f5be21d2e730a57cf087c6606/dom/payments/PaymentRequestManager.cpp#547-551 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
Comment 2•6 years ago
|
||
> This should be covered by a web platform test.
I'll add a test for it.
Comment 3•6 years ago
|
||
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?
Comment 5•6 years ago
|
||
It is now :) https://w3c-test.org/payment-request/change-shipping-option-select-last-manual.https.html
Flags: needinfo?(mcaceres)
(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
Comment hidden (mozreview-request) |
Comment 9•6 years ago
|
||
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)
Updated•6 years ago
|
Flags: needinfo?(jaws) → needinfo?(MattN+bmo)
Updated•6 years ago
|
Whiteboard: [webpayments]
Assignee | ||
Comment 11•6 years 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•6 years 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.
Comment 13•6 years ago
|
||
Eden, if you're able to take this, I'll let you assign it to yourself.
Assignee: hsivonen → nobody
Status: ASSIGNED → NEW
Updated•6 years ago
|
Priority: P1 → P2
Assignee | ||
Comment 14•6 years 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. :)
Updated•6 years ago
|
Assignee: nobody → chenyu.chuang
Updated•6 years ago
|
Status: NEW → ASSIGNED
Priority: P2 → P1
Assignee | ||
Comment 15•6 years 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?
Reporter | ||
Comment 16•6 years ago
|
||
(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•6 years 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 18•6 years ago
|
||
Attachment #8958446 -
Attachment is obsolete: true
Attachment #8966365 -
Flags: review?(amarchesini)
Assignee | ||
Comment 19•6 years ago
|
||
Attachment #8961083 -
Attachment is obsolete: true
Attachment #8966366 -
Flags: review?(amarchesini)
Updated•6 years ago
|
Attachment #8966365 -
Flags: review?(amarchesini) → review+
Updated•6 years ago
|
Attachment #8966366 -
Flags: review?(amarchesini) → review+
Reporter | ||
Comment 20•6 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=19635a1b96df861e47c96ea4a77571af6710794a
Comment 21•6 years 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•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8b12f74a216d https://hg.mozilla.org/mozilla-central/rev/f07f19c30db9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
Flags: in-testsuite? → in-testsuite+
Reporter | ||
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•