Closed Bug 1469464 Opened 5 years ago Closed 5 years ago

Update PaymentRequest header and footer match the visual specs

Categories

(Firefox :: WebPayments UI, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

Details

(Whiteboard: [webpayments])

User Story

* 63: Footer button consistent position
* Scrollable area above the footer
* Footer grey background
* 63: Remove View Cart / View All Items
* Remove unused "Total" label element
* 63: Add "Pay to"
* Add currency code to header
* total colouring
* Remove "Your Payment" heading

Attachments

(5 files, 1 obsolete file)

Various updates discussed during the SF All Hands to match the visual specs.
User Story: (updated)
While making the footer position correct, I may as well make the middle area scrollable.
User Story: (updated)
Priority: P2 → P1
Depends on: 1470203
Comment on attachment 8986087 [details]
Bug 1469464 - Consistent PaymentRequest footer positioning with <payment-request-page>.

https://reviewboard.mozilla.org/r/251542/#review263034

I got test failures at this point in the patch series. I'm going through in order, so if those are addressed in a later patch maybe they should be folded together? It looks great otherwise. Should just be some updating of references so I'll call it a nit.
Attachment #8986087 - Flags: review?(sfoster) → review+
Comment on attachment 8986085 [details]
Bug 1469464 - Always hide View All, remove summary heading, add 'Pay to'.

https://reviewboard.mozilla.org/r/251538/#review263036

Looks good
Attachment #8986085 - Flags: review?(sfoster) → review+
Comment on attachment 8986086 [details]
Bug 1469464 - Display the currency code with the total.

https://reviewboard.mozilla.org/r/251540/#review263038

Big improvement
Attachment #8986086 - Flags: review?(sfoster) → review+
Comment on attachment 8990581 [details]
Bug 1469464 - Fix default shippingAddressErrors value for debugging.

https://reviewboard.mozilla.org/r/255646/#review263040
Attachment #8990581 - Flags: review?(sfoster) → review+
Comment on attachment 8990858 [details]
Bug 1469464 - PaymentRequest footer styling.

https://reviewboard.mozilla.org/r/255856/#review263044

::: browser/components/payments/res/containers/address-form.css:77
(Diff revision 1)
>  :-moz-any(input, textarea, select):not(:focus) + .error-text,
>  :-moz-any(input, textarea, select):valid + .error-text {
>    display: none;
>  }
> +
> +address-form > footer > .cancel-button {

Isnt this (eventually) going to follow platform UI conventions?
Attachment #8990858 - Flags: review?(sfoster) → review+
Comment on attachment 8986087 [details]
Bug 1469464 - Consistent PaymentRequest footer positioning with <payment-request-page>.

https://reviewboard.mozilla.org/r/251542/#review263034

Oops, I think I renamed that variable near the end and forget to re-run the tests.
Comment on attachment 8990858 [details]
Bug 1469464 - PaymentRequest footer styling.

https://reviewboard.mozilla.org/r/255856/#review263044

> Isnt this (eventually) going to follow platform UI conventions?

Yeah, I suspect so (I mentioned it to UX the other day) but UX is susposed to get back to us on the button ordering. I'll include that in bug 1468153 and mention the bug number.
Comment on attachment 8986085 [details]
Bug 1469464 - Always hide View All, remove summary heading, add 'Pay to'.

https://reviewboard.mozilla.org/r/251538/#review263324


Code analysis found 1 defect in this patch:
 - 1 defect 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/mochitest/test_payment_dialog.html:97
(Diff revision 5)
>  add_task(async function test_viewAllButtonVisibility() {
>    await setup();
>  
>    let button = el1._viewAllButton;
>    ok(button.hidden, "Button is initially hidden when there are no items to show");
> +  ok(isHidden(button), "Button should be visibly hidden since bug 1469464")

Error: Missing semicolon. [eslint: semi]
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/4b86bde79c91
Always hide View All, remove summary heading, add 'Pay to'. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/9453814beada
Display the currency code with the total. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/90654d5df81a
Fix default shippingAddressErrors value for debugging. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/58c1a79fb661
Consistent PaymentRequest footer positioning with <payment-request-page>. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/59c418929764
PaymentRequest footer styling. r=sfoster
Comment on attachment 8986087 [details]
Bug 1469464 - Consistent PaymentRequest footer positioning with <payment-request-page>.

https://reviewboard.mozilla.org/r/251542/#review263326


Code analysis found 1 defect in this patch:
 - 1 defect 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/mochitest/test_payment_dialog.html:97
(Diff revision 5)
>  add_task(async function test_viewAllButtonVisibility() {
>    await setup();
>  
>    let button = el1._viewAllButton;
>    ok(button.hidden, "Button is initially hidden when there are no items to show");
>    ok(isHidden(button), "Button should be visibly hidden since bug 1469464")

Error: Missing semicolon. [eslint: semi]
Comment on attachment 8991520 [details]
Bug 1469464 - Fix eslint missing semicolon in test_payment_dialog.html.

https://reviewboard.mozilla.org/r/256432/#review263328
Attachment #8991520 - Flags: review?(MattN+bmo) → review+
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/f9d4885e8956
Fix eslint missing semicolon in test_payment_dialog.html. r=MattN
Backed out 6 changesets (bug 1469464) for browser-chrome failures at browser/base/content/test/static/browser_parsable_css.js 

Backout: https://hg.mozilla.org/integration/autoland/rev/135aaab3fef515f55cd323f2b2248d9c9aa61c88

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=59c4189297640ec9c2bdbb2d3bb7b617cb74e6e5

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=187760336&repo=autoland&lineNumber=1681

0:17:28     INFO - Ignored error "Expected media feature name but found ‘-moz-is-glyph’." on resource://gre/res/svg.css because of whitelist item {"sourceName":"/\\b(autocomplete-item|svg)\\.css$/","errorMessage":"/Expected media feature name but found \\u2018-moz.*/i","isFromDevTools":false,"used":true}
00:17:28     INFO - Ignored error "Expected media feature name but found ‘-moz-is-resource-document’." on resource://gre/res/svg.css because of whitelist item {"sourceName":"/\\b(autocomplete-item|svg)\\.css$/","errorMessage":"/Expected media feature name but found \\u2018-moz.*/i","isFromDevTools":false,"used":true}
00:17:28     INFO - Ignored error "Unknown pseudo-class or pseudo-element ‘-moz-svg-foreign-content’.  Ruleset ignored due to bad selector." on resource://gre/res/svg.css because of whitelist item {"sourceName":"/\\b(contenteditable|EditorOverride|svg|forms|html|mathml|ua)\\.css$/i","errorMessage":"/Unknown pseudo-class.*-moz-/i","isFromDevTools":false,"used":true}
00:17:28     INFO - Ignored error "Unknown pseudo-class or pseudo-element ‘-moz-svg-text’.  Ruleset ignored due to bad selector." on resource://gre/res/svg.css because of whitelist item {"sourceName":"/\\b(contenteditable|EditorOverride|svg|forms|html|mathml|ua)\\.css$/i","errorMessage":"/Unknown pseudo-class.*-moz-/i","isFromDevTools":false,"used":true}
00:17:28     INFO - Ignored error "Unknown pseudo-class or pseudo-element ‘-moz-svg-marker-anon-child’.  Ruleset ignored due to bad selector." on resource://gre/res/svg.css because of whitelist item {"sourceName":"/\\b(contenteditable|EditorOverride|svg|forms|html|mathml|ua)\\.css$/i","errorMessage":"/Unknown pseudo-class.*-moz-/i","isFromDevTools":false,"used":true}
00:17:28     INFO - Buffered messages finished
00:17:28     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_parsable_css.js | Got error message for resource://payments/paymentRequest.css: Expected declaration but found ‘"disabled-overlay"’.  Skipped to next declaration. - 
00:17:28     INFO - Stack trace:
00:17:28     INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_parsable_css.js:messageIsCSSError:243
00:17:28     INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_parsable_css.js:checkAllTheCSS:442
00:17:28     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1103
00:17:28     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1094
00:17:28     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:996
00:17:28     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795
00:17:28     INFO - Ignored error "Unknown property ‘user-select’.  Declaration dropped." on resource://pdf.js/web/viewer.css because of whitelist item {"sourceName":"/web\\/viewer\\.css$/i","errorMessage":"/Unknown property.*(appearance|user-select)/i","isFromDevTools":false,"used":true}
00:17:28     INFO - Ignored error "Unknown property ‘appearance’.  Declaration dropped." on resource://pdf.js/web/viewer.css because of whitelist item {"sourceName":"/web\\/viewer\\.css$/i","errorMessage":"/Unknown property.*(appearance|user-select)/i","isFromDevTools":false,"used":true}
00:17:28     INFO - Ignored error "Unknown property ‘user-select’.  Declaration dropped." on resource://pdf.js/web/viewer.css because of whitelist item {"sourceName":"/web\\/viewer\\.css$/i","errorMessage":"/Unknown property.*(appearance|user-select)/i","isFromDevTools":false,"used":true}
00:17:28     INFO - Ignored error "Unknown property ‘user-select’.  Declaration dropped." on resource://pdf.js/web/viewer.css because of whitelist item {"sourceName":"/web\\/viewer\\.css$/i","errorMessage":"/Unknown property.*(appearance|user-select)/i","isFromDevTools":false,"used":true}
00:17:28     INFO - Ignored error "Unknown property ‘user-select’.  Declaration dropped." on resource://pdf.js/web/viewer.css because of whitelist item {"sourceName":"/web\\/viewer\\.css$/i","errorMessage":"/Unknown property.*(appearance|user-select)/i","isFromDevTools":false,"used":true}
00:17:28     INFO - Ignored error "Unknown property ‘user-select’.  Declaration dropped." on resource://pdf.js/web/viewer.css because of whitelist item {"sourceName":"/web\\/viewer\\.css$/i","errorMessage":"/Unknown property.*(appearance|user-select)/i","isFromDevTools":false,"used":true}
00:17:28     INFO - Ignored error "Unknown property ‘user-select’.  Declaration dropped." on resource://pdf.js/web/viewer.css because of whitelist item {"sourceName":"/web\\/viewer\\.css$/i","errorMessage":"/Unknown property.*(appearance|user-select)/i","isFromDevTools":false,"used":true}
00:17:28     INFO - Ignored error "Unknown property ‘user-select’.  Declaration dropped." on resource://pdf.js/web/viewer.css because of whitelist item {"sourceName":"/web\\/viewer\\.css$/i","errorMessage":"/Unknown property.*(appearance|user-select)/i","isFromDevTools":false,"used":true}
00:17:28     INFO - Ignored error "Unknown property ‘user-select’.  Declaration dropped." on resource://pdf.js/web/viewer.css because of whitelist item {"sourceName":"/web\\/viewer\\.css$/i","errorMessage":"/Unknown property.*(appearance|user-select)/i","isFromDevTools":false,"used":true}
00:17:28     INFO - Ignored error "Unknown property ‘user-select’.  Declaration dropped." on resource://pdf.js/web/viewer.css because of whitelist item {"sourceName":"/web\\/viewer\\.css$/i","errorMessage":"/Unknown property.*(appearance|user-select)/i","isFromDevTools":false,"used":true}
00:17:28     INFO - Ignored error "Expected media feature name but found ‘-moz-windows-default-theme’." on jar:file:///Users/cltbld/tasks/task_1531379587/build/application/Firefox%20Nightly.app/Contents/Resources/browser/features/formautofill@mozilla.org.xpi!/chrome/skin/windows/autocomplete-item.css because of whitelist item {"sourceName":"/\\b(autocomplete-item|svg)\\.css$/","errorMessage":"/Expected media feature name but found \\u2018-moz.*/i","isFromDevTools":false,"used":true}
00:17:28     INFO - Not taking screenshot here: see the one that was previously logged
00:17:28     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_parsable_css.js | All the styles (205) loaded without errors. - Got 1, expected 0
00:17:28     INFO - Stack trace:
00:17:28     INFO - chrome://mochikit/content/browser-test.js:test_is:1305
00:17:28     INFO - chrome://mochitests/content/browser/browser/base/content/test/static/browser_parsable_css.js:checkAllTheCSS:443
00:17:28     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1103
00:17:28     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1094
00:17:28     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:996
00:17:28     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795
00:17:28     INFO - Leaving test bound checkAllTheCSS
00:17:28     INFO - GECKO(1993) | MEMORY STAT | vsize 4778MB | residentFast 780MB | heapAllocated 413MB
Flags: needinfo?(MattN+bmo)
Flags: needinfo?(MattN+bmo)
Attachment #8991520 - Attachment is obsolete: true
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/825671e0c5cc
Always hide View All, remove summary heading, add 'Pay to'. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/5ffc9a512843
Display the currency code with the total. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/d6174a81ff89
Fix default shippingAddressErrors value for debugging. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/e6f0e3fb69d6
Consistent PaymentRequest footer positioning with <payment-request-page>. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/8da83fe09309
PaymentRequest footer styling. r=sfoster
You need to log in before you can comment on or make changes to this bug.