Implement a multi-line PaymentRequest shipping address picker

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: MattN, Assigned: MattN)

Tracking

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

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [webpayments])

Attachments

(10 attachments, 1 obsolete attachment)

59 bytes, text/x-review-board-request
jaws
: review+
Details
59 bytes, text/x-review-board-request
jaws
: review+
Details
59 bytes, text/x-review-board-request
jaws
: review+
Details
59 bytes, text/x-review-board-request
jaws
: review+
Details
59 bytes, text/x-review-board-request
jaws
: review+
Details
59 bytes, text/x-review-board-request
jaws
: review+
Details
59 bytes, text/x-review-board-request
jaws
: review+
Details
59 bytes, text/x-review-board-request
jaws
: review+
Details
59 bytes, text/x-review-board-request
jaws
: review+
Details
59 bytes, text/x-review-board-request
jaws
: review+
Details
Using the data from bug 1387221 and the widget from bug 1422164 we can implement a multi-line shipping address selector according to the UX spec.
Comment on attachment 8944777 [details]
Bug 1427950 - Implement a multi-line PaymentRequest shipping address picker.

Clearing review since Jared told me it wasn't ready
Attachment #8944777 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

a year ago
mozreview-review
Comment on attachment 8944777 [details]
Bug 1427950 - Implement a multi-line PaymentRequest shipping address picker.

https://reviewboard.mozilla.org/r/214940/#review221812

::: commit-message-5e046:1
(Diff revision 5)
> +Bug 1427950 - Implement a multi-line PaymentRequest shipping address picker. r?MattN

Please change the commit message and bug summary to reflect the fact that this is now just about saving the selected address in state

::: toolkit/components/payments/res/components/rich-select.js:152
(Diff revision 5)
>      let selectedChild;
>      for (let child of popupBox.children) {
>        if (child.selected) {
> +        // Only allow one child to be selected at a time.
> +        if (selectedChild) {
> +          selectedChild.selected = false;
> +        }
>          selectedChild = child;
>        }
>      }
>      if (!selectedChild && popupBox.children.length) {
>        selectedChild = popupBox.children[0];
>        selectedChild.selected = true;
>      }

It seems like we have too many places dealing with option selectedness. I'm pretty sure it can be simplified by reducing local state and relying on the  state as the source of truth about which one or zero option is selected. I'll try out an approach locally.

::: toolkit/components/payments/res/containers/address-picker.js:19
(Diff revision 5)
>  
>  class AddressPicker extends PaymentStateSubscriberMixin(HTMLElement) {
>    constructor() {
>      super();
>      this.dropdown = document.createElement("rich-select");
> +    this.dropdown.picker = this;

This is leftover, right?

::: toolkit/components/payments/res/containers/address-picker.js:34
(Diff revision 5)
> +        let state = {};
> +        state[selectedKey] = select.querySelector("[selected]").guid;
> +        this.requestStore.setState(state);

This can all be done in one statement in a way that doesn't reduce readability IMO:
```js
this.requestStore.setState({
  [selectedKey]: select.querySelector("[selected]").guid,
});
```

::: toolkit/components/payments/res/containers/address-picker.js:54
(Diff revision 5)
> +      if (guid == state[this.selectedStateKey]) {
> +        optionEl.setAttribute("selected", "true");
> +      }

Why not have this handle unselecting the other options?  It seems like it could simplify things:
```js
optionEl.selected = guid == selectedGUID;

```

You should generally use setters, instead of setAttribute too.

::: toolkit/components/payments/res/containers/address-picker.js:54
(Diff revision 5)
>          optionEl.guid = guid;
>        }
>        for (let [key, val] of Object.entries(address)) {
>          optionEl.setAttribute(key, val);
>        }
> +      if (guid == state[this.selectedStateKey]) {

You could hoist this lookup outside the loop to help perf a bit:
```js
let selectedGUID = state[this.selectedStateKey];
```
(Assignee)

Comment 10

a year ago
mozreview-review
Comment on attachment 8944777 [details]
Bug 1427950 - Implement a multi-line PaymentRequest shipping address picker.

https://reviewboard.mozilla.org/r/214940/#review222132

I think since there are quite a few related issues that require refactoring things to make this work well it'll be better for me to write separate commits and get you to review them than to get you to fix issues in previous bugs that are now showing. We'll see what's left after my separate commits.

::: toolkit/components/payments/res/components/rich-select.js:96
(Diff revision 5)
>      } else if (event.key == "ArrowDown") {
>        let selectedOption = this.selectedOption;
>        let next = selectedOption.nextElementSibling;
>        if (next) {
>          next.selected = true;
>          selectedOption.selected = false;

We don't want to set the option to be selected until the user closes the popup (at least that's how <select> work on Windows 8 and macOS) and in the case of an address picker this may lead to a privacy leak and a bad experience since we would have to tell the merchant the user's address with every up and down key press plus we're supposed to prevent further changes while we're waiting for an update from the merchant. Which option is "selected" needs to be separated from what happens while the user uses the up+down keys (and also separate from any hover styling).

::: toolkit/components/payments/res/components/rich-select.js:155
(Diff revision 5)
> +        // Only allow one child to be selected at a time.
> +        if (selectedChild) {
> +          selectedChild.selected = false;
> +        }

This doesn't feel right to me since it seems like this will give rendering a rich-select a side-effect of changing an option.
Attachment #8944777 - 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)
Don't be overwhelmed by the 10 commits as you'll see that none are huge, they are just well split up. I may want to add some tests still but I'll need to think about it more. I may as well assign this to myself since https://reviewboard.mozilla.org/r/216604/diff/#index_header covers what your commit did. We still need a follow-up for sending the selected address to the DOM code.
Assignee: jaws → MattN+bmo

Comment 22

a year ago
mozreview-review
Comment on attachment 8946568 [details]
Bug 1427950 - Change rich-select to default to an indeterminate state when no options are selected.

https://reviewboard.mozilla.org/r/216596/#review222276


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

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


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


::: toolkit/components/payments/res/components/rich-select.js:38
(Diff revision 1)
> +        for (let addedNode of mutation.addedNodes) {
> +          if (addedNode.nodeType != Node.ELEMENT_NODE ||
> +              !addedNode.matches(".rich-option:not(.rich-select-selected-clone)")) {
> +            continue;
> +          }
> +          console.log("moved", addedNode);

Error: Unexpected console statement. [eslint: no-console]
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 30

a year ago
mozreview-review
Comment on attachment 8946565 [details]
Bug 1427950 - Add re-render button to PaymentRequest debugging panel.

https://reviewboard.mozilla.org/r/216590/#review222442
Attachment #8946565 - Flags: review?(jaws) → review+

Comment 31

a year ago
mozreview-review
Comment on attachment 8946566 [details]
Bug 1427950 - Have setStateFromParent handle removing deleted GUIDs from the "selected" keys.

https://reviewboard.mozilla.org/r/216592/#review222452

::: toolkit/components/payments/res/debugging.js:143
(Diff revision 1)
>  let buttonActions = {
>    delete1Address() {
>      let savedAddresses = Object.assign({}, requestStore.getState().savedAddresses);
>      delete savedAddresses[Object.keys(savedAddresses)[0]];
> -    requestStore.setState({
> +    // Use setStateFromParent since it ensures there is no dangling
> +    // `selectedShippingAddress` FK reference.

s/FK/foreign key/ since I don't normally see "foreign key" abbreviated like this.
Attachment #8946566 - Flags: review?(jaws) → review+

Comment 32

a year ago
mozreview-review
Comment on attachment 8946567 [details]
Bug 1427950 - Make <rich-select>'s .selectedOption setter the only supported way to change selection.

https://reviewboard.mozilla.org/r/216594/#review222456

::: toolkit/components/payments/res/containers/address-picker.js:44
(Diff revision 1)
> -    this.dropdown.popupBox.append(...desiredOptions);
> +    for (let option of desiredOptions) {
> +      this.dropdown.popupBox.append(option);
> +    }

Why the change away from ...desiredOptions?
Attachment #8946567 - Flags: review?(jaws) → review+

Comment 33

a year ago
mozreview-review
Comment on attachment 8946568 [details]
Bug 1427950 - Change rich-select to default to an indeterminate state when no options are selected.

https://reviewboard.mozilla.org/r/216596/#review222464

::: toolkit/components/payments/res/components/rich-select.js:188
(Diff revision 2)
>          selectedClone = selectedChild.cloneNode(false);
>          selectedClone.removeAttribute("id");
>          selectedClone.removeAttribute("selected");
> +      } else {
> +        selectedClone = document.createElement("rich-option");
> +        selectedClone.textContent = "(None selected)";

We should be pulling this from a locale file. I don't see a bug on the breakdown spreadsheet for l10n or localization work so we should file that now. I'm OK with pushing this to a follow-up.
Attachment #8946568 - Flags: review?(jaws) → review+

Comment 34

a year ago
mozreview-review
Comment on attachment 8946569 [details]
Bug 1427950 - Share common rich-option styles and make rich-select inline-block to improve layout.

https://reviewboard.mozilla.org/r/216598/#review222466
Attachment #8946569 - Flags: review?(jaws) → review+

Comment 35

a year ago
mozreview-review
Comment on attachment 8946570 [details]
Bug 1427950 - Create address and basic-card option cached children in the constructor and re-use them.

https://reviewboard.mozilla.org/r/216600/#review222468
Attachment #8946570 - Flags: review?(jaws) → review+

Comment 36

a year ago
mozreview-review
Comment on attachment 8946571 [details]
Bug 1427950 - Dispatch a "change" event from <rich-select> when a user changes the selectedness.

https://reviewboard.mozilla.org/r/216602/#review222470

::: toolkit/components/payments/res/components/rich-select.js:111
(Diff revision 2)
>      }
> +
> +    let option = event.target.closest(".rich-option");
> +    if (this.open && option && !option.matches(".rich-select-selected-clone")) {
> +      this.selectedOption = option;
> +      this._dispatchChangeEvent();

Are you sure you want to dispatch the change event if the user clicks the already selected option?

.rich-select-selected-clone is not set on the popupBox child that is selected, that class is only set on the clone that exists outside of the popupBox.

I understand if you just wanted to block dispatching "change" for if the user clicks on the clone while the popupBox is open, but I think you should also add a check for the [selected] attribute.

Alternatively, you could only go forward with the change if `event.target.closest(".rich-select-popup-box") && !event.target.closest("rich-option").selected`.

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

I don't see anything in this patch set that notifies the page of the change to the selected address, which is something you asked me to add. Is that still coming?
Attachment #8946571 - Flags: review?(jaws) → review+

Comment 37

a year ago
mozreview-review
Comment on attachment 8946572 [details]
Bug 1427950 - Use the <rich-select> 'change' event to update selectedShippingAddress state.

https://reviewboard.mozilla.org/r/216604/#review222474
Attachment #8946572 - Flags: review?(jaws) → review+

Comment 38

a year ago
mozreview-review
Comment on attachment 8946573 [details]
Bug 1427950 - Only use the mutation observer for migrating new option children to the popup.

https://reviewboard.mozilla.org/r/216606/#review222476
Attachment #8946573 - Flags: review?(jaws) → review+

Comment 39

a year ago
mozreview-review
Comment on attachment 8946574 [details]
Bug 1427950 - Fix test_rich_select.html to handle the indeterminate state with no selection.

https://reviewboard.mozilla.org/r/216608/#review222480

::: toolkit/components/payments/test/mochitest/test_rich_select.html:369
(Diff revision 2)
>    await asyncElementRendered();
>  
> -  is(select3.childElementCount, 1, "There should now be one child");
> +  is(select3.childElementCount, 2, "There should still be two children");
>    is(popupBox.childElementCount, 0, "The popup box should not have any children");
>    clonedOption = select3.querySelector(".rich-select-selected-clone");
> -  ok(!clonedOption, "there should be no cloned option");
> +  todo(clonedOption.textContent.includes("None selected"), "the dropdown should show no selection");

Why is this a todo? I believe this should pass now, right?
Attachment #8946574 - Flags: review?(jaws) → review+

Comment 40

a year ago
mozreview-review-reply
Comment on attachment 8946571 [details]
Bug 1427950 - Dispatch a "change" event from <rich-select> when a user changes the selectedness.

https://reviewboard.mozilla.org/r/216602/#review222470

> I don't see anything in this patch set that notifies the page of the change to the selected address, which is something you asked me to add. Is that still coming?

I'll work on this in a follow-up bug. You can disregard this question.
(Assignee)

Comment 41

a year ago
mozreview-review-reply
Comment on attachment 8946567 [details]
Bug 1427950 - Make <rich-select>'s .selectedOption setter the only supported way to change selection.

https://reviewboard.mozilla.org/r/216594/#review222456

> Why the change away from ...desiredOptions?

Oops, I meant to change to .appendChild to workaround https://github.com/webcomponents/custom-elements/issues/139 

That may be the reason why I had to add `class="rich-option"` in the test… I can maybe revert that now.
(Assignee)

Comment 42

a year ago
mozreview-review-reply
Comment on attachment 8946568 [details]
Bug 1427950 - Change rich-select to default to an indeterminate state when no options are selected.

https://reviewboard.mozilla.org/r/216596/#review222464

> We should be pulling this from a locale file. I don't see a bug on the breakdown spreadsheet for l10n or localization work so we should file that now. I'm OK with pushing this to a follow-up.

Yeah, that's true of other existing strings too. I'll file a bug.
Attachment #8944777 - Attachment is obsolete: true
(Assignee)

Comment 43

a year ago
mozreview-review-reply
Comment on attachment 8946567 [details]
Bug 1427950 - Make <rich-select>'s .selectedOption setter the only supported way to change selection.

https://reviewboard.mozilla.org/r/216594/#review222456

> Oops, I meant to change to .appendChild to workaround https://github.com/webcomponents/custom-elements/issues/139 
> 
> That may be the reason why I had to add `class="rich-option"` in the test… I can maybe revert that now.

> That may be the reason why I had to add class="rich-option" in the test… I can maybe revert that now.

Nope, seems like the issue requiring the predefined `class="rich-option"` in the tests still occurs with .appendChild. We don't hit this issue in our shipping code since we don't have option children pre-defined in any selects.
(Assignee)

Comment 44

a year ago
mozreview-review-reply
Comment on attachment 8946574 [details]
Bug 1427950 - Fix test_rich_select.html to handle the indeterminate state with no selection.

https://reviewboard.mozilla.org/r/216608/#review222480

> Why is this a todo? I believe this should pass now, right?

It doesn't pass since I basically removed this support as I was re-working things. The main change was that I removed `subtree` from the Mutation Observer since it would also observe all the <span> changes inside the options which was wasteful. I could attach a separate mutation observer on the popupBox to handle removals but since we won't need this for PaymentRequest it didn't seem like it was worth reimplementing now.
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 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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 65

a year ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/6a159c9b1773
Add re-render button to PaymentRequest debugging panel. r=jaws
https://hg.mozilla.org/integration/autoland/rev/c74872552dba
Have setStateFromParent handle removing deleted GUIDs from the "selected" keys. r=jaws
https://hg.mozilla.org/integration/autoland/rev/71f9f72b2d85
Make <rich-select>'s .selectedOption setter the only supported way to change selection. r=jaws
https://hg.mozilla.org/integration/autoland/rev/025720084eb9
Change rich-select to default to an indeterminate state when no options are selected. r=jaws
https://hg.mozilla.org/integration/autoland/rev/e4e028b18367
Share common rich-option styles and make rich-select inline-block to improve layout. r=jaws
https://hg.mozilla.org/integration/autoland/rev/edf1ecbd9f44
Create address and basic-card option cached children in the constructor and re-use them. r=jaws
https://hg.mozilla.org/integration/autoland/rev/3793ba14d8ae
Dispatch a "change" event from <rich-select> when a user changes the selectedness. r=jaws
https://hg.mozilla.org/integration/autoland/rev/93987f62f19b
Use the <rich-select> 'change' event to update selectedShippingAddress state. r=jaws
https://hg.mozilla.org/integration/autoland/rev/3605aff2cad6
Only use the mutation observer for migrating new option children to the popup. r=jaws
https://hg.mozilla.org/integration/autoland/rev/e64f835d2425
Fix test_rich_select.html to handle the indeterminate state with no selection. r=jaws
Whiteboard: [webpayments]
Component: WebPayments UI → WebPayments UI
Product: Toolkit → Firefox
Target Milestone: mozilla60 → Firefox 60
You need to log in before you can comment on or make changes to this bug.