add redactList for PaymentAddress (Part 1) #654

marcoscaceres · 2017-11-20T09:08:52Z

The following tasks have been completed:

MDN Docs added

Implementation commitment:

Safari (implemented in ApplePay - redacts)
Chrome - planning.
Firefox - redacts.
Edge - keeping current behavior (no redact)

marcoscaceres · 2017-11-20T09:13:35Z

@domenic, @rsolomakhin, all, please note that this is incomplete, just seeking feedback on approach at this point (you will spot a "To Be Written" algo).

I need to read E.164 spec in details, so to understand a bit more about phone number formats and structure. In any case, let me know what you think!

Renaming PaymentAddress might be "nice" too... but maybe too late, unless we go down the NamedConstructor route 🙀, which I don't think we want.. or we can just live with the crappy name.

rsolomakhin · 2017-11-20T16:53:59Z

index.html

+        </section>
+        <section>
+          <h2>
+            Creating a <code>PaymentAddress</code>


Pardon my noob question. Why do we need both "constructor" and "creating" sections?

Yeah, we might need to merge them... one is supposed to impose order when constructing from user input (should rename this section more clearly), while the other is for calling the constructor from script.

*constructing

I'm going to change this algo to use an AddressInit dictionary instead. The algorithm will also take an optional "excludeList", which will exclude dictionary members as per #648. That should solve for onpaymentaddresschange privacy issues.

rsolomakhin

Overall looks good!

domenic

Found some nits!

domenic · 2017-12-01T01:26:20Z

index.html

+          address for display.
+        </dd>
+        <dt>
+          <dfn>organization</dfn>


The capitalization is inconsistent on these components of an address. I think consistently lowercase would be best.

domenic · 2017-12-01T01:27:22Z

index.html

        </h2>
+        <pre class="idl">
+          [SecureContext, Exposed=(Window,Worker), Constructor(optional AddressInit details)]


Does exposing this in workers make sense?

I think we want MAY want expose it for Payment Hander. I can redact it until we are 100% on the API.

domenic · 2017-12-01T01:27:48Z

index.html

+          [SecureContext, Exposed=(Window,Worker), Constructor(optional AddressInit details)]
+          interface PaymentAddress {
+            [Default] object toJSON();
+            readonly attribute DOMString country;


The order here doesn't match the above, which is pretty confusing.

domenic · 2017-12-01T01:28:17Z

index.html

+          </h2>
+          <p>
+            The steps to <dfn data-lt="PaymentAddress.PaymentAddress()"
+            data-no-default="">construct a <code>PymentAddress</code></dfn>


Typo "PymentAddress"

domenic · 2017-12-01T01:29:01Z

index.html

+            <code><a data-cite=
+            "!WEBIDL#idl-sequence">sequence</a>&lt;<a data-cite=
+            "WEBIDL#idl-DOMString">DOMString</a>&gt;</code>, and all other
+            internal slots set to the empty string.


Do you want to distinguish between null and empty string? Null feels a bit more natural for "unset" fields.

I have a feeling we discussed this before... apologies if so.

We indeed have... I wanted null too :) But then I started playing around with integrating with <form> and the empty string turned out to be a better fit.

domenic · 2017-12-01T01:31:47Z

index.html

+              <ol>
+                <li>Check if <var>details</var>["<a>phone</a>"] is a
+                <a>structurally valid phone number</a>, throw a
+                <a>RangeError</a>.


Presumably only throw the RangeError if it is not a structurally valid phone number. As phrased, this always throws it.

domenic · 2017-12-01T01:31:59Z

index.html

+                <a>RangeError</a>.
+                </li>
+                <li>Set <var>address</var>.<a>[[\phone]]</a> to the result of
+                <a>canonicalize a phone number</a>


canonicalizing the phone number

domenic · 2017-12-01T01:33:26Z

index.html

+              </td>
+              <td>
+                A <a>country</a> as an [[!ISO3166]] alpha-2 code or the empty
+                string. The canonical form is upper case. For example, "JP".


I would phrase this as "stored in its canonical uppercase form"

domenic · 2017-12-01T01:34:25Z

index.html

+            <li>If <var>tel</var> contains U+000A LINE FEED (LF) or U+000D
+            CARRIAGE RETURN (CR) characters, return false.
+            </li>
+            <li>Strip and collapse ASCII whitespace in <var>tel</var>.


Link to https://infra.spec.whatwg.org/#strip-and-collapse-ascii-whitespace

domenic · 2017-12-01T01:34:52Z

index.html

+            </li>
+            <li>Strip and collapse ASCII whitespace in <var>tel</var>.
+            </li>
+            <li>If the length is greater 15, return false.


If tel's length is greater than 15...

marcoscaceres · 2017-12-06T09:59:30Z

Commit I just pushed is still WIP... not ready for review.

domenic · 2018-01-19T22:27:08Z

@marcoscaceres would you like re-review of this?

marcoscaceres · 2018-01-19T22:33:05Z

Not yet. I will try to finish it this week. It’s still missing some stuff around phone numbers.

stpeter · 2018-01-25T18:56:50Z

index.html

+          The [[!INFRA] specification defines how to <dfn data-cite=
+          "!INFRA#strip-leading-and-trailing-ascii-whitespace">strip leading
+          and trailing ascii whitespace</dfn>. It also defines the concpet of a
+          <dfn data-cite="!INFRA#list">list</dfn>.


If the input form allows a wider range of UTF-8 encoded Unicode characters (not just the ASCII range), then stripping only ASCII whitespace will not necessarily yield the hoped-for result here because a fair number of Unicode code points have a Unicode general category of "Zs". For example, U+1680 (OGHAM SPACE MARK) would not get stripped unless non-ASCII space gets converted to ASCII space.

yeah, that's a good point... specially because this might be coming from free form input from a end-user (e.g., "region").

@domenic, seems JS String.prototpye.trim() does mostly the right thing (some bugginess across browsers). Do you have any suggestions?

Here is a test string:

var whiteSpace = "\u0009\u000A\u000B\u000C\u000D\u0020\u0085\u00A0\u1680\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200A\u2028\u2029\u202F\u205F\u3000" whiteSpace.trim().length; // 1, because u0085 is apparently not WS.

We never trim Unicode whitespace on the platform. Only ASCII whitespace is considered as "allowed sloppiness" in argument values.

An alternative is to not remove such whitespace at all.

@domenic Hmm. Do we assume that non-ASCII Unicode code points with a General Category of "Zs" will be filtered out somehow? (For example, by conversion of such code points to SPACE [U+0020]?) If this is guidance that the i18n WG has provided in the past, perhaps I've just missed the pointer. I can ask in the next i18n WG call if needed.

Generally if a string contains Unicode whitespace, we don't try to second-guess the user or developer, and just pass it through.

Thanks for the clarification! I just asked about it on the www-international list, too: https://lists.w3.org/Archives/Public/www-international/2018JanMar/0042.html

I'm ok with doing the minimal ASCII whitespace stripping, for platform consistency. Developers can then call .trim() to strip out the rest if so desired.

marcoscaceres · 2018-02-15T23:22:26Z

So, from here, we will only take the optional privacy preserving list.

marcoscaceres · 2018-02-27T07:34:19Z

@domenic, this is ready to review again.

Based on discussions with Editors, this PR primarily:

adds regionCode attribute to PaymentAddress (closes ContactAddress 'region' inconsistencies contact-picker#65)
introduces excludeList to match Safari's privacy-preserving behavior (closes redact full shipping address from event until payment response #648)
adds a "internal constructor" (closes Make Payment Address construction into its own sub algorithm #643) ... purpose being that we will eventually make that constructor public, if we do Merchant-specified default shipping address #653 (folks from Apple requested this, and we will probably support that in Firefox too).

marcoscaceres · 2018-02-27T07:37:03Z

Noting for those following at home: excludeList is optional as Chrome and Edge wish to preserve their current behavior. Firefox will make use of the excludeList and behave similarly to Safari. We might tweak the excludeList values if we find it's overly restrictive.

rsolomakhin · 2018-02-27T16:34:19Z

marcoscaceres · 2018-02-28T06:15:32Z

Added tests web-platform-tests/wpt#9706

marcoscaceres · 2018-02-28T06:23:44Z

Added implementation commitment from Firefox.

marcoscaceres · 2018-02-28T09:22:16Z

Actually, I want to land this in two parts.

The editorial rewrite of Physical Addresses.
Then the region code.

That way, 1 can land now... and then 2 can land when we get another implementer to commit.

marcoscaceres · 2018-02-28T09:29:07Z

With just the excludeList, this matches WebKit and Firefox - and let's Chrome and Edge keep returning full address information for the onpaymentaddresschange event.

* closes #692

marcoscaceres · 2018-03-15T02:28:00Z

Thanks @rsolomakhin for also having a look! Oki, should hopefully be all good now 🤞

marcoscaceres · 2018-03-15T03:37:53Z

Thanks again @domenic!!! (the above is @rsolomakhin's fault, he started the whole gifs thing 😁)

rsolomakhin · 2018-03-15T14:53:03Z

… promise from PaymentRequest.show() resolves successfully. r=baku Spec PR: w3c/payment-request#654 MozReview-Commit-ID: 2AiKI7htRhk UltraBlame original commit: 8a29a154e88dc58d0ffde543a2159d70638ce53c

… promise from PaymentRequest.show() resolves successfully. r=baku Spec PR: w3c/payment-request#654 MozReview-Commit-ID: 2AiKI7htRhk UltraBlame original commit: fbc6dec319be8067e0d26055e9c93a5fca74d718

… promise from PaymentRequest.show() resolves successfully. r=baku Spec PR: w3c/payment-request#654 MozReview-Commit-ID: 2AiKI7htRhk UltraBlame original commit: 8a29a154e88dc58d0ffde543a2159d70638ce53c

… promise from PaymentRequest.show() resolves successfully. r=baku Spec PR: w3c/payment-request#654 MozReview-Commit-ID: 2AiKI7htRhk UltraBlame original commit: fbc6dec319be8067e0d26055e9c93a5fca74d718

marcoscaceres requested review from domenic and rsolomakhin November 20, 2017 09:13

rsolomakhin reviewed Nov 20, 2017

View reviewed changes

marcoscaceres mentioned this pull request Nov 23, 2017

fix: define how to set billingAddress (closes #44) w3c/payment-method-basic-card#45

Merged

domenic requested changes Dec 1, 2017

View reviewed changes

marcoscaceres force-pushed the paymentaddress_constructor branch from 6b73009 to 610ab72 Compare December 6, 2017 09:58

marcoscaceres changed the title ~~WIP: add constructor to PaymentAddress~~ Feat: add constructor to PaymentAddress Dec 8, 2017

marcoscaceres mentioned this pull request Dec 8, 2017

Standardize phone number format #652

Closed

4 tasks

marcoscaceres mentioned this pull request Dec 20, 2017

ContactAddress 'region' inconsistencies w3c/contact-picker#65

Open

marcoscaceres force-pushed the paymentaddress_constructor branch from 400f759 to ab1ba80 Compare January 24, 2018 06:20

stpeter reviewed Jan 25, 2018

View reviewed changes

marcoscaceres force-pushed the paymentaddress_constructor branch 3 times, most recently from eb380dd to 6d74799 Compare February 27, 2018 05:36

marcoscaceres changed the title ~~Feat: add constructor to PaymentAddress~~ Feat: add regionCode and exclusionList to PaymentAddress Feb 27, 2018

marcoscaceres force-pushed the paymentaddress_constructor branch from 41ccdf0 to 82dd802 Compare February 28, 2018 09:25

marcoscaceres changed the title ~~Feat: add regionCode and exclusionList to PaymentAddress~~ Feat: add exclusionList for PaymentAddress Feb 28, 2018

marcoscaceres added 14 commits March 15, 2018 13:25

nits

67f4e7a

Editorial improvements

6f751d7

remove regionCode

f5ff02e

s/excludeList/redactList

31428e7

Review nits/typos

1807bf4

fix construction details

358f08b

trim things

82eee2e

Add privacy note about redactList

d032e25

assure request+response use same shippingAddress

30e6806

* closes #692

fix respec warning

a7e615b

tidy

f968a0c

If not shipping, response address is null

50f6cdd

Review feedback

331280c

fix: typo

e21719c

marcoscaceres force-pushed the paymentaddress_constructor branch from f85627e to e21719c Compare March 15, 2018 02:26

domenic approved these changes Mar 15, 2018

View reviewed changes

marcoscaceres merged commit 56a0c18 into gh-pages Mar 15, 2018

marcoscaceres deleted the paymentaddress_constructor branch March 15, 2018 03:39

This was referenced Mar 15, 2018

Make Payment Address construction into its own sub algorithm #643

Closed

When "pay" happens, PaymentAddress request and response should match. #692

Closed

marcoscaceres mentioned this pull request Apr 27, 2018

A distinct invoice address should be supported #550

Closed

add redactList for PaymentAddress (Part 1) #654

add redactList for PaymentAddress (Part 1) #654

Conversation

marcoscaceres commented Nov 20, 2017 • edited by pr-preview bot Loading

marcoscaceres commented Nov 20, 2017

Choose a reason for hiding this comment

marcoscaceres Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoscaceres Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

rsolomakhin left a comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoscaceres commented Dec 6, 2017

domenic commented Jan 19, 2018

marcoscaceres commented Jan 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoscaceres commented Feb 15, 2018

marcoscaceres commented Feb 27, 2018

marcoscaceres commented Feb 27, 2018

rsolomakhin commented Feb 27, 2018

marcoscaceres commented Feb 28, 2018

marcoscaceres commented Feb 28, 2018

marcoscaceres commented Feb 28, 2018

marcoscaceres commented Feb 28, 2018

marcoscaceres commented Mar 15, 2018

marcoscaceres commented Mar 15, 2018

rsolomakhin commented Mar 15, 2018

marcoscaceres commented Nov 20, 2017 •

edited by pr-preview bot

Loading

marcoscaceres Nov 21, 2017 •

edited

Loading

marcoscaceres Dec 1, 2017 •

edited

Loading