Use native <option> UI with <rich-select> in the open state

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P1
normal
RESOLVED FIXED
11 months ago
9 months ago

People

(Reporter: MattN, Assigned: prathiksha)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
Firefox 63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [webpayments])

Attachments

(5 attachments, 2 obsolete attachments)

When a <rich-select> popup is "opened", we currently use a custom popup style in-content. We should switch to using native <option>s by having a hidden <select> which receives the clicks on the closed <rich-select>.

We should ensure that the tab order and keyboard accessibility is correct with @tabindex and friends.
Attachment #8979729 - Attachment mime type: text/plain → text/html
(Assignee)

Updated

11 months ago
Assignee: nobody → prathikshaprasadsuman
Status: NEW → ASSIGNED

Updated

11 months ago
Priority: P2 → P1
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8983235 - Flags: feedback?(MattN+bmo)
(Reporter)

Comment 2

11 months ago
mozreview-review
Comment on attachment 8983235 [details]
Bug 1463554 - test changes.

https://reviewboard.mozilla.org/r/249104/#review255300

This is on the right track and a great proof-of-concept! I have some comments to help handle some of the complexity to deal with the other pickers and picker modes.

::: browser/components/payments/res/components/rich-select.css:35
(Diff revision 1)
> +select {
> +  opacity: 0;
> +}

You should only do this for select inside of rich-select or it will hide the <select> we use on add/edit screens that aren't in <rich-select>.

::: browser/components/payments/res/components/rich-select.js:7
(Diff revision 1)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  import ObservedPropertiesMixin from "../mixins/ObservedPropertiesMixin.js";
>  import RichOption from "./rich-option.js";
> +import AddressOption from "./address-option.js";

<rich-select> should remain generic and not depend on a specific RichOption subclass directly. Instead we could have an attribute on <rich-select> to specify which tag name to use for the closed state. e.g.
```
html
<rich-select … optionTag="address-option">
```

then we can use `createElement` with the attribute value in the code where we currently do `new AddressOption()`

::: browser/components/payments/res/components/rich-select.js:16
(Diff revision 1)
>      return [
> -      "open",

FYI You need to update existing mochitests which depend on this property and it would probably be good to do that with this line in a separate commit before this commit.

::: browser/components/payments/res/components/rich-select.js
(Diff revision 1)
> -  get selectedOption() {
> -    return this.popupBox.querySelector(":scope > [selected]");
> -  }
> -
> -  /**
> -   * This is the only supported method of changing the selected option. Do not
> -   * manipulate the `selected` property or attribute on options directly.
> -   * @param {HTMLOptionElement} option
> -   */
> -  set selectedOption(option) {

We may want to have something like this still which can just forward to the equivalent API on <select> so outside consumers don't need to all know about `popupBox` and you'd have less tests to change I think.

::: browser/components/payments/res/containers/address-picker.js:105
(Diff revision 1)
> -        optionEl = new AddressOption();
> +        optionEl = document.createElement("option");
>          optionEl.value = guid;
> +        optionEl.textContent = address.name + ", " + address["street-address"];
> +        optionEl.setAttribute("option-type", "shipping-address");
> +        for (let field in address) {
> +          optionEl.setAttribute(field, address[field]);
> -      }
> +        }

The reason I had suggested making the various rich-option extend from the HTMLOptionElement is so this logic could remain inside the relevant option class rather than move outside to the picker. I think it may still be worth doing but it'll be more clear as the bug starts to handle the other pickers.

::: browser/components/payments/res/containers/address-picker.js
(Diff revision 1)
> -      for (let key of AddressOption.recordAttributes) {
> -        let val = address[key];
> -        if (val) {
> -          optionEl.setAttribute(key, val);
> -        } else {
> -          optionEl.removeAttribute(key);
> -        }

You should revert this loop change as it properly handles removing attributes that get deleted.

::: browser/components/payments/res/containers/address-picker.js:125
(Diff revision 1)
> +    // No option is selected by default initially (See Bug 1443735).
> +    this.dropdown.popupBox.value = "";

This is only true for the shipping address picker
Attachment #8983235 - Flags: feedback?(MattN+bmo) → feedback+
(Reporter)

Comment 3

11 months ago
mozreview-review
Comment on attachment 8983235 [details]
Bug 1463554 - test changes.

https://reviewboard.mozilla.org/r/249104/#review255314

::: browser/components/payments/res/components/rich-select.css:9
(Diff revision 1)
>  rich-select:not([open]) > .rich-select-popup-box {
>    display: none;
>  }
>  
>  rich-select[open] {
>    position: relative;
>  }
>  
>  rich-select[open] > .rich-select-popup-box {
>    box-shadow: 0 0 5px black;
>    position: absolute;
>    z-index: 1;
>  }
>  
>  .rich-select-popup-box > .rich-option[selected] {
>    background-color: #ffa;
>  }

I think you can delete the CSS rules I'm selecting.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8983235 - Flags: review?(MattN+bmo)

Comment 6

11 months ago
mozreview-review
Comment on attachment 8983235 [details]
Bug 1463554 - test changes.

https://reviewboard.mozilla.org/r/249104/#review256394


Code analysis found 6 defects in this patch:
 - 6 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/payments/res/components/rich-select.js:7
(Diff revision 3)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  import ObservedPropertiesMixin from "../mixins/ObservedPropertiesMixin.js";
>  import RichOption from "./rich-option.js";
> +import AddressOption from "./address-option.js";

Error: 'addressoption' is defined but never used. [eslint: no-unused-vars]

::: browser/components/payments/res/components/rich-select.js:42
(Diff revision 3)
>    }
>  
>    handleEvent(event) {
>      switch (event.type) {
> -      case "blur": {
> -        this.onBlur(event);
> +      case "change": {
> +      	this.selectedOption = event.target.value;

Error: Mixed spaces and tabs. [eslint: no-mixed-spaces-and-tabs]

::: browser/components/payments/res/components/rich-select.js:42
(Diff revision 3)
>    }
>  
>    handleEvent(event) {
>      switch (event.type) {
> -      case "blur": {
> -        this.onBlur(event);
> +      case "change": {
> +      	this.selectedOption = event.target.value;

Error: Unexpected tab character. [eslint: no-tabs]

::: browser/components/payments/res/components/rich-select.js:43
(Diff revision 3)
> -      let selectedOption = this.selectedOption;
> -      let next = selectedOption.previousElementSibling;
> -      if (next) {
> -        next.selected = true;
> -        selectedOption.selected = false;
>        	this._dispatchChangeEvent();

Error: Mixed spaces and tabs. [eslint: no-mixed-spaces-and-tabs]

::: browser/components/payments/res/components/rich-select.js:43
(Diff revision 3)
> -      let selectedOption = this.selectedOption;
> -      let next = selectedOption.previousElementSibling;
> -      if (next) {
> -        next.selected = true;
> -        selectedOption.selected = false;
>        	this._dispatchChangeEvent();

Error: Unexpected tab character. [eslint: no-tabs]

::: browser/components/payments/res/containers/payment-method-picker.js:57
(Diff revision 3)
>      for (let [guid, basicCard] of Object.entries(basicCards)) {
>        let optionEl = this.dropdown.getOptionByValue(guid);
>        if (!optionEl) {
> -        optionEl = new BasicCardOption();
> +        optionEl = document.createElement("option");
>          optionEl.value = guid;
> +        optionEl.textContent = basicCard["cc-number"] + ", " + basicCard["cc-exp"] + ", " + basicCard["cc-given-name"] + " " + basicCard["cc-family-name"];

Error: Line 57 exceeds the maximum line length of 100. [eslint: max-len]

Comment 7

11 months ago
mozreview-review
Comment on attachment 8983235 [details]
Bug 1463554 - test changes.

https://reviewboard.mozilla.org/r/249104/#review256396


Code analysis found 6 defects in this patch:
 - 6 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/payments/res/components/rich-select.js:7
(Diff revision 2)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  import ObservedPropertiesMixin from "../mixins/ObservedPropertiesMixin.js";
>  import RichOption from "./rich-option.js";
> +import AddressOption from "./address-option.js";

Error: 'addressoption' is defined but never used. [eslint: no-unused-vars]

::: browser/components/payments/res/components/rich-select.js:42
(Diff revision 2)
>    }
>  
>    handleEvent(event) {
>      switch (event.type) {
> -      case "blur": {
> -        this.onBlur(event);
> +      case "change": {
> +      	this.selectedOption = event.target.value;

Error: Mixed spaces and tabs. [eslint: no-mixed-spaces-and-tabs]

::: browser/components/payments/res/components/rich-select.js:42
(Diff revision 2)
>    }
>  
>    handleEvent(event) {
>      switch (event.type) {
> -      case "blur": {
> -        this.onBlur(event);
> +      case "change": {
> +      	this.selectedOption = event.target.value;

Error: Unexpected tab character. [eslint: no-tabs]

::: browser/components/payments/res/components/rich-select.js:43
(Diff revision 2)
> -      let selectedOption = this.selectedOption;
> -      let next = selectedOption.previousElementSibling;
> -      if (next) {
> -        next.selected = true;
> -        selectedOption.selected = false;
>        	this._dispatchChangeEvent();

Error: Mixed spaces and tabs. [eslint: no-mixed-spaces-and-tabs]

::: browser/components/payments/res/components/rich-select.js:43
(Diff revision 2)
> -      let selectedOption = this.selectedOption;
> -      let next = selectedOption.previousElementSibling;
> -      if (next) {
> -        next.selected = true;
> -        selectedOption.selected = false;
>        	this._dispatchChangeEvent();

Error: Unexpected tab character. [eslint: no-tabs]

::: browser/components/payments/res/containers/payment-method-picker.js:57
(Diff revision 2)
>      for (let [guid, basicCard] of Object.entries(basicCards)) {
>        let optionEl = this.dropdown.getOptionByValue(guid);
>        if (!optionEl) {
> -        optionEl = new BasicCardOption();
> +        optionEl = document.createElement("option");
>          optionEl.value = guid;
> +        optionEl.textContent = basicCard["cc-number"] + ", " + basicCard["cc-exp"] + ", " + basicCard["cc-given-name"] + " " + basicCard["cc-family-name"];

Error: Line 57 exceeds the maximum line length of 100. [eslint: max-len]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 10

11 months ago
mozreview-review
Comment on attachment 8983235 [details]
Bug 1463554 - test changes.

https://reviewboard.mozilla.org/r/249104/#review256404

Here are some initial comments

::: browser/components/payments/res/components/rich-select.css:17
(Diff revision 5)
> +rich-select > select {
> +  opacity: 0;
> +}

Nit: Move this rule up above the rich-option so that the rules are closer to DOM order and it stays closer to the existing rich-select one

::: browser/components/payments/res/components/rich-select.js
(Diff revision 5)
> -  /**
> -   * This is the only supported method of changing the selected option. Do not
> -   * manipulate the `selected` property or attribute on options directly.
> -   * @param {HTMLOptionElement} option
> -   */
> -  set selectedOption(option) {
> -    for (let child of this.popupBox.children) {
> -      child.selected = child == option;
> -    }
> -
>      this.render();
>    }

Why don't we keep this to simply set `this.popupBox.value = option.value`? I don't think we need to duplicate the selectedOption state from the <select> inside the local state of the RichSelect.

::: browser/components/payments/res/containers/shipping-option-picker.js:7
(Diff revision 5)
> +// eslint-disable-next-line no-unused-vars
>  import ShippingOption from "../components/shipping-option.js";

You shouldn't disable this rule, instead just do:
```js
import "../components/shipping-option.js";
```

::: browser/components/payments/res/containers/shipping-option-picker.js:37
(Diff revision 5)
> +        optionEl.textContent = option.label;
>        }
> -      optionEl.label = option.label;
> -      optionEl.amountCurrency = option.amount.currency;
> -      optionEl.amountValue = option.amount.value;
> +
> +      optionEl.setAttribute("label", option.label);
> +      optionEl.setAttribute("amount-currency", option.amount.currency);
> +      optionEl.setAttribute("amount-value", option.amount.value);

This change is why I think we should extend HTMLOptionElement for our RichOption subclasses… then you wouldn't need to change this logic. I'll need to look into removing our custom elements polyfill first though… let me do that.

::: browser/components/payments/res/containers/shipping-option-picker.js:47
(Diff revision 5)
> +      optionEl.setAttribute("amount-value", option.amount.value);
> +
>        desiredOptions.push(optionEl);
>      }
>      let el = null;
> -    while ((el = this.dropdown.popupBox.querySelector(":scope > shipping-option"))) {
> +    while ((el = this.dropdown.popupBox.querySelector(":scope > option"))) {

Not that we use a real <select> we can instead use ….popupBox.options instead of querySelector. The same applies to other places.
(Reporter)

Comment 11

11 months ago
mozreview-review-reply
Comment on attachment 8983235 [details]
Bug 1463554 - test changes.

https://reviewboard.mozilla.org/r/249104/#review256404

> Why don't we keep this to simply set `this.popupBox.value = option.value`? I don't think we need to duplicate the selectedOption state from the <select> inside the local state of the RichSelect.

After discussing, the reason is that .value doesn't dispatch a `change` event so we wouldn't re-render. I suggest we rename the observed attribute to "value" to match <select> since it's no longer an option.
Comment hidden (mozreview-request)
(Reporter)

Comment 13

11 months ago
mozreview-review
Comment on attachment 8983235 [details]
Bug 1463554 - test changes.

https://reviewboard.mozilla.org/r/249104/#review256722

::: browser/components/payments/res/components/rich-select.js
(Diff revision 6)
> - * Note: The only supported way to change the selected option is via the
> - *       `selectedOption` setter.

Bring this comment back but change `selectedOption` to `value`

::: browser/components/payments/res/components/rich-select.js:49
(Diff revision 6)
>     * Only dispatched upon a user-initiated change.
>     */
>    _dispatchChangeEvent() {
>      let changeEvent = document.createEvent("UIEvent");
>      changeEvent.initEvent("change", true, true);
>      this.dispatchEvent(changeEvent);
>    }

I think this may be unused now.

::: browser/components/payments/res/components/rich-select.js:65
(Diff revision 6)
>      if (selectedClone) {
>        selectedClone.remove();
>      }
>  
> -    if (selectedChild) {
> -      selectedClone = selectedChild.cloneNode(false);
> -      selectedClone.removeAttribute("id");
> +    if (this.value) {
> +      let optionType = this.getAttribute("optionType");
> +      selectedClone = document.createElement(optionType);

Can you rename the `selectedClone` variable since it's no longer a clone. Maybe call it `selectedRichOption` (`selectedOption` seemed like it would be confused with the selected <option>).

::: browser/components/payments/res/components/rich-select.js:75
(Diff revision 6)
> +      for (let i = 0; i < attributes.length - 1; i++) {
> +        selectedClone.setAttribute(attributes[i].name, attributes[i].value);
> +      }

We generally prefer for…of loops as they are more readable and avoid off-by-one errors on the bounds:
```js
for (let attribute of attributes) {
  selectedClone.setAttribute(attribute.name, attribute.value);
}
```

::: browser/components/payments/res/components/shipping-option.js:16
(Diff revision 6)
>    static get observedAttributes() {
> -    return RichOption.observedAttributes.concat([
> +    return ([

What is this change for?

::: browser/components/payments/res/containers/payment-method-picker.js:57
(Diff revision 6)
> +        // eslint-disable-next-line max-len
> +        optionEl.textContent = basicCard["cc-number"] + ", " + basicCard["cc-exp"] + ", " + basicCard["cc-given-name"] + " " + basicCard["cc-family-name"];

This is not a good case where you should disable the eslint rule since you can trivially break this into multiple lines. Anyways I think you should move this logic into a static method on `BasicCardOption` to keep the formatting of options (single and multi-line) in one place:
```js
optionEl.textContent = BasicCardOption.formatOptionLabel(basicCard)
```

Due to leaks with disconnectedCallback we still can't switch to using the native custom-elements code so we can subclass HTMLOptionElement yet. We can do the same for the other Rich options too.
Attachment #8983235 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

10 months ago
mozreview-review-reply
Comment on attachment 8983235 [details]
Bug 1463554 - test changes.

https://reviewboard.mozilla.org/r/249104/#review256722

> What is this change for?

This change is not necessary, sorry.
Comment on attachment 8983235 [details]
Bug 1463554 - test changes.

From our discussions the other day it seemed like you were still fixing issues you found with this patch so I'll clear review until you have the fixes.
Attachment #8983235 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8987902 - Attachment is obsolete: true
Attachment #8987902 - Flags: review?(MattN+bmo)
(Assignee)

Updated

10 months ago
Attachment #8987902 - Attachment is obsolete: false
Attachment #8987902 - Attachment is patch: true
Attachment #8987902 - Attachment mime type: text/x-review-board-request → text/plain

Comment 20

10 months ago
mozreview-review
Comment on attachment 8983235 [details]
Bug 1463554 - test changes.

https://reviewboard.mozilla.org/r/249104/#review259830


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/payments/test/browser/browser_address_edit.js:122
(Diff revision 9)
>  
>        let {tempAddresses, savedAddresses} = await PTU.DialogContentUtils.getCurrentState(content);
>        is(Object.keys(tempAddresses).length, 0, "No temporary addresses at the start of test");
>        is(Object.keys(savedAddresses).length, 1, "1 saved address at the start of test");
>  
> -      let picker = content.document
> +      await content.document.querySelector("address-picker[selected-state-key='selectedShippingAddress']").focus();

Error: Line 122 exceeds the maximum line length of 100. [eslint: max-len]

::: browser/components/payments/test/browser/browser_address_edit.js:126
(Diff revision 9)
>  
> -      let picker = content.document
> -                     .querySelector("address-picker[selected-state-key='selectedShippingAddress']");
> -      Cu.waiveXrays(picker).dropdown.click();
> -      Cu.waiveXrays(picker).dropdown.popupBox.children[0].click();
> +      await content.document.querySelector("address-picker[selected-state-key='selectedShippingAddress']").focus();
> +    });
> +
> +    // Select shipping address from dropdown.
> +    await BrowserTestUtils.synthesizeKey("Timothy John Berners-Lee, 32 Vassar Street MIT Room 32-G524", {}, frame);

Error: Line 126 exceeds the maximum line length of 100. [eslint: max-len]
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8987902 - Attachment is obsolete: true
Attachment #8987902 - Attachment is patch: false
(Reporter)

Comment 23

10 months ago
mozreview-review
Comment on attachment 8988002 [details]
Bug 1463554 - Fix auto-selection errors in pickers on the payment summary page.

https://reviewboard.mozilla.org/r/253262/#review259910

::: browser/components/payments/res/containers/address-picker.js:165
(Diff revision 1)
>      let nextState = {
>        page: {
>          id: "address-page",
>        },
> +      selectedStateKey: key,

I don't think it makes sense to have a `selectedStateKey` property at the top-level of the state as is seems like it's value wouldn't apply globally to all relevant containers… `selectedStateKey` is currently picker-specific but it seems like you're making it global but perhaps want it to the page-specific? 

This doesn't feel right to me but maybe if you explain it more in the commit message (after the first line) then I'll understand the idea and could suggest a better property name (assuming the idea works).
Attachment #8988002 - Flags: review?(MattN+bmo)
(Reporter)

Comment 24

10 months ago
mozreview-review
Comment on attachment 8983235 [details]
Bug 1463554 - test changes.

https://reviewboard.mozilla.org/r/249104/#review259912

We discussed the fixes for this patch over Vidyo
Attachment #8983235 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8983235 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 30

10 months ago
mozreview-review
Comment on attachment 8988001 [details]
Bug 1463554 - Use native <option> UI with <rich-select> in the open state.

https://reviewboard.mozilla.org/r/253260/#review260238

Here are some comments from version 2 I had but didn't submit

::: browser/components/payments/res/components/address-option.js:59
(Diff revision 2)
>        this.appendChild(this[`_${name}`]);
>      }
>      super.connectedCallback();
>    }
>  
> +  static formatOptionLabel(address) {

Nit: `formatSingleLineLabel` would perhaps be a better name.

You should also add a stub version of the method to rich-option.js that throws if it's not implemented and add a JSDoc comment there to explain what the method is for.

::: browser/components/payments/res/components/address-option.js:59
(Diff revision 2)
> +  static formatOptionLabel(address) {
> +    return address.name + ", " + address["street-address"];

The comma needs to be localized but we can't do substitutions with DTD so the ideal solution can probably wait until we get Fluent. In the meantime can you use an entity for the comma character and file a bug to do proper substitution (e.g. `%s, %s`) once we have Fluent.

::: browser/components/payments/res/components/rich-select.css:9
(Diff revision 2)
> -rich-select:not([open]) > .rich-select-popup-box {
> -  display: none;
> +rich-select > select {
> +  opacity: 0;

Add a comment explaining why we're hiding the <select>

::: browser/components/payments/res/components/rich-select.js:11
(Diff revision 2)
>   * </rich-select>
> - *
>   * Note: The only supported way to change the selected option is via the

Nit: revert this
(Reporter)

Comment 31

10 months ago
mozreview-review
Comment on attachment 8988001 [details]
Bug 1463554 - Use native <option> UI with <rich-select> in the open state.

https://reviewboard.mozilla.org/r/253260/#review261138

::: browser/components/payments/res/containers/address-picker.js:124
(Diff revisions 2 - 3)
> -    for (let el of this.dropdown.popupBox.options) {
> +    let el = null;
> +    while ((el = this.dropdown.popupBox.querySelector(":scope > option"))) {
>        el.remove();
>      }

I think this is the same as:
```js
this.dropdown.popupBox.textContent = "";
```
(Reporter)

Comment 32

10 months ago
mozreview-review
Comment on attachment 8988002 [details]
Bug 1463554 - Fix auto-selection errors in pickers on the payment summary page.

https://reviewboard.mozilla.org/r/253262/#review261140

::: browser/components/payments/res/containers/address-picker.js:163
(Diff revision 3)
> +    let key = [];
> +    key.push(this.selectedStateKey);
> +
>      let nextState = {
>        page: {
>          id: "address-page",
> +        selectedStateKey: key,

You can inline the array construction in the object literal:
```js
…
selectedStateKey: [this.selectedStateKey],
…
```

::: browser/components/payments/res/containers/basic-card-form.js:151
(Diff revision 3)
>        }
> +
>        // Adding a new record: default persistence to checked when in a not-private session

Nit: revert this

::: browser/components/payments/res/containers/basic-card-form.js:285
(Diff revision 3)
>      // overwrite the unmasked card number with the masked one.
>      if (!editing) {
>        record["cc-number"] = record["cc-number"] || "";
>      }
>  
> +    record.billingAddressGUID = this.form.querySelector("#billingAddressGUID").value;

`this.formHandler.buildFormObject` should have already created a record object with the proper `billingAddressGUID`. If not, that should be fixed I think.
Attachment #8988002 - Flags: review?(MattN+bmo)
(Reporter)

Comment 33

10 months ago
mozreview-review
Comment on attachment 8988001 [details]
Bug 1463554 - Use native <option> UI with <rich-select> in the open state.

https://reviewboard.mozilla.org/r/253260/#review262062

Clearing review until the next version comes.
Attachment #8988001 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 40

10 months ago
mozreview-review
Comment on attachment 8988001 [details]
Bug 1463554 - Use native <option> UI with <rich-select> in the open state.

https://reviewboard.mozilla.org/r/253260/#review262684

I tested the patches and the results is quite awesome. This approach will work well… it's getting very close.

::: browser/components/payments/res/components/address-option.js:60
(Diff revision 4)
>      }
>      super.connectedCallback();
>    }
>  
> +  static formatSingleLineLabel(address) {
> +    return address.name + ", " + address["street-address"];

Are you going to fix the "null"/"undefined"/"" case in https://bugzilla.mozilla.org/show_bug.cgi?id=1463545 (see comment 1 there).

::: browser/components/payments/res/components/rich-select.css:10
(Diff revision 4)
> -  display: none;
> -}
> -
> + *The HTML select element is hidden and placed on the rich-option
> + *element to make it look like clicking on the rich-option element
> + *in the closed state opens the HTML select dropdown. */

Nit add spaces after "*"

::: browser/components/payments/res/components/rich-select.css:19
(Diff revision 4)
> -  background-color: #ffa;
>  }
>  
>  .rich-option {
>    display: grid;
>    border-bottom: 1px solid #ddd;

I think border-bottom isn't necessary as that was for separating options in the dropdown I think

::: browser/components/payments/res/components/rich-select.js:49
(Diff revision 4)
>    handleEvent(event) {
>      switch (event.type) {
> -      case "blur": {
> -        this.onBlur(event);
> +      case "change": {
> +        this.value = event.target.value;
>          break;
>        }

Hmm… this doesn't make sense to me… `this.value` would already be the same as the <select>.value via the `value` getter. Is this even doing anything useful? it definitely needs a comment if it is. Is this just useful for calling .render?

::: browser/components/payments/res/components/rich-select.js:59
(Diff revision 4)
> -
> -    return true;
> -  }
> -
>    render() {
> -    let selectedChild;
> +    let selectedRichOption = this.querySelector(":scope > .rich-select-selected-clone");

I still think you should change the `.rich-select-selected-clone` class to `.rich-select-selected-option` since it's not a clone.

::: browser/components/payments/res/components/rich-select.js:69
(Diff revision 4)
> +      let attributes = option.attributes;
> +      for (let attribute of attributes) {
> +        selectedRichOption.setAttribute(attribute.name, attribute.value);

I think you shoud only duplicate the observedAttributes, not all attributes. Are there some that are missing from that list that you need? Maybe do observedAttributes + a custom list instead? I think I mentioned it in a previous version but you're not handling removing of attributes still. e.g. rich-option has @foo but the selected <option> doesn't have @foo at all… you should be deleting @foo from rich-option then.

::: browser/components/payments/res/components/shipping-option.js:41
(Diff revision 4)
> +  static formatSingleLineLabel(option) {
> +    return option.label;

This would be a regression since we definitely need to show the price in the dropdown.

::: browser/components/payments/res/containers/address-picker.js
(Diff revision 4)
> -    if (selectedAddressGUID && !optionWithGUID) {
> -      throw new Error(`${this.selectedStateKey} option ${selectedAddressGUID}` +
> -                      `does not exist in options`);
> -    }

Can you put the errors back in all cases but updated them to work with thew code:
```js
if (selectedAddressGUID && selectedAddressGUID != this.dropdown.value) {
…
```

This applies to all the pickers. I think it's useful to find bugs in our code.

::: browser/components/payments/res/containers/payment-method-picker.js:108
(Diff revision 4)
> -      case this.dropdown: {
> -        stateChange[selectedKey] = target.selectedOption && target.selectedOption.guid;
> +      case this.dropdown.popupBox: {
> +        stateChange[selectedKey] = this.dropdown.value;
>          // Select the security code text since the user is likely to edit it next.
>          // We don't want to do this if the user simply blurs the dropdown.
>          this.securityCodeInput.select();
>          break;
>        }

I thought you were going to remove this and file a follow-up instead? It doesn't seem to work currently.

If you remove it, also remove
```js
this.dropdown.addEventListener("change", this);
```

::: browser/components/payments/res/containers/shipping-option-picker.js:38
(Diff revision 4)
> -      optionEl.amountCurrency = option.amount.currency;
> -      optionEl.amountValue = option.amount.value;
> +      optionEl.setAttribute("label", option.label);
> +      optionEl.setAttribute("amount-currency", option.amount.currency);
> +      optionEl.setAttribute("amount-value", option.amount.value);

Can you do this the same way it's done for the other pickers using a loop over recordAttributes?

::: browser/components/payments/res/containers/shipping-option-picker.js:65
(Diff revision 4)
>    onChange(event) {
> -    let select = event.target;
> +    let selectedOptionId = this.dropdown.value;
> -    let selectedOptionId = select.selectedOption && select.selectedOption.value;
>      this.requestStore.setState({
>        selectedShippingOption: selectedOptionId,

Just a thought… I wonder if there will be a case with any of the pickers where the picker visually gets cleared but the state still has the old value because a change event didn't happen?
Attachment #8988001 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 45

10 months ago
mozreview-review
Comment on attachment 8988860 [details]
Bug 1463554 - Fix the tests that are broken due to change in the internal structure of rich-select.

https://reviewboard.mozilla.org/r/254016/#review262694

::: browser/components/payments/res/containers/address-picker.js:102
(Diff revision 3)
>      let filteredAddresses = this.filterAddresses(addresses, fieldNames);
> -
>      for (let [guid, address] of Object.entries(filteredAddresses)) {

tiny nit: it would be nicer to fix this in the first commit when you were already touching this file.

::: browser/components/payments/res/containers/payment-method-picker.js:107
(Diff revision 3)
>      switch (target) {
>        case this.dropdown.popupBox: {
>          stateChange[selectedKey] = this.dropdown.value;
> -        // Select the security code text since the user is likely to edit it next.
> -        // We don't want to do this if the user simply blurs the dropdown.
> -        this.securityCodeInput.select();

Ignore my comment in commit 1 about this not working, I didn't realize this test commit was changing this part too… this change should be moved to part 1.

::: browser/components/payments/test/browser/head.js:494
(Diff revision 3)
>    }, {card: aCard, options: aOptions});
>  }
> +
> +// The JSDoc validator does not support @returns tags in abstract functions or
> +// star functions without return statements.
> +/* eslint-disable valid-jsdoc */

This will disable it for the whole file IIRC which is not good.
(Reporter)

Comment 46

10 months ago
mozreview-review
Comment on attachment 8988002 [details]
Bug 1463554 - Fix auto-selection errors in pickers on the payment summary page.

https://reviewboard.mozilla.org/r/253262/#review262704

::: browser/components/payments/res/containers/address-picker.js:163
(Diff revision 5)
> +        selectedStateKey: [this.selectedStateKey],
>        },
>        "address-page": {
>          addressFields: this.getAttribute("address-fields"),
> -        selectedStateKey: this.selectedStateKey,

Can you add this property to the mixin (as a comment) to document which objects it's supported on. That could have probably avoided this mistake from happening.
Attachment #8988002 - Flags: review?(MattN+bmo) → review+
(Reporter)

Comment 47

10 months ago
mozreview-review
Comment on attachment 8990440 [details]
Bug 1463554 - Position the HTML select dropdown exactly on the rich option element.

https://reviewboard.mozilla.org/r/255516/#review262706

::: browser/components/payments/res/components/rich-select.css:12
(Diff revision 3)
> +rich-select:focus-within {
> +  outline: 0.2px dashed blue;

Where did you get the 0.2px and the colour "blue" from?
(Reporter)

Comment 48

10 months ago
mozreview-review
Comment on attachment 8988001 [details]
Bug 1463554 - Use native <option> UI with <rich-select> in the open state.

https://reviewboard.mozilla.org/r/253260/#review262708

Clearing review since re-review wasn't intentionally requested
Attachment #8988001 - Flags: review?(MattN+bmo)
(Assignee)

Comment 49

10 months ago
mozreview-review-reply
Comment on attachment 8988001 [details]
Bug 1463554 - Use native <option> UI with <rich-select> in the open state.

https://reviewboard.mozilla.org/r/253260/#review262684

> Are you going to fix the "null"/"undefined"/"" case in https://bugzilla.mozilla.org/show_bug.cgi?id=1463545 (see comment 1 there).

Yes.

> Hmm… this doesn't make sense to me… `this.value` would already be the same as the <select>.value via the `value` getter. Is this even doing anything useful? it definitely needs a comment if it is. Is this just useful for calling .render?

Yes.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 54

9 months ago
mozreview-review
Comment on attachment 8988001 [details]
Bug 1463554 - Use native <option> UI with <rich-select> in the open state.

https://reviewboard.mozilla.org/r/253260/#review263240

::: browser/components/payments/res/components/address-option.js:59
(Diff revision 6)
> +  static formatSingleLineLabel(address) {
> +    return address.name + ", " + address["street-address"];
> +  }

Can you change this to use:
```js
PaymentDialogUtils.getAddressLabel(address);
```
like we do for the billing address. That will use consistent rules for determining the 2nd piece of data to show. You will probably also need the following at the top:

/* import-globals-from ../unprivileged-fallbacks.js */
(Reporter)

Comment 55

9 months ago
mozreview-review
Comment on attachment 8988001 [details]
Bug 1463554 - Use native <option> UI with <rich-select> in the open state.

https://reviewboard.mozilla.org/r/253260/#review263246

Looks good!

::: browser/components/payments/res/components/rich-select.js:51
(Diff revision 6)
> -      case "blur": {
> -        this.onBlur(event);
> +      case "change": {
> +        this.render();

Can you add a comment explaining why we need an explicit render e.g.:

Since the `render` function depends on the popupBox's value, we need to re-render if the value changes.

::: browser/components/payments/res/components/rich-select.js:72
(Diff revision 6)
> +
> +      let option = this.getOptionByValue(this.value);
> +      let attributeNames = selectedRichOption.constructor.recordAttributes;
> +      for (let attributeName of attributeNames) {
> +        let attributeValue = option.getAttribute(attributeName);
> +        selectedRichOption.setAttribute(attributeName, attributeValue);

You should handle calling `removeAttribute` instead if `attributeValue` is falsy like we do in other places: https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/browser/components/payments/res/containers/address-picker.js#112-116

::: browser/components/payments/res/components/shipping-option.js:47
(Diff revision 6)
> +    amount.setAttribute("value", option.amount.value);
> +    amount.setAttribute("currency", option.amount.currency);

Can you use the setter properties instead of the attribute? You should almost always prefer them. They handle removing if the argument is falsy in this case and can potentially do other kinds of data validation in the setter whereas the setAttribute can't do any validation.

::: browser/components/payments/res/components/shipping-option.js:51
(Diff revision 6)
> +    let amount = new CurrencyAmount();
> +    amount.setAttribute("value", option.amount.value);
> +    amount.setAttribute("currency", option.amount.currency);
> +
> +    let name = document.createElement("span");
> +    name.textContent = ", " + option.label;

To match the spec, there shouldn't be a comma and you probably don't need a span at all:
```js
singleLineLabel.append(amount, " ", option.label)
```

::: browser/components/payments/res/components/shipping-option.js:53
(Diff revision 6)
> +    let singleLineLabel = document.createDocumentFragment();
> +    singleLineLabel.appendChild(amount);
> +    singleLineLabel.appendChild(name);
> +
> +    return singleLineLabel;

This method should return a string, not a DocumentFragment so return `singleLineLabel.textContent`.

::: browser/components/payments/res/containers/shipping-option-picker.js:57
(Diff revision 6)
>  
>      // Update selectedness after the options are updated
>      let selectedShippingOption = state.selectedShippingOption;
> -    let selectedOptionEl =
> -      this.dropdown.getOptionByValue(selectedShippingOption);
> -    this.dropdown.selectedOption = selectedOptionEl;
> +    this.dropdown.value = selectedShippingOption;
> +
> +    if (selectedShippingOption && selectedShippingOption !== this.dropdown.popupBox.value) {

Remove ".popupBox" here and in the other places since this.dropdown.value is a getter for for this.dropdown.popupBox.value IIUC.

::: browser/components/payments/res/containers/shipping-option-picker.js:59
(Diff revision 6)
>      let selectedShippingOption = state.selectedShippingOption;
> -    let selectedOptionEl =
> -      this.dropdown.getOptionByValue(selectedShippingOption);
> -    this.dropdown.selectedOption = selectedOptionEl;
> -
> -    if (selectedShippingOption && !selectedOptionEl) {
> +    this.dropdown.value = selectedShippingOption;
> +
> +    if (selectedShippingOption && selectedShippingOption !== this.dropdown.popupBox.value) {
> +      throw new Error(`Selection of shipping option ${selectedShippingOption} ` +
> +                      `is not visible in the shipping option picker`);

Revert: s/is not visible/does not exist/
Attachment #8988001 - Flags: review?(MattN+bmo) → review+
(Reporter)

Comment 56

9 months ago
mozreview-review
Comment on attachment 8990440 [details]
Bug 1463554 - Position the HTML select dropdown exactly on the rich option element.

https://reviewboard.mozilla.org/r/255516/#review263250

::: commit-message-7a181:1
(Diff revision 4)
> +Position the HTML select dropdown exactly on the rich option element. r?MattN

The commit message is missing the "Bug 1463554 - " portion.

::: browser/components/payments/res/components/rich-select.css:13
(Diff revision 4)
> +}
> +
> +/* Focusing on the underlying select element outlines the outer
> +rich-select wrapper making it appear like rich-select is focused. */
> +rich-select:focus-within {
> +  outline: 0.2px dashed;

This should be 1px I think
Attachment #8990440 - Flags: review?(MattN+bmo) → review+
(Reporter)

Comment 57

9 months ago
mozreview-review
Comment on attachment 8988001 [details]
Bug 1463554 - Use native <option> UI with <rich-select> in the open state.

https://reviewboard.mozilla.org/r/253260/#review263274

::: browser/components/payments/res/components/rich-select.js:36
(Diff revision 6)
> -  get selectedOption() {
> -    return this.popupBox.querySelector(":scope > [selected]");
> +  get value() {
> +    return this.popupBox.value;
> -  }

Rather than changing tests to reach into the `popupBox.selectedOptions[0]`, I think it would be fine to put this getter back:
```js
get selectedOption() {
  return this.popupBox.selectedOptions[0];
}
```
(Reporter)

Comment 58

9 months ago
mozreview-review
Comment on attachment 8988860 [details]
Bug 1463554 - Fix the tests that are broken due to change in the internal structure of rich-select.

https://reviewboard.mozilla.org/r/254016/#review263270

::: commit-message-e2d9d:1
(Diff revision 5)
> +Fix the tests.r?MattN

Please improve this commit message to explain what is changing and include the bug number.

::: browser/components/payments/test/PaymentTestUtils.jsm:119
(Diff revision 5)
>  
> -    selectShippingAddressByCountry: country => {
> +    selectShippingAddressByCountry: async (country) => {
>        let doc = content.document;

I guess we don't need this to be async anymore?

::: browser/components/payments/test/browser/browser_address_edit.js:127
(Diff revision 5)
> +    // eslint-disable-next-line max-len
> +    BrowserTestUtils.synthesizeKey("Timothy John Berners-Lee, 32 Vassar Street MIT Room 32-G524", {}, frame);

Don't hardcode the text and then you wouldn't need to disable the `max-len` rule. You only need to synthesize a unique prefix, not the whole label. You can reference `.name` of `PTU.Addresses.TimBL` and/or  from `savedAddresses`.

::: browser/components/payments/test/browser/browser_shippingaddresschange_error.js:13
(Diff revision 5)
>        await setupPaymentDialog(browser, {
>          methodData: [PTU.MethodData.basicCard],
>          details: Object.assign({},
>                                 PTU.Details.twoShippingOptions,
>                                 PTU.Details.total2USD),
>          options: PTU.Options.requestShippingOption,
>          merchantTaskFn: PTU.ContentTasks.createAndShowRequest,
>        }
>      );
>  
> +    await injectEventUtilsInContentTask(frame);

Why didn't you put `injectEventUtilsInContentTask` inside `setupPaymentDialog` instead of duplicating it everywhere?

::: browser/components/payments/test/browser/head.js:524
(Diff revision 5)
> +    EventUtils.synthesizeClick = element => new Promise(resolve => {
> +      element.addEventListener("click", function() {
> +        resolve();
> +      }, {once: true});
> +
> +      EventUtils.synthesizeMouseAtCenter(element,
> +        { type: "mousedown", isSynthesized: false }, content);
> +      EventUtils.synthesizeMouseAtCenter(element,
> +        { type: "mouseup", isSynthesized: false }, content);
> +    });

Since you're not using `synthesizeClick` I think you should remove it. Also please file a follow-up in Testing::Mochitest to make EventUtils work in all content tasks.

::: browser/components/payments/test/mochitest/test_address_form.html:161
(Diff revision 5)
>        "family-name": "Swaj",
> +      "additional-name": "",
>        "organization": "Allizom",

Why does this need to change? I don't see how it's related to this bug.

::: browser/components/payments/test/mochitest/test_address_picker.html:79
(Diff revision 5)
>        "48bnds6854t": {
>          // Same GUID, different values to trigger an update
> -        "address-level1": "MI-edit",
> -        // address-level2 was cleared which means it's not returned
> +        "address-level1": "MI",
> +        "address-level2": "Some City",
>          "country": "CA",
>          "guid": "48bnds6854t",

Same question here… it seems like this change is hiding a regression. Why is the new version expected?

::: browser/components/payments/test/mochitest/test_basic_card_form.html:130
(Diff revision 5)
>        "cc-number": "4111111111111111",
> +      "billingAddressGUID": "",

Users who have saved cards from before we added this property to storage will be in the situation that the tests was testing so if there was a problem then it needs to be fixed, the test shouldn't change.

::: browser/components/payments/test/mochitest/test_shipping_option_picker.html:176
(Diff revision 5)
>  
>    await stateChangedPromise;
>    let options = picker1.dropdown.popupBox.children;
>    is(options.length, 1, "Check dropdown has one remaining address");
>    ok(options[0].textContent.includes("Tortoise"), "Check remaining address");
> -  is(picker1.dropdown.selectedOption, options[0], "Tortoise selected by default");
> +  is(picker1.dropdown.popupBox.selectedOptions[0], options[0], "Tortoise selected by default");

See my comment on part 1 about putting back the `selectedOption` getter so the tests don't need to konw about implementation details.
Attachment #8988860 - Flags: review?(MattN+bmo)
Duplicate of this bug: 1474896
(Reporter)

Comment 60

9 months ago
mozreview-review
Comment on attachment 8988001 [details]
Bug 1463554 - Use native <option> UI with <rich-select> in the open state.

https://reviewboard.mozilla.org/r/253260/#review263566

::: browser/components/payments/res/components/basic-card-option.css:13
(Diff revision 6)
>  rich-select[open] > .rich-select-popup-box > basic-card-option {
>    grid-template-areas:
>      "cc-name   type"
>      "cc-number  cc-exp";
>  }

Can you also delete this rule that will be obsolete after your patch due to [open] not existing.
(Reporter)

Comment 61

9 months ago
mozreview-review
Comment on attachment 8988001 [details]
Bug 1463554 - Use native <option> UI with <rich-select> in the open state.

https://reviewboard.mozilla.org/r/253260/#review263572

Not trying to pile on on there are two related trivial fixes that would be nice to have:

::: browser/components/payments/res/components/basic-card-option.css:8
(Diff revision 6)
>    grid-template-areas:
>      "cc-name  type"
>      "cc-number ...";

You could also fix this to:
```css
grid-template-areas: "type cc-number cc-name cc-exp";
```

::: browser/components/payments/res/components/basic-card-option.css:42
(Diff revision 6)
> -rich-select > .rich-select-selected-clone > .cc-exp {
> +rich-select > .rich-select-selected-option > .cc-exp {
>    display: none;
>  }

and delete this and move the `display: none` to the `basic-card-option > .type` rule above.
(Reporter)

Comment 62

9 months ago
mozreview-review
Comment on attachment 8988001 [details]
Bug 1463554 - Use native <option> UI with <rich-select> in the open state.

https://reviewboard.mozilla.org/r/253260/#review263574

Sorry, I'm just building on top of your patch so I'm noticing more small things.

::: browser/components/payments/res/components/basic-card-option.js:45
(Diff revision 6)
> +  static formatSingleLineLabel(basicCard) {
> +    return basicCard["cc-number"] + ", " + basicCard["cc-exp"] + ", " + basicCard["cc-name"];

Nit: can you get rid of the commas
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 67

9 months ago
mozreview-review
Comment on attachment 8988001 [details]
Bug 1463554 - Use native <option> UI with <rich-select> in the open state.

https://reviewboard.mozilla.org/r/253260/#review263582

::: browser/components/payments/res/components/basic-card-option.js:46
(Diff revisions 6 - 7)
>      }
>      super.connectedCallback();
>    }
>  
>    static formatSingleLineLabel(basicCard) {
> -    return basicCard["cc-number"] + ", " + basicCard["cc-exp"] + ", " + basicCard["cc-name"];
> +    return basicCard["cc-number"] + basicCard["cc-exp"] + basicCard["cc-name"];

You deleted the spaces too instead of only the commas.

::: browser/components/payments/res/components/rich-select.js:83
(Diff revisions 6 - 7)
>        for (let attributeName of attributeNames) {
>          let attributeValue = option.getAttribute(attributeName);
> +        if (attributeValue) {
> -        selectedRichOption.setAttribute(attributeName, attributeValue);
> +          selectedRichOption.setAttribute(attributeName, attributeValue);
> +        } else {
> +          selectedRichOption.removeAttribute(attributeName, attributeValue);

There is no 2nd argument to removeAttribute

::: browser/components/payments/res/components/shipping-option.js:51
(Diff revisions 6 - 7)
> +    let singleLineLabel;
> +    singleLineLabel = amount.textContent + " " + option.label;
>      return singleLineLabel;

Nit: There's no need for the temporary variable

::: browser/components/payments/res/containers/shipping-option-picker.js:42
(Diff revisions 6 - 7)
>        let optionElTextContent = ShippingOption.formatSingleLineLabel(option);
> -      optionEl.textContent = "";
> +      optionEl.textContent = optionElTextContent;

Nit: no need for a temporary variable here either
(Reporter)

Comment 68

9 months ago
mozreview-review
Comment on attachment 8988860 [details]
Bug 1463554 - Fix the tests that are broken due to change in the internal structure of rich-select.

https://reviewboard.mozilla.org/r/254016/#review263580

I still need to review test_payer_address_picker.html and test_rich_select.html more closely.

::: browser/components/payments/test/browser/browser_address_edit.js:125
(Diff revision 6)
>        is(Object.keys(savedAddresses).length, 1, "1 saved address at the start of test");
>  
>        let picker = content.document
>                       .querySelector("address-picker[selected-state-key='selectedShippingAddress']");
> -      Cu.waiveXrays(picker).dropdown.click();
> -      Cu.waiveXrays(picker).dropdown.popupBox.children[0].click();
> +      Cu.waiveXrays(picker).dropdown.popupBox.focus();
> +      EventUtils.synthesizeKey("Timothy", {}, content.window);

I meant to not hard-code a string at all i.e. get Timothy by looking it up from PTU.Addresses.TimBL.…

::: browser/components/payments/test/browser/head.js:267
(Diff revision 6)
>    info("helper functions injected into frame");
>  
> +  await injectEventUtilsInContentTask(frame);

Nit: put this before the `info` since it applies to event utils too

::: browser/components/payments/test/mochitest/test_address_picker.html:72
(Diff revision 6)
> -  ok(options[0].textContent.includes("123 Sesame Street"), "Check first address");
> -  ok(options[1].textContent.includes("P.O. Box 123"), "Check second address");
> +  ok(options[0].textContent.includes("Mr. Foo"), "Check first address");
> +  ok(options[1].textContent.includes("Mrs. Bar"), "Check second address");

What is this change for?

::: browser/components/payments/test/mochitest/test_rich_select.html:62
(Diff revision 6)
> +  "68gjdh354j": {
> +    "email": "johnz9382@email.com",
> +    "name": "John Zembrano",
> +    "street-address": "42 Fairydust Lane",
> +    "address-level2": "Lala Land",
> +    "missinginformation": "true",

I think this line is not used and can be deleted.

::: browser/components/payments/test/mochitest/test_shipping_option_picker.html:78
(Diff revision 6)
>    let options = picker1.dropdown.popupBox.children;
>    is(options.length, 2, "Check dropdown has both options");
>    ok(options[0].textContent.includes("Carrier Pigeon"), "Check first option");
> -  is(options[0].querySelector(".amount").currency, "USD", "Check currency");
> +  is(options[0].getAttribute("amount-currency"), "USD", "Check currency");
>    ok(options[1].textContent.includes("Lightspeed (default)"), "Check second option");
> -  is(picker1.dropdown.selectedOption, options[1], "Lightspeed selected by default");
> +  is(picker1.dropdown.popupBox.selectedOptions[0], options[1], "Lightspeed selected by default");

Please do a search and replace in the directory to find other unnecessary changes from `.dropdown.selectedOption` to `.dropdown.popupBox.selectedOptions[0]`
(Reporter)

Comment 69

9 months ago
mozreview-review
Comment on attachment 8988001 [details]
Bug 1463554 - Use native <option> UI with <rich-select> in the open state.

https://reviewboard.mozilla.org/r/253260/#review263806

::: browser/components/payments/res/containers/address-picker.js:25
(Diff revision 7)
>  
>    constructor() {
>      super();
>      this.dropdown = new RichSelect();
>      this.dropdown.addEventListener("change", this);
> +    this.dropdown.setAttribute("optionType", "address-option");

Sorry for the last minute change but really `optionType` should be `option-type` as attributes in HTML are case-insensitive and the hyphen syntax is what we normally use for node attributes in the dialog.
(Reporter)

Comment 70

9 months ago
mozreview-review
Comment on attachment 8988860 [details]
Bug 1463554 - Fix the tests that are broken due to change in the internal structure of rich-select.

https://reviewboard.mozilla.org/r/254016/#review263802

::: browser/components/payments/test/mochitest/test_payer_address_picker.html:182
(Diff revision 6)
>      selectedPayerAddress: "48bnds6854t",
>    });
>  
>    await asyncElementRendered();
>  
> -  let visibleOptions = getVisiblePickerOptions(elPicker);
> +  let richOption = elPicker.dropdown.querySelector("address-option");

This should use ".rich-select-selected-option" since that's actually what you carea about so it doesn't break when we convert the <option> to also be <address-option> later. The same issue apples in a few places in this file.

I would also rename the `richOption` variable for the same reason. Maybe `closedRichOption`?

::: browser/components/payments/test/mochitest/test_rich_select.html
(Diff revision 6)
> -      <!-- the class="rich-option" is required to be hard-coded due to a bug with the custom
> -           elements polyfill causing the address-option constructor to happen too late. -->
> -      <address-option id="option1"
> -                      class="rich-option"
> -                      email="emzembrano92@email.com"
> -                      name="Emily Zembrano"
> -                      street-address="717 Hyde Street #6"
> -                      address-level2="San Francisco"

I agree with you that this probably shouldn't be testing with address-option or basic-card-option as they are too complicated. We should switch to <shipping-option> instead to reduce the boilerplate and distraction from the core <rich-select> parts of the test but I'll leave that as optional cleanup.

::: browser/components/payments/test/mochitest/test_rich_select.html:97
(Diff revision 6)
>  add_task(async function test_streetAddress_combines_street_level2_level1_postalCode_country() {
>    ok(option1, "option1 exists");
> -  let streetAddress = option1.querySelector(".street-address");
> -  /* eslint-disable max-len */
> -  is(streetAddress.textContent,
> -     `${option1.streetAddress} ${option1.addressLevel2} ${option1.addressLevel1} ${option1.postalCode} ${option1.country}`);
> -  /* eslint-enable max-len */
> -});
> -
> -add_task(async function test_no_option_selected() {
> -  ok(select1, "select1 exists");
>  
> +  select1.popupBox.focus();
> +  synthesizeKey(option2.textContent, {});
>    await asyncElementRendered();
>  
> -  is_hidden(option1, "option 1 should be hidden when popup is not open");
> -  is_hidden(option2, "option 2 should be hidden when popup is not open");
> -  is_hidden(option3, "option 3 should be hidden when popup is not open");
> -  ok(!option1.selected, "option 1 should not be selected");
> -  ok(!option1.hasAttribute("selected"), "option 1 should not have selected attribute");
> -  let selectedClone = get_selected_clone();
> +  let richoption1 = document.querySelector("address-option");
> +  let streetAddress = richoption1.querySelector(".street-address");
> +  /* eslint-disable max-len */
> +  is(streetAddress.textContent,
> +     `${option2.getAttribute("street-address")} ${option2.getAttribute("address-level2")} ${option2.getAttribute("address-level1")} ${option2.getAttribute("postal-code")} ${option2.getAttribute("country")}`);
> +  /* eslint-enable max-len */
> -  is_visible(selectedClone, "The selected clone should be visible at all times");
> -  ok(selectedClone.classList.contains("rich-option"), "Default option should be a rich-option");
> -  ok(selectedClone.textContent.includes("None selected"), "Check default text");
>  });

As we discussed, this really should move to test_address_picker.html

::: browser/components/payments/test/mochitest/test_rich_select.html:114
(Diff revision 6)
> -  ok(!select1.open, "select is not open by default");
> -  ok(!option1.selected, "option 1 should not be selected by default");
> +  select1.popupBox.focus();
> +  synthesizeKey(option1.textContent, {});
> -
> -  select1.click();
> -
> -  ok(select1.open, "select is open after clicking on it");
> -  is(select1.selectedOption, null, "No selected option when open");
> -  ok(!option1.selected, "option 1 should not be selected when open");
> -  is_visible(option1, "option 1 is visible when select is open");
> -  is_visible(option2, "option 2 is visible when select is open");
> -  is_visible(option3, "option 3 is visible when select is open");
> -
> -  option2.click();
> -
> -  ok(!select1.open, "select is not open after blur");
> -  ok(!option1.selected, "option 1 is not selected after click on option 2");
> -  ok(option2.selected, "option 2 is selected after clicking on it");
> -  is_hidden(option1, "option 1 is hidden when select is closed");
> -  is_hidden(option2, "option 2 is hidden when select is closed");
> -  is_hidden(option3, "option 3 is hidden when select is closed");
> -
>    await asyncElementRendered();
> +  ok(option1.selected, "option 1 is now selected");
>  
>    let selectedClone = get_selected_clone();
>    is_visible(selectedClone, "The selected clone should be visible at all times");
> -  is(selectedClone.getAttribute("email"), option2.getAttribute("email"),
> +  is(selectedClone.getAttribute("email"), option1.getAttribute("email"),
>       "The selected clone email should be equivalent to the selected option 2");
> -  is(selectedClone.getAttribute("name"), option2.getAttribute("name"),
> +  is(selectedClone.getAttribute("name"), option1.getAttribute("name"),
>       "The selected clone name should be equivalent to the selected option 2");
>  });
>  
> -add_task(async function test_changing_option_selected_affects_other_options() {
> -  ok(option2.selected, "Option 2 should be selected from prior test");
> +add_task(async function test_multi_select_not_supported_in_dropdown() {
> +  ok(option1.selected, "Option 1 should be selected from prior test");

Why the change from option 2 to option 1?
Attachment #8988860 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 75

9 months ago
mozreview-review
Comment on attachment 8988860 [details]
Bug 1463554 - Fix the tests that are broken due to change in the internal structure of rich-select.

https://reviewboard.mozilla.org/r/254016/#review263834

Thanks
Attachment #8988860 - Flags: review?(MattN+bmo) → review+
(Reporter)

Comment 76

9 months ago
mozreview-review
Comment on attachment 8990440 [details]
Bug 1463554 - Position the HTML select dropdown exactly on the rich option element.

https://reviewboard.mozilla.org/r/255516/#review263844

::: browser/components/payments/res/components/rich-select.css:13
(Diff revision 6)
> +}
> +
> +/* Focusing on the underlying select element outlines the outer
> +rich-select wrapper making it appear like rich-select is focused. */
> +rich-select:focus-within {
> +  outline: 1px dashed;

Just realized this is supposed to be dotted, not dashed
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 85

9 months ago
Pushed by prathikshaprasadsuman@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/587bc807f560
Use native <option> UI with <rich-select> in the open state. r=MattN
https://hg.mozilla.org/integration/autoland/rev/e47e23fcb698
Fix auto-selection errors in pickers on the payment summary page.r=MattN
https://hg.mozilla.org/integration/autoland/rev/74971b656b92
Fix the tests that are broken due to change in the internal structure of rich-select.r=MattN
https://hg.mozilla.org/integration/autoland/rev/3acee2d5cd9d
Position the HTML select dropdown exactly on the rich option element.r=MattN
You need to log in before you can comment on or make changes to this bug.