Closed Bug 1437879 Opened 5 years ago Closed 5 years ago

The shipping-option-picker doesn't show the selected option when the popup is closed

Categories

(Firefox :: WebPayments UI, enhancement, P1)

Product:
Firefox
Component:
WebPayments UI
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

Details

(Whiteboard: [webpayments])

Attachments

(1 file)

Bug 1437879 - Set the currency-amount properties as attributes on the shipping-option so they get copied to the cloned option.
59 bytes, text/x-review-board-request
MattN
: review+
Details 
The shipping-option-picker doesn't show the selected option when the popup is closed. Instead we were just showing an empty option with no text.

We clone the selected option to show it when the popup is closed, but we don't clone all the properties of the option.

Shipping options rely on the amount property to render, and since we didn't clone the option's properties we weren't rendering the cloned shipping option.
Comment on attachment 8950596 [details]
Bug 1437879 - Set the currency-amount properties as attributes on the shipping-option so they get copied to the cloned option.

https://reviewboard.mozilla.org/r/219866/#review225688

::: commit-message-38b3c:1
(Diff revision 1)
> +Bug 1437879 - Clone properties of cloned rich-select options so they will render when the popup is closed. r?mattn

I would very much prefer we move to using 2 attributes for the amount (amount-value & amount-currency) like we do for <payment-details-item>:
https://dxr.mozilla.org/mozilla-central/rev/6d8f470b2579e7570f14e3db557264dc075dd654/toolkit/components/payments/res/components/payment-details-item.js#18,22-23

I didn't like the inconsistency when I first reviewed the change to use a property but I thought maybe it will work out. Now that is caused problems and we're going to work around it more I think it was a mistake. Attributes also have the advantage of being easier to debug by looking at the inspector.

::: toolkit/components/payments/test/mochitest/test_shipping_option_picker.html:88
(Diff revision 1)
>    is(options[0].querySelector(".amount").getAttribute("currency"), "USD", "Check currency");
>    ok(options[1].textContent.includes("Lightspeed (default)"), "Check second option");
>    is(picker1.dropdown.selectedOption, options[1], "Lightspeed selected by default");
> +
> +  let selectedClone = picker1.dropdown.querySelector(".rich-select-selected-clone");
> +  is(selectedClone.textContent, "$20.00Lightspeed (default)",

I wouldn't recommend relying on the whitespace of .textContent (especially between nodes). You can use .includes instead. You should probably also check the visibility of the selectedClone with `isHidden` from `EventUtils.js`
Attachment #8950596 - Flags: review?(MattN+bmo)
Comment on attachment 8950596 [details]
Bug 1437879 - Set the currency-amount properties as attributes on the shipping-option so they get copied to the cloned option.

https://reviewboard.mozilla.org/r/219866/#review226134

::: commit-message-38b3c:1
(Diff revision 2)
> +Bug 1437879 - Clone properties of cloned rich-select options so they will render when the popup is closed. r?mattn

This commit message is leftover.

::: toolkit/components/payments/res/components/rich-select.js:203
(Diff revision 2)
> +      if ("clonedProperties" in selectedChild.constructor) {
> +        for (let prop of selectedChild.constructor.clonedProperties) {
> +          selectedClone[prop] = selectedChild[prop];
> +        }
> +      }

This seems leftover. Please revert.
Attachment #8950596 - Flags: review?(MattN+bmo) → review+
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d6a1c905d383
Set the currency-amount properties as attributes on the shipping-option so they get copied to the cloned option. r=MattN
https://hg.mozilla.org/mozilla-central/rev/d6a1c905d383
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Priority: -- → P1
Whiteboard: [webpayments]
Product: Toolkit → Firefox
Target Milestone: mozilla60 → Firefox 60
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.