Details

Reviewers

emz
flod
Itiel

Group Reviewers

desktop-theme-reviewers

Commits

rMOZILLACENTRALd62ca666e600: Bug 1804728, part 4 - Make UI match mock, without pictures - r=pbz,desktop…

Bugzilla Bug ID

1804728

Diff Detail

Repository

rMOZILLACENTRAL mozilla-central

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Build Status

Buildable 508049
Build 604417: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

flod added a subscriber: flod.Feb 3 2023, 6:32 PM

flod added inline comments.

browser/components/credentialmanager/identityCredentialNotification.ftl
16	Please match the case of the character in the label.
21	"Log into" is not a form our content team supports https://mozilla-hub.atlassian.net/wiki/spaces/FIREFOX/pages/11048304/Word+List#WordList-Login%2FLogin Not sure if the linter complains about files outside of localization paths, but `'` should be `’`.

bvandersloot marked an inline comment as done.Feb 3 2023, 6:45 PM

bvandersloot added inline comments.

browser/components/credentialmanager/identityCredentialNotification.ftl
21	Interesting... I'm pulling from a Figma I thought a content team member had gone over. I'll double check that.

Itiel added a subscriber: Itiel.Feb 4 2023, 4:59 PM

Itiel added inline comments.

browser/themes/shared/icons/globe.svg
1 ↗	(On Diff #677120)	This SVG is very similar to a pre-existing SVG: https://searchfox.org/mozilla-central/source/toolkit/themes/shared/icons/defaultFavicon.svg Can this be used instead? If not, can you ask UX if it should be updated with the new one you've providing here?

How can I test this patch?
Following the instructions at https://phabricator.services.mozilla.com/D158778#5217801 doesn't work anymore.

flod edited reviewers, added: flod; removed: Restricted Project.Feb 5 2023, 6:29 AM

bvandersloot updated this revision to Diff 677613.Feb 6 2023, 3:11 PM

Harbormaster completed remote builds in B503706: Diff 677613.Feb 6 2023, 3:11 PM

In D168817#5556241, @Itiel wrote:

How can I test this patch?
Following the instructions at https://phabricator.services.mozilla.com/D158778#5217801 doesn't work anymore.

These instructions work for me... although I had to rebase onto the head of hg pull because the testing certificate expired and I was getting errors. It may be better to test with the changeset at D168819 instead, to integrate all of the changes for this bug. What do you see that isn't a dialog?

browser/themes/shared/icons/globe.svg
1 ↗	(On Diff #677120)	Good catch- that can be used instead.

bvandersloot updated this revision to Diff 677703.Feb 6 2023, 5:51 PM

Harbormaster completed remote builds in B503772: Diff 677703.Feb 6 2023, 5:51 PM

Itiel added inline comments.Feb 7 2023, 4:57 PM

browser/themes/shared/identity-credential-notification.css
54	You can remove this `width: 100%` and `align-items: flex-start` here, and then you could also remove `align-self: stretch`, `flex-direction: row`, and `align-items: flex-start` in the next rule. Also, what is the `padding: 0` below trying to reset?
64	Instead of hardcoding this, how about changing swapping it with this? padding-block: max(calc(var(--arrowpanel-menuitem-padding-block) * 2), 4px); padding-inline: calc(var(--arrowpanel-menuitem-padding-inline) * 2); This way it won't look too big for compact mode.
73	All of these should be `background-color` instead
80	All of the uses of `--in-content-*` won't work in this context because common-shared.css isn't imported here, nor it should. If this item was a XUL `radio` it would have been easy, but that's not the case. Something like that: https://searchfox.org/mozilla-central/rev/188d0f76a73e0671d12e744a71e9f5701668cc37/browser/themes/shared/customizableui/panelUI-shared.css#1445-1463 should be implemented for non-in-content input[type=radio]
84	There's no need for that in this context
100	`0.6` is a more common way to write that in the codebase

Also, this could be added to make the close button nicer:

.close-icon {
  margin: 0;
  width: auto;
}

Sorry for the delay! This looks great! The change seems mostly fine, I just had some questions and nits. I haven't focused too much on the CSS changes since @Itiel is really the expert here.

Is there a spec for this I should be looking at?

In the test dom/credentialmanagement/identity/tests/mochitest/test_two_providers.html when I close the popup notification via the X, I can see an error being thrown in the console:

JavaScript error: resource://gre/modules/PopupNotifications.sys.mjs, line 1806: TypeError: can't access property "options", notification is null

Is this expected?

browser/themes/shared/identity-credential-notification.css
82	Why do we need to set a background image here, doesn't the radio button come with that? I don't see a difference when I comment this out.
toolkit/components/credentialmanagement/IdentityCredentialPromptService.sys.mjs
78–82	This has come up before, but I'm wondering if we could just loop over `x` once instead and grab the attributes we need.
99–138	Why do we need to manually remove the popup notification? Also, what happens if `getNotification` returns `null` because the notification is already gone?
103	This works but `addEventListener` is probably better because it's more flexible. Using `onchange` means there can only be one listener, which is probably fine for now but makes this harder to extend.
105	It would be nice if we could do this in a way that doesn't require us to manually manage the state like that. The radio buttons should already have the information we need. Is this in lack of a CSS parent selector like `has`?
110	Why is this necessary?
128–132	Consider using a query selector that matches both the class name and the `checked` attribute value instead. Then you don't need to use `find`.
293	Why do we need to call this explicitly? It looks fine when testing manually without this line. We're not awaiting the result.
342	nit: strictly speaking, isn't this `providerURL`?
358–362	Same as above, these find calls seem inefficient.
374–375	Same as above, why do we need to remove the notification manually?
378	Same as above `addEventListener`
385	Why do we need to stop propagation?

This revision now requires changes to proceed.Feb 9 2023, 3:41 PM

RE: the close button. I looked again and it is now missing from the mocks I'm working from. So it is gone now, resolving any issues with it...

browser/themes/shared/identity-credential-notification.css
73	I've gotten some feedback from UI and have given this different colors based on various transparencies of `--button-primary-bgcolor` and using `--button-bgcolor` for a default border. That is updated here.
80	I've found some options that look good and are in chrome (I think) instead from here: https://searchfox.org/mozilla-central/source/browser/themes/shared/browser-custom-colors.css#9-16,20-28
82	We don't- removing.
toolkit/components/credentialmanagement/IdentityCredentialPromptService.sys.mjs
78–82	Given that it is a loop over two elements in each of these, I don't think this is worth worrying about. Especially so long as the formatting is done synchronously as well.
105	That's exactly right. This is a lack of `:has` and a label with child input[type=radio].
110	In retrospect, I don't think it is.
293	I found that I needed it sometimes, and couldn't figure out the conditions that require it. We shouldn't need it, but it is a known issue that sometimes you have to force a l10n when you shouldn't.

bvandersloot requested review of this revision.Feb 9 2023, 5:46 PM

bvandersloot updated this revision to Diff 679322.

bvandersloot marked 2 inline comments as done.

Harbormaster completed remote builds in B505784: Diff 679322.Feb 9 2023, 5:46 PM

bvandersloot updated this revision to Diff 679849.Feb 10 2023, 4:34 PM

Harbormaster completed remote builds in B506307: Diff 679849.Feb 10 2023, 4:34 PM

flod accepted this revision.Feb 11 2023, 6:01 AM

Please apply this file instead of the one you have in this patch:

identity-credential-notification.css3 KBDownload

This should make the radio button behave more like current radio buttons elsewhere, and make things more readable in HCM.

Itiel attached a referenced file: F4471736: identity-credential-notification.css. (Show Details)Feb 11 2023, 6:21 PM

Could you please link the spec if you have any? Maybe I've missed it? Thanks!

I found a bug: With Alpenglow the background color for the unselected radio button doesn't seem to work:

This revision now requires changes to proceed.Feb 13 2023, 11:43 AM

emz added inline comments.Feb 13 2023, 11:54 AM

toolkit/components/credentialmanagement/IdentityCredentialPromptService.sys.mjs
78–82	👍🏻 sure, I don't want to block on this! Just wondering if there is a proper way to do this going forward...

In D168817#5579848, @pbz wrote:

Could you please link the spec if you have any? Maybe I've missed it? Thanks!

https://docs.google.com/document/d/1ZVLROLvmYpi-lYmoySyXDAAx4CaMNCun1gtZH6VBADU/edit#heading=h.yt9huhl7d3xf

I found a bug: With Alpenglow the background color for the unselected radio button doesn't seem to work:

This is resolved with Itiel's patch, which I am submitting now.

In D168817#5578354, @Itiel wrote:

Please apply this file instead of the one you have in this patch:

identity-credential-notification.css3 KBDownload

This should make the radio button behave more like current radio buttons elsewhere, and make things more readable in HCM.

Thanks Itiel! Replacing!

bvandersloot requested review of this revision.Feb 13 2023, 2:12 PM

bvandersloot updated this revision to Diff 680313.

Herald added a subscriber: morgan. · View Herald TranscriptFeb 13 2023, 2:12 PM

Harbormaster completed remote builds in B506701: Diff 680313.Feb 13 2023, 2:12 PM

Itiel added inline comments.Feb 13 2023, 3:15 PM