For #12855 - Upstream the CFRPopup from Fenix to AC to share it in multiple projects #12828

Mugurell · 2022-09-22T16:06:50Z

Found 2 small bugs that I fixed in the process, testing on Focus everything works as expected.

Proposed new API tested on Focus which would allow the needed customization - https://github.com/mozilla-mobile/android-components/pull/12828/files#diff-55b4a3672da3aa9ede82afef5dcc594077503876393b52b77cedbdf9d7315584

CFRPopupsInLTR.mp4

CFRPopupInRTL.mp4

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

Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

GitHub Automation

Fixes #12855

Mugurell · 2022-09-22T16:10:00Z

components/compose/cfr/src/main/java/mozilla/components/compose/cfr/helper/ScreenOrientation.kt

+enum class Orientation
+
+// WIP
+object ScreenOrientation {


Fenix uses GeckoScreenOrientation from geckoview which seems like a just a bit too big of a dependency to add for just that listener.
So we have to add our own.
Thinking that it might be useful to have it in a separate module, available to other ones which don't need the CFR.

I saw this earlier when investigating other CFRs for Focus and I was curious, why do we use the GeckoScreenOrientation listener for the CFRs? It seems like it's used for knowing when the app was rotated and we can get that directly from Android APIs.

+100 for this upstream change though. :)

Was using GeckoScreenOrientation since it was already available and ready to use.
Having to find a replacement now reminded me that although there are Android APIs for this (which GeckoScreenOrientation uses), there are many such APIs, some deprecated in other versions of Android and there is no simple listener that would just work.
For example the OrientationEventListener Google recommends will also fire if "Auto-rotate" is disabled, similar to a sensor listener though the display will not actually be rotated.

gbrownmozilla

taskcluster and build changes lgtm. Someone else should review all the .kt files.

jonalmeida

This is a good example of Compose code to have upstream for referencing that makes it easy to read too. It also has nice examples of propagation of properties needed by inner functions. 🙂

Let's remove the debug println and add a translatable doc - everything else looks great!

jonalmeida · 2022-10-12T22:03:26Z

components/compose/cfr/README.md

+            style = MaterialTheme.typography.body2,
+        )
+    },
+    action = {


Self-note: Comparing to what is in Focus, it looks like action is the new API addition with the added properties above too.

class CFRPopup( private val container: View, private val text: String, private val anchor: View, private val properties: CFRPopupProperties = CFRPopupProperties(), private val onDismiss: () -> Unit = {}, )

jonalmeida · 2022-10-12T22:07:50Z

components/compose/cfr/src/main/java/mozilla/components/compose/cfr/CFRPopup.kt

+data class CFRPopupProperties(
+    val popupWidth: Dp = CFRPopup.DEFAULT_WIDTH.dp,
+    val popupAlignment: PopupAlignment = PopupAlignment.BODY_TO_ANCHOR_CENTER,
+    val popupBodyColors: List<Int> = listOf(Color.Blue.toArgb()),
+    val dismissButtonColor: Int = Color.Black.toArgb(),
+    val indicatorDirection: IndicatorDirection = IndicatorDirection.UP,
+    val dismissOnBackPress: Boolean = true,
+    val dismissOnClickOutside: Boolean = true,
+    val overlapAnchor: Boolean = false,
+    val popupVerticalOffset: Dp = CFRPopup.DEFAULT_VERTICAL_OFFSET.dp,
+    val indicatorArrowStartOffset: Dp = CFRPopup.DEFAULT_INDICATOR_START_OFFSET.dp,
+)


Cosmetic (update only if there are other valuable additions to make to the PR): keep the popup, indicator, and dismiss properties together.

Suggested change

data class CFRPopupProperties(

val popupWidth: Dp = CFRPopup.DEFAULT_WIDTH.dp,

val popupAlignment: PopupAlignment = PopupAlignment.BODY_TO_ANCHOR_CENTER,

val popupBodyColors: List<Int> = listOf(Color.Blue.toArgb()),

val dismissButtonColor: Int = Color.Black.toArgb(),

val indicatorDirection: IndicatorDirection = IndicatorDirection.UP,

val dismissOnBackPress: Boolean = true,

val dismissOnClickOutside: Boolean = true,

val overlapAnchor: Boolean = false,

val popupVerticalOffset: Dp = CFRPopup.DEFAULT_VERTICAL_OFFSET.dp,

val indicatorArrowStartOffset: Dp = CFRPopup.DEFAULT_INDICATOR_START_OFFSET.dp,

)

data class CFRPopupProperties(

val popupWidth: Dp = CFRPopup.DEFAULT_WIDTH.dp,

val popupAlignment: PopupAlignment = PopupAlignment.BODY_TO_ANCHOR_CENTER,

val popupBodyColors: List<Int> = listOf(Color.Blue.toArgb()),

val popupVerticalOffset: Dp = CFRPopup.DEFAULT_VERTICAL_OFFSET.dp,

val dismissButtonColor: Int = Color.Black.toArgb(),

val dismissOnBackPress: Boolean = true,

val dismissOnClickOutside: Boolean = true,

val overlapAnchor: Boolean = false,

val indicatorDirection: IndicatorDirection = IndicatorDirection.UP,

val indicatorArrowStartOffset: Dp = CFRPopup.DEFAULT_INDICATOR_START_OFFSET.dp,

)

jonalmeida · 2022-10-12T22:08:40Z

components/compose/cfr/src/main/java/mozilla/components/compose/cfr/CFRPopupFullscreenLayout.kt

+        println("Mugurel: anchor start?: ${anchor.x}")
+        println("Mugurel: popup start?: ${popupBounds.startCoord}")


jonalmeida · 2022-10-12T22:10:27Z

components/compose/cfr/src/main/java/mozilla/components/compose/cfr/CFRPopupFullscreenLayout.kt

+            ),
+        )
+
+        MaterialTheme {


I'm not well versed in Compose, so disregard accordingly: how does the MaterialTheme interact with our FirefoxTheme if we have it wrapped here?

They both are singletons from where inner composables can request values.
The composables are hardcoded to request a certain value from a certain singleton theme.
For example a platform composable uses values from MaterialTheme - https://cs.android.com/androidx/platform/tools/dokka-devsite-plugin/+/master:testData/compose/source/androidx/compose/material/Card.kt;l=59,60;drc=6fed3de7a56143de954d55e508a7449deb9af582
Or here Fenix composables use values from the FirefoxTheme - https://github.com/mozilla-mobile/fenix/blob/262aa169911853f23afdd94ebf2b0def0b274cb2/app/src/main/java/org/mozilla/fenix/home/recentbookmarks/view/RecentBookmarks.kt#L135-L138

Wrapping composables in themes would allow theme values to be overriden

like I did here for in place overrides or

as shown here for general overrides which would then require wrapping all platform composables with that new theme which overrides all MaterialTheme colors.

but we don't do/need that here so all references to MaterialTheme can be removed.

Tested on Focus with Gabi's patch from mozilla-mobile/focus-android#7705 and saw everything working just as before.

components/compose/cfr/src/main/java/mozilla/components/compose/cfr/CFRPopupContent.kt

jonalmeida · 2022-10-12T22:21:42Z

components/compose/cfr/src/main/res/values/strings.xml

+   - License, v. 2.0. If a copy of the MPL was not distributed with this
+   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
+<resources>
+    <string name="mozac_cfr_dismiss_button_content_description">Dismiss</string>


Needs translation description above this, right?

jonalmeida · 2022-10-12T22:22:41Z

components/compose/cfr/src/test/resources/robolectric.properties

@@ -0,0 +1 @@
+sdk=28


We shouldn't need this anymore with robolectric upgraded now.

jonalmeida · 2022-10-12T22:23:48Z

...ompose/cfr/src/main/java/mozilla/components/compose/cfr/helper/DisplayOrientationListener.kt

+    private val displayManager = context.getSystemService(Context.DISPLAY_SERVICE) as DisplayManager
+
+    @VisibleForTesting
+    internal var currentOrientation = -10


Where did -10 come from?

Wanted an invalid value to start with to ensure the below check display.rotation != currentOrientation will always pass.
Should've been better documented.
Instead of using a magic number SCREEN_ORIENTATION_UNSET/UNSPECIFIED would better suffice.

...ompose/cfr/src/main/java/mozilla/components/compose/cfr/helper/DisplayOrientationListener.kt

mergify · 2022-10-12T22:28:42Z

This pull request has conflicts when rebasing. Could you fix it @Mugurell? 🙏

mergify · 2022-10-18T14:24:32Z

This pull request has conflicts when rebasing. Could you fix it @Mugurell? 🙏

Mugurell · 2022-10-20T15:36:24Z

Not sure why the CI tasks are stuck.
Restarting them.

Mugurell · 2022-10-20T16:48:13Z

Seems like firefoxci-taskcluster / toolchain-linux64-android-gradle-dependencies failed because of

[task 2022-10-20T16:13:03.496Z] * What went wrong:
[task 2022-10-20T16:13:03.496Z] java.lang.StackOverflowError (no error message)

in :compose-cfr:compileDebugKotlin (compose-cfr is the newly added module)

Locally this builds fine.
@gbrownmozilla Do you know what could cause the error?

This upstreams the CFR composable already used on Fenix allowing it to be reused on other projects also. The setup process requires quite a few parameters because as it is highly customizable supporting different indicator orientations or positionings in relation to the anchor and also supporting RTL.

Mugurell · 2022-10-24T12:19:03Z

Seems like firefoxci-taskcluster / toolchain-linux64-android-gradle-dependencies failed because of

[task 2022-10-20T16:13:03.496Z] * What went wrong:
[task 2022-10-20T16:13:03.496Z] java.lang.StackOverflowError (no error message)

in :compose-cfr:compileDebugKotlin (compose-cfr is the newly added module)

Locally this builds fine. @gbrownmozilla Do you know what could cause the error?

Seems like this was failing because of the freeCompilerArgs argument where I was using -Xjvm-default=enable which was deprecated in Kotlin 1.7 which we recently upgraded to.
Changing to -Xjvm-default=all allows the project to build.

Mugurell · 2022-10-24T12:19:52Z

build-feature-downloads failed because of #11994.
Restarting the task.

Mugurell · 2022-10-26T16:32:42Z

@Mergifyio backport releases/107.0

mergify · 2022-10-26T16:33:28Z

backport releases/107.0

✅ Backports have been created

#13015 For #12855 - Upstream the CFRPopup from Fenix to AC to share it in multiple projects (backport #12828) has been created for branch releases/107.0

Mugurell requested review from a team as code owners September 22, 2022 16:06

Mugurell requested review from gbrownmozilla and removed request for a team September 22, 2022 16:06

Mugurell marked this pull request as draft September 22, 2022 16:06

Mugurell commented Sep 22, 2022

View reviewed changes

Mugurell force-pushed the cfrPopup branch from 8baee4c to 369a8a9 Compare September 28, 2022 12:50

Mugurell changed the title ~~Uplifting the CFRPopup from Fenix to AC to share it in multiple projects~~ For 12855 - Upstream the CFRPopup from Fenix to AC to share it in multiple projects Sep 28, 2022

Mugurell changed the title ~~For 12855 - Upstream the CFRPopup from Fenix to AC to share it in multiple projects~~ For #12855 - Upstream the CFRPopup from Fenix to AC to share it in multiple projects Sep 28, 2022

Mugurell marked this pull request as ready for review September 28, 2022 13:05

Mugurell force-pushed the cfrPopup branch from 369a8a9 to 24e8187 Compare September 29, 2022 15:04

Mugurell added the 🕵️‍♀️ needs review PRs that need to be reviewed label Sep 29, 2022

gbrownmozilla approved these changes Sep 29, 2022

View reviewed changes

jonalmeida approved these changes Oct 12, 2022

View reviewed changes

iorgamgabriel requested review from jonalmeida and a team October 14, 2022 10:02

Mugurell force-pushed the cfrPopup branch from 24e8187 to 65024be Compare October 18, 2022 13:49

Mugurell force-pushed the cfrPopup branch from 65024be to 5802218 Compare October 20, 2022 09:12

jonalmeida approved these changes Oct 20, 2022

View reviewed changes

Mugurell closed this Oct 20, 2022

Mugurell reopened this Oct 20, 2022

jonalmeida closed this Oct 21, 2022

jonalmeida reopened this Oct 21, 2022

Mugurell force-pushed the cfrPopup branch from 5802218 to ccf0097 Compare October 24, 2022 10:35

Mugurell added 🛬 needs landing PRs that are ready to land and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Oct 24, 2022

mergify bot merged commit 96444dd into mozilla-mobile:main Oct 24, 2022

Mugurell deleted the cfrPopup branch October 24, 2022 12:51

mergify bot mentioned this pull request Oct 26, 2022

For #12855 - Upstream the CFRPopup from Fenix to AC to share it in multiple projects (backport #12828) #13015

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For #12855 - Upstream the CFRPopup from Fenix to AC to share it in multiple projects #12828

For #12855 - Upstream the CFRPopup from Fenix to AC to share it in multiple projects #12828

Mugurell commented Sep 22, 2022 •

edited by github-actions bot

Mugurell Sep 22, 2022

jonalmeida Sep 23, 2022

jonalmeida Sep 23, 2022

Mugurell Sep 28, 2022 •

edited

gbrownmozilla left a comment

jonalmeida left a comment

jonalmeida Oct 12, 2022

jonalmeida Oct 12, 2022

jonalmeida Oct 12, 2022

jonalmeida Oct 12, 2022

Mugurell Oct 18, 2022

jonalmeida Oct 12, 2022

jonalmeida Oct 12, 2022

jonalmeida Oct 12, 2022

Mugurell Oct 18, 2022 •

edited

mergify bot commented Oct 12, 2022

mergify bot commented Oct 18, 2022

Mugurell commented Oct 20, 2022

Mugurell commented Oct 20, 2022 •

edited

Mugurell commented Oct 24, 2022

Mugurell commented Oct 24, 2022

Mugurell commented Oct 26, 2022

mergify bot commented Oct 26, 2022

		println("Mugurel: anchor start?: ${anchor.x}")
		println("Mugurel: popup start?: ${popupBounds.startCoord}")

		@@ -0,0 +1 @@
		sdk=28

For #12855 - Upstream the CFRPopup from Fenix to AC to share it in multiple projects #12828

For #12855 - Upstream the CFRPopup from Fenix to AC to share it in multiple projects #12828

Conversation

Mugurell commented Sep 22, 2022 • edited by github-actions bot

Pull Request checklist

After merge

GitHub Automation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mugurell Sep 28, 2022 • edited

Choose a reason for hiding this comment

gbrownmozilla left a comment

Choose a reason for hiding this comment

jonalmeida left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mugurell Oct 18, 2022 • edited

Choose a reason for hiding this comment

mergify bot commented Oct 12, 2022

mergify bot commented Oct 18, 2022

Mugurell commented Oct 20, 2022

Mugurell commented Oct 20, 2022 • edited

Mugurell commented Oct 24, 2022

Mugurell commented Oct 24, 2022

Mugurell commented Oct 26, 2022

mergify bot commented Oct 26, 2022

✅ Backports have been created

Mugurell commented Sep 22, 2022 •

edited by github-actions bot

Mugurell Sep 28, 2022 •

edited

Mugurell Oct 18, 2022 •

edited

Mugurell commented Oct 20, 2022 •

edited