Closed
Bug 1466720
Opened 4 years ago
Closed 4 years ago
Basic PaymentRequest dialog default styles
Categories
(Firefox :: WebPayments UI, enhancement, P1)
Firefox
WebPayments UI
Tracking
()
RESOLVED
FIXED
Firefox 62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: MattN, Assigned: MattN)
References
Details
(Whiteboard: [webpayments])
Attachments
(3 files)
System fonts, default <a> appearance, etc. rather than using the html.css defaults
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•4 years ago
|
||
mozreview-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 ::: 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 5•4 years ago
|
||
mozreview-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 6•4 years ago
|
||
mozreview-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+
Assignee | ||
Comment 7•4 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 8•4 years ago
|
||
mozreview-review-reply |
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).
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•4 years ago
|
Attachment #8984807 -
Flags: review?(sfoster)
Comment 13•4 years ago
|
||
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
Comment 14•4 years ago
|
||
bugherder |
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: 4 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.
Description
•