POC Unified Chrome Management #3311
Conversation
rahulsainani
force-pushed
the
unified-chrome-management-poc
branch
from
0a26dda to
bf331b7
Compare
rahulsainani
force-pushed
the
unified-chrome-management-poc
branch
from
bf331b7 to
a94c627
Compare
| TranslationYOnDrawFetcher( | ||
| toolbar, | ||
| onDraw = { offset -> viewModel.updateToolbarOffset(offset) } | ||
| ) |
| * | ||
| * @param browserStore The [BrowserStore] instance user to observe state. | ||
| */ | ||
| class MainContainerViewModel( |
There was a problem hiding this comment.
This seems like a new idiom for the reference browser.
Why not use a Store?
If the intent is to merge this patch as is and present the code here as the reference way of building a browser it seems like the patch does significantly more than allowing to share the browser toolbar and with using a ViewModel as state holder directly written to/read from goes against our MVI.
Asking this because I think the Stores (at least how heavy they are in Fenix) are probably too slow to support animations (like updating toolbarOffset). If you saw the same here maybe it's indicative of us needing to refine the Store API / related idioms but this seems to be a larger discussion.
Also I'm thinking more and more that panels that want to scroll together with the toolbar should be part of the same "toolbarContainer" that would allow to be composed dynamically and ensure everything is scrolled as one.
| * https://medium.com/androiddevelopers/viewcompositionstrategy-demystefied-276427152f34 | ||
| * https://developer.android.com/develop/ui/compose/migrate/interoperability-apis/compose-in-views#composition-strategy | ||
| */ | ||
| abstract class ComposeFragment : Fragment() { |
There was a problem hiding this comment.
As a separate note: It would be nice to upstream and use this from components.compose.base
| (toolbar.layoutParams as? CoordinatorLayout.LayoutParams)?.apply { | ||
| behavior = EngineViewScrollingBehavior( | ||
| view.context, | ||
| null, | ||
| ViewPosition.BOTTOM, | ||
| ) | ||
| } |
There was a problem hiding this comment.
As a small note, we're already working with Gecko - APZ to expose the scroll data through GeckoView and hoping in a few months we'll be able to do this more elegantly - through the usual nested scrolling framework.
But for now it's great that translating the toolbar based on a child fragment is supported by EngineViewScrollingBehavior.
| @@ -46,7 +49,7 @@ open class BrowserActivity : AppCompatActivity() { | |||
| * Returns a new instance of [BrowserFragment] to display. | |||
| */ | |||
| open fun createBrowserFragment(sessionId: String?): Fragment = | |||
There was a problem hiding this comment.
Having BrowserActivity call createBrowserFragment which would create MainContainerFragment seems forced.
RB uses one activity per feature so if the feature now is "Main.."
Maybe this should be renames to MainActivity & createMainFragment.
Speaking of which, why do we need a fragment parent for home & browser and not just use this Activity?
| engineView.setDynamicToolbarMaxHeight(resources.getDimensionPixelSize(R.dimen.browser_toolbar_height)) | ||
| } | ||
| parentFragment?.parentFragmentManager |
There was a problem hiding this comment.
Communicating between fragments or with the parent should normally be done through a parent ViewModel or in our case a Store.
Or as it happens in many cases and if we know that our parent is showing a toolbar we could just access it directly.
Curious why going with the fragment result instead.
How
– Based on the proposal.
– Add
MainContainerFragment. Move chrome related "features" fromBrowserFragmenttoMainContainerFragment.– Add
HomeFragment.–
HomeFragmentandBrowserFragmentare children of theMainContainerFragment. One of those are navigated to using the nav graph based on the selected tab.– Add mechanisms to share state between parent and child fragments using
MainContainerViewModel. Showcase how the parent fragment can share info about the chrome position and how child fragment views (e.g. find in page UI) can use that. Similarly views on HomeFragment .– ExternalBrowser uses a different layout with the UI elements it needs - find in page, toolbar, engine view and the same coordinator layout behaviour setup.
– Setup specific external browser features in ExternalBrowserFragment
Video
RB-NewTab.mp4
Feedback
– Please focus on the idea and the POC and not on the nits.
– I'd really appreciate if folks can highlight some edge cases and share solutions along with the questions too :)