Bug 1842203 - Implement the final UX for the FedCM provider prompt by titooan · Pull Request #3091 · mozilla-mobile/firefox-android

titooan · 2023-08-02T17:21:31Z

Pull Request checklist

Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry or does not need one
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

Breaking Changes: If this is a breaking Android Components change, please push a draft PR on Reference Browser to address the breaking issues.

To download an APK when reviewing a PR (after all CI tasks finished running):

Click on Checks at the top of the PR page.
Click on the firefoxci-taskcluster group on the left to expand all tasks.
Click on the build-apk-{fenix,focus,klar}-debug task you're interested in.
Click on View task in Taskcluster in the new DETAILS section.
The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

https://bugzilla.mozilla.org/show_bug.cgi?id=1842203
https://bugzilla.mozilla.org/show_bug.cgi?id=1847784

Amejia481 · 2023-08-16T14:30:55Z

Thanks for putting this patch, it is looking really good!
My main concern will be to see how we will be handling dark and light themes , I'm not sure if we are already handling compose themes on AC.

There are some components that are already using compose, but not sure if they are already handling Themes.
https://github.com/mozilla-mobile/firefox-android/blob/main/fenix/app/src/main/java/org/mozilla/fenix/search/awesomebar/AwesomeBarWrapper.kt
https://github.com/mozilla-mobile/firefox-android/blob/main/android-components/components/compose/cfr/src/main/java/mozilla/components/compose/cfr/CFRPopupFullscreenLayout.kt

Amejia481 · 2023-08-16T14:49:41Z

It looks like in the previous compose implementation we didn't have the concept of themes, we were just passing colors https://github.com/mozilla-mobile/firefox-android/blob/main/android-components/components/compose/awesomebar/src/main/java/mozilla/components/compose/browser/awesomebar/AwesomeBarDefaults.kt#L24-L30.

Amejia481 · 2023-08-16T15:00:06Z

Ideally we would like to create a generic class or contract for all AC components to use, and apps could provide to AC, which just covers the minimal colors and theme management for now and we could extend later. This is what we are doing in Fenix for themes , we would like to have simplify version on AC, which could live on our Colors components or even better if we could create a new component for having everything related to compose where we could have themes, theme management, and a maybe (in the future) have based reusable compose components which could be reuse across AC ✨.

Amejia481 · 2023-08-16T15:59:47Z

Ideally this contract could be pass to PromptFeature or there AC components which will compose.

PromptFeature(
    private val provideTheme : ()-> ACTheme
//....
)

Having a function could help us to delegate the responsibility of theme management to the app and AC will only query the actual correct theme dark or light.

MozillaNoah

first pass. I mainly have a bunch of nits, but this is coming together well!

MozillaNoah · 2023-08-17T19:48:30Z

+    startComposable: (@Composable () -> Unit)? = null,
+) {
+    // Used to propagate the ripple effect to the whole row
+    val interactionSource = remember { MutableInteractionSource() }


was this necessary? Normally Modifier.clickable applies a ripple to the entire layout.

For some reason I don't understand, not using this interactionSource gives a "solid" selected state that isn't a ripple :

no-ripple.mov

This is the result I get when using the interactionSource:

ripple.mov

If I had to guess, it's likely because of the lack of MaterialTheme wrapping all of the Compose code, which might be addressed by the patch we've talked about offline.

MozillaNoah

second pass. this is probably good to come out of draft now!

MozillaNoah · 2023-08-30T21:22:34Z

    <string name="mozac_feature_prompts_manage_address">Manage addresses</string>

    <!-- Federated Credential Management prompts -->
+    <!--Content description for the Provider favicon in the Select Provider FedCM prompt -->


You might want to be a little more descriptive and avoid some of the business acronyms for the comments, since a lot of the localizers don't necessarily poke into everything in the app to be able to have the context.

I realised I ended up not using this string, as I think there's no need for the Screen Reader to read a view that is purely decorative. (see Cases that don't require content labels in Google Documentation.

Thanks for pointing this out!

MozillaNoah · 2023-08-30T21:24:51Z

+    startComposable: (@Composable () -> Unit)? = null,
+) {
+    // Used to propagate the ripple effect to the whole row
+    val interactionSource = remember { MutableInteractionSource() }


If I had to guess, it's likely because of the lack of MaterialTheme wrapping all of the Compose code, which might be addressed by the patch we've talked about offline.

…dialogs in Compose

github-actions Bot added the 🕵️‍♀️ needs review PRs that need to be reviewed label Aug 2, 2023

titooan marked this pull request as draft August 2, 2023 17:21

github-actions Bot added work in progress Not ready to land yet. Work in progress (WIP). and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Aug 2, 2023

titooan force-pushed the bug1842203-fedcm-provider-prompt branch 2 times, most recently from c0ed29b to 663a7b3 Compare August 14, 2023 15:23

Amejia481 reviewed Aug 16, 2023

View reviewed changes

Comment thread ...src/main/java/mozilla/components/feature/prompts/identitycredential/BasicProvidersAdapter.kt

Amejia481 reviewed Aug 16, 2023

View reviewed changes

Comment thread ...s/src/main/java/mozilla/components/feature/prompts/identitycredential/SelectAccountDialog.kt Outdated

MozillaNoah reviewed Aug 17, 2023

View reviewed changes

titooan force-pushed the bug1842203-fedcm-provider-prompt branch from 663a7b3 to 8f66a5d Compare August 17, 2023 21:16

titooan force-pushed the bug1842203-fedcm-provider-prompt branch from 8f66a5d to f572f8f Compare August 30, 2023 14:32

MozillaNoah reviewed Aug 30, 2023

View reviewed changes

titooan force-pushed the bug1842203-fedcm-provider-prompt branch from f572f8f to b7375e1 Compare August 31, 2023 19:22

titooan marked this pull request as ready for review September 18, 2023 18:47

github-actions Bot added 🕵️‍♀️ needs review PRs that need to be reviewed and removed work in progress Not ready to land yet. Work in progress (WIP). labels Sep 18, 2023

titooan force-pushed the bug1842203-fedcm-provider-prompt branch from b7375e1 to 53a2b6b Compare September 18, 2023 18:49

titooan changed the title ~~WIP - Bug 1842203 - Implement the final UX for the FedCM provider prompt~~ Bug 1842203 - Implement the final UX for the FedCM provider prompt Sep 20, 2023

Amejia481 approved these changes Sep 21, 2023

View reviewed changes

titooan added approved PR that has been approved 🛬 needs landing PRs that are ready to land and removed 🕵️‍♀️ needs review PRs that need to be reviewed 🛬 needs landing PRs that are ready to land labels Sep 21, 2023

titooan force-pushed the bug1842203-fedcm-provider-prompt branch from 53a2b6b to b34e7c9 Compare September 21, 2023 13:21

github-actions Bot assigned Amejia481 Sep 21, 2023

titooan added the 🛬 needs landing PRs that are ready to land label Sep 21, 2023

titooan removed the 🛬 needs landing PRs that are ready to land label Sep 21, 2023

titooan force-pushed the bug1842203-fedcm-provider-prompt branch 3 times, most recently from 8eb5047 to 133c811 Compare September 21, 2023 21:40

Bug 1842203: Implement of the final UX of FedCM Provider and Account …

a362cb1

…dialogs in Compose

titooan force-pushed the bug1842203-fedcm-provider-prompt branch 3 times, most recently from bc8d695 to 42f4c97 Compare September 22, 2023 17:41

Bug 1847784: Provide a Theme to FedCM Provider Dialogs

eadef48

titooan force-pushed the bug1842203-fedcm-provider-prompt branch from 42f4c97 to eadef48 Compare September 22, 2023 18:00

titooan added the 🛬 needs landing PRs that are ready to land label Sep 25, 2023

Merge branch 'main' into bug1842203-fedcm-provider-prompt

2ec0d00

mergify Bot merged commit 06ba878 into mozilla-mobile:main Sep 25, 2023

Conversation

titooan commented Aug 2, 2023 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request checklist

After merge

To download an APK when reviewing a PR (after all CI tasks finished running):

GitHub Automation

Uh oh!

Uh oh!

Uh oh!

Amejia481 commented Aug 16, 2023

Uh oh!

Amejia481 commented Aug 16, 2023

Uh oh!

Amejia481 commented Aug 16, 2023

Uh oh!

Amejia481 commented Aug 16, 2023

Uh oh!

MozillaNoah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MozillaNoah Aug 17, 2023

Choose a reason for hiding this comment

Uh oh!

titooan Aug 18, 2023

Choose a reason for hiding this comment

Uh oh!

MozillaNoah Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MozillaNoah left a comment

Choose a reason for hiding this comment

Uh oh!

MozillaNoah Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

titooan Aug 31, 2023

Choose a reason for hiding this comment

Uh oh!

MozillaNoah Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

titooan commented Aug 2, 2023 •

edited by github-actions Bot

Loading