Closed Bug 1466720 Opened 3 years ago Closed 3 years ago

Basic PaymentRequest dialog default styles

Categories

(Firefox :: WebPayments UI, enhancement, P1)

Product:
Firefox
Component:
WebPayments UI
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

Details

(Whiteboard: [webpayments])

Attachments

(3 files)

Bug 1466720 - Don't reload l10n.js in the PaymentRequest scope on a refresh.
59 bytes, text/x-review-board-request
sfoster
: review+
Details
Bug 1466720 - Default PaymentRequest dialog styles for background, <a>, and debugging.html.
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1466720 - Make PR shipping options use inline amounts followed by labels.
59 bytes, text/x-review-board-request
sfoster
: review+
Details 
System fonts, default <a> appearance, etc. rather than using the html.css defaults
Depends on: 1468153
Comment on attachment 8984808 [details]
Bug 1466720 - Make PR shipping options use inline amounts followed by labels.

https://reviewboard.mozilla.org/r/250616/#review256938

::: browser/components/payments/res/components/shipping-option.css:15
(Diff revision 1)
> -  grid-area: amount;
> +  white-space: nowrap;
>  }
>  
> -shipping-option > .label,
>  shipping-option > .amount {
> -  white-space: nowrap;
> +  margin-inline-end: 1ex;

(Idly) curious why use the 'ex' unit? I understand it makes this gap proportional to the font in use, but why not 'ch' which measures a width rather than a height.
Attachment #8984808 - Flags: review?(sfoster) → review+
Comment on attachment 8984806 [details]
Bug 1466720 - Don't reload l10n.js in the PaymentRequest scope on a refresh.

https://reviewboard.mozilla.org/r/250612/#review256990

Looks much improved. 
We have some tests that check textContent that indirectly test localization of the UI. But maybe it would be useful to add something in a follow-up to explicitly sanity-check this?

::: browser/extensions/formautofill/content/l10n.js
(Diff revision 1)
>      attributes: true,
>      attributeFilter: L10N_ATTRIBUTES,
>      subtree: true,
>    });
> -}, {
> -  once: true,

AFAICT the new arrangement looks like a much more robust way to load this l10n.js, initially localize and monitor the dialog content document but this gives me pause. Are there (were there) situations in which DOMContentLoaded would be fired more than once? I don't understand why this 'once' param was necessary originally, so I can't be sure how that scenario is addressed by the new patch.
Attachment #8984806 - Flags: review?(sfoster) → review+
Comment on attachment 8984807 [details]
Bug 1466720 - Default PaymentRequest dialog styles for background, <a>, and debugging.html.

https://reviewboard.mozilla.org/r/250614/#review257104

::: browser/components/payments/res/paymentRequest.css:25
(Diff revision 1)
>    height: 100%;
>    margin: 0;
>    overflow: hidden;
>  }
>  
> +/* Default link styles from https://design.firefox.com/photon/components/links.html */

Can you put a screenshot of this page on the bug? I don't feel too confident about this URL living longer than a year ;)

::: browser/components/payments/res/paymentRequest.css:37
(Diff revision 1)
> +.text-link:active,
> +a:active {

We generally use :hover:active for :active styling.
Attachment #8984807 - Flags: review?(jaws) → review+
Comment on attachment 8984808 [details]
Bug 1466720 - Make PR shipping options use inline amounts followed by labels.

https://reviewboard.mozilla.org/r/250616/#review256938

> (Idly) curious why use the 'ex' unit? I understand it makes this gap proportional to the font in use, but why not 'ch' which measures a width rather than a height.

Just because 1ex seemed too wide for a space… I just changed it to a space instead.
Comment on attachment 8984806 [details]
Bug 1466720 - Don't reload l10n.js in the PaymentRequest scope on a refresh.

https://reviewboard.mozilla.org/r/250612/#review256990

I thought about this but since it only affects local development and this file should go away with Fluent it didn't seem worth the effort to add tests now.

> AFAICT the new arrangement looks like a much more robust way to load this l10n.js, initially localize and monitor the dialog content document but this gives me pause. Are there (were there) situations in which DOMContentLoaded would be fired more than once? I don't understand why this 'once' param was necessary originally, so I can't be sure how that scenario is addressed by the new patch.

Note that we we previously called `localizeMarkup` before too so I didn't change what happens. DOMContentLoad can only be fired once per document and the event target is always used so we don't need to worry about subframes DCL. I think the `once` was just to avoid mozilla/balanced-listeners (which shouldn't matter in this context).
Attachment #8984807 - Flags: review?(sfoster)
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/afba719bf903
Don't reload l10n.js in the PaymentRequest scope on a refresh. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/beee9be72f9d
Default PaymentRequest dialog styles for background, <a>, and debugging.html. r=jaws
https://hg.mozilla.org/integration/autoland/rev/09023041e95a
Make PR shipping options use inline amounts followed by labels. r=sfoster
https://hg.mozilla.org/mozilla-central/rev/afba719bf903
https://hg.mozilla.org/mozilla-central/rev/beee9be72f9d
https://hg.mozilla.org/mozilla-central/rev/09023041e95a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in before you can comment on or make changes to this bug.