Skip to content
This repository has been archived by the owner on Jun 17, 2024. It is now read-only.

Bug 1875465 - Add tab strip for tablets #5218

Merged

Conversation

rahulsainani
Copy link
Contributor

@rahulsainani rahulsainani commented Jan 19, 2024

What

– Add Secret Settiings - "Enable Tab Strip" that's visible only on tablets.
– Display open tabs in a Tab Strip above toolbar
– Add Ability to add a new tab and close tabs.

While these are not features of the Tab Strip UI, these are changes are in Settings when tab strip is enabled:
– toolbar customization in Settings is not shown as Toolbar defaults to Top.
– Swipe tab gesture to switch tabs in settings is not shown as it is disabled.

How

– Update toolbar position based on form factor. This required replacing usages of settings.shouldUseBottomToolbar to settings.toolbarPosition.
ToolbarPosition is defaults to TOP when it's a tablet. This is done to simplify the implementation.
– Add dimensions to help update layouts and positioning logic
– Display TabStrip on HomeFragment and BrowserFragment. It would be ideal to join the two fragments but that's outside the scope right now.
– Add TabStrip composable that provides callbacks for adding a tab, selecting a tab and removal of the last tab. These are used by BrowserFragment and HomeFragment differently.
– Create TabUIState model that's mapped from TabSessionState. This simplifies the observation and the only contains information required to render the TabStrip. Also helps in recompositions since only if the content in this model changes would it recompose.
– LazyRow to show Tabs

Preview

tablet-tab-strip-3.webm

Known issues

There's ghost padding at the bottom of EngineView. Keener eyes than mine could help solve this.

Non exhastive list of Todos

  • Add secret settings feature flag
  • Remove customize toolbar setting for tablets
  • Update scroll when selecting tabs items that are partially visible.
  • TabStripState can be marked @Immutable to make compose compiler aware so it doesn't need to recompose when the list doesn't change. This can also be solved by using Immutable collections from kotlinx.
  • Same tab width for all tabs based on number of tabs.
  • Display url when content.title is blank.
  • Hide tab strip when full screen.
  • Tab strip should scroll up when toolbar scrolls up with content.

Note for reviewers

This isn't pulling from a specific design but is using the usual design system tokens.

26 Feb Update

Any remaining bugs and functionality will be tracked separate as part of this meta.


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

  1. Click on Checks at the top of the PR page.
  2. Click on the firefoxci-taskcluster group on the left to expand all tasks.
  3. Click on the build-apk-{fenix,focus,klar}-debug task you're interested in.
  4. Click on View task in Taskcluster in the new DETAILS section.
  5. 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=1875465

@github-actions github-actions bot added the work in progress Not ready to land yet. Work in progress (WIP). label Jan 19, 2024
@rahulsainani rahulsainani requested a review from a team January 19, 2024 13:27
@rahulsainani rahulsainani force-pushed the 1875465-tab-strip-for-tablets branch 3 times, most recently from bb0be7a to 2d0a2f8 Compare January 22, 2024 11:00
@Kuuh4
Copy link

Kuuh4 commented Jan 26, 2024

Very Nice! Congratulations and thanks!

@riddlerboss248
Copy link

Would love to have it on mobile too. Like how brave(ios), Vivaldi and opera have it.

@NVins
Copy link

NVins commented Jan 27, 2024

Please also add a tablet tab bar for the mobile version of Firefox Android.

@danieljl
Copy link

Thanks! Please also consider adding an option to use the old (current) tab UI for users who prefer it on their smaller tablets.

@sukhrob-shakirov
Copy link

Finally!! thanks

@tkompis
Copy link

tkompis commented Feb 9, 2024

Hello! Is it possible to download an APK to test/try this feature?

Edit: After 5 minutes of more carefull re-reading, I understood how to get the APK. Installed it on my S6 Lite, and it's superb. Are there any problems using this preview as my main Firefox browser? Signing in with my Firefox account etc?

@tkompis
Copy link

tkompis commented Feb 9, 2024

A recommendation that popped in my mind, is the addition of a tab width limiter/lock. Depending on the length of a website's name, all tabs are different lengths on the tab bar currently.

@fbcbl
Copy link

fbcbl commented Feb 16, 2024

soo good! need this shipped!

@rahulsainani rahulsainani force-pushed the 1875465-tab-strip-for-tablets branch 3 times, most recently from 7556e1a to 47647ed Compare February 16, 2024 15:37
@Nick768
Copy link

Nick768 commented Feb 16, 2024

Ok, I've tested it and I have to say, it works! I know it crashes sometimes, but this could also be because it is built with an alpha level gecko version. The only problems so far:

  1. Some Tabs doesn't show titles (I think every github tab)
  2. The blank area at the bottom (it has exactly the same size as the normal FF navigation bar, therefore shouldn't be hard to find when searching for references to it)
  3. In fullscreen mode (e.g. YouTube fullscreen playback) the tabbar is still there. That shouldn't be of course
  4. It should be done quick because I need it so much but don't have time to contribute :-)

But after all this is what every tablet user is waiting for. Since it isn't production ready I have to stick with Firefox in Termux with some other drawbacks...

Edit to 1: this seems to be the case if a tab doesn't have a title. Check out duckduckgo.com and search something

@Nick768
Copy link

Nick768 commented Feb 16, 2024

Thank you, you are amazing! Everything is fixed, except YouTube fullscreen, but my Tablet has a 16:10 screen, therefore a 16:9 video fits even with the tab bar shown...

Now somebody has to figure out how to enable desktop like key combos like CTRL + W, CTRL + T, CTRL + TAB and so on...

@girst
Copy link

girst commented Feb 17, 2024

Thank you for working on this!

Just looking at tablet-tab-strip.webm, it feels very large. On desktop the tall tabs don't take up much space relative to the screen, but on smaller tablets every pixel counts. I hope you are considering an option to make them smaller, like browser.uidensity;1 on desktop.

@Aasikki
Copy link

Aasikki commented Feb 19, 2024

Thank you for working on this!

Just looking at tablet-tab-strip.webm, it feels very large. On desktop the tall tabs don't take up much space relative to the screen, but on smaller tablets every pixel counts. I hope you are considering an option to make them smaller, like browser.uidensity;1 on desktop.

Yeah this is awesome otherwise but the tabs are absolutely huge even on a tab with 14.6" display.

@rahulsainani
Copy link
Contributor Author

Thanks everyone for already trying and testing this version and the feedback, it's really appreciated! 🙌 I will try to summarize some of the updates and address some of the questions in this message. The video now shows the updated version and shows that the old/current tabs tray, that is not going anywhere.

@tkompis tab width should be the same now. This PR is on top of last week's main branch so all the other functionalities should work if you want to set it as your default. However, you would miss out on the other improvements that keep happening on the main branch, till this is merged.
@Nick768 full screen issue should also be solved now too. Could you please share when/how the app crashed, that really shouldn't be happening. The shortcuts are a great idea, we've talked about that, but that won't be part of this PR.
@girst @Aasikki Valid points! Could you please check if the tabs are still large on the latest update? If so, could you please share the device details and possiblly a link to the screenshot?

Thanks again!

@tkompis
Copy link

tkompis commented Feb 19, 2024

Thanks everyone for already trying and testing this version and the feedback, it's really appreciated! 🙌 I will try to summarize some of the updates and address some of the questions in this message. The video now shows the updated version and shows that the old/current tabs tray, that is not going anywhere.

@tkompis tab width should be the same now. This PR is on top of last week's main branch so all the other functionalities should work if you want to set it as your default. However, you would miss out on the other improvements that keep happening on the main branch, till this is merged. – @Nick768 full screen issue should also be solved now too. Could you please share when/how the app crashed, that really shouldn't be happening. The shortcuts are a great idea, we've talked about that, but that won't be part of this PR. – @girst @Aasikki Valid points! Could you please check if the tabs are still large on the latest update? If so, could you please share the device details and possiblly a link to the screenshot?

Thanks again!

The updated video is showing as corrupt, I can't see it.

I've decided to be patient and wait for it to be merged into the main branch other than using it right now. Still, superb work, and it shows why Firefox is the best browser. I wish I was more knowledgeable in terms of programming, to help!

@Aasikki
Copy link

Aasikki commented Feb 19, 2024

Thanks everyone for already trying and testing this version and the feedback, it's really appreciated! 🙌 I will try to summarize some of the updates and address some of the questions in this message. The video now shows the updated version and shows that the old/current tabs tray, that is not going anywhere.

@tkompis tab width should be the same now. This PR is on top of last week's main branch so all the other functionalities should work if you want to set it as your default. However, you would miss out on the other improvements that keep happening on the main branch, till this is merged. – @Nick768 full screen issue should also be solved now too. Could you please share when/how the app crashed, that really shouldn't be happening. The shortcuts are a great idea, we've talked about that, but that won't be part of this PR. – @girst @Aasikki Valid points! Could you please check if the tabs are still large on the latest update? If so, could you please share the device details and possiblly a link to the screenshot?

Thanks again!

Yeah the tabs still seem to be the same size.
Screenshot_20240219_132733_Firefox Fenix

Here's chrome on the same tab for comparison.
Screenshot_20240219_132726_Chrome

This is on a Samsung Galaxy Tab S9 Ultra.
I also noticed that there probably should be an animation when making a new tab.
Huge thinks for working on this, can't wait to also switch to Firefox on the tab!

@rahulsainani rahulsainani force-pushed the 1875465-tab-strip-for-tablets branch 2 times, most recently from 9205395 to 5430c0f Compare February 19, 2024 13:41
@rahulsainani
Copy link
Contributor Author

@tkompis Refreshing the page should work. I've updated the video again, maybe now you can see it. Your help in testing and feedback has been great!

@Aasikki Thanks for sharing the screenshots! The tabs should adjust their width when there are more tabs. I've reduced the height and updated the paddings so it looks a bit more clean. The new tab animation might be outside the scope of this change since we open the home page on clicking new tab.

@rahulsainani rahulsainani marked this pull request as ready for review February 19, 2024 16:45
@rahulsainani
Copy link
Contributor Author

rahulsainani commented Feb 27, 2024

Thanks @MozillaNoah for a very thorough review! 🙌 As discussed offline, we want to iterate on it using the meta and create any follow ups there. If there are things we haven't addressed here, we will in the future.

@rahulsainani
Copy link
Contributor Author

Thanks everyone for testing it out, sharing the feedback and your excitement! It's great to see! I'll try to address more recent feedback here

@Kuuh4 We'll look into it!
@IrradiatedHaggis shortcuts would be great! it's something we'd like to add as well, it's outside the scope of this feature which still needs work.
@regs01 Some of the designs will update as we iterate, also the reason why this is behind secret settings right now. Tab strip now scrolling up with content is a known issue right now.

🙌

@rahulsainani rahulsainani added 🛬 needs landing PRs that are ready to land approved PR that has been approved and removed 🕵️‍♀️ needs review PRs that need to be reviewed labels Feb 27, 2024
@mergify mergify bot merged commit 792ca98 into mozilla-mobile:main Feb 27, 2024
23 checks passed
@rahulsainani rahulsainani deleted the 1875465-tab-strip-for-tablets branch February 27, 2024 10:41
@systema-encephale
Copy link

I have tried to get the tab strip while using Firefox Nightly in the Samsung DeX desktop mode. I can see and activate the tab strip setting in the Secret Settings menu, but the strip itself does not appear. Is there something else I need to set?

@rahulsainani
Copy link
Contributor Author

@systema-encephale that's odd, were there any open tabs or did you open any new tabs? If so could you please share a video?

@systema-encephale
Copy link

systema-encephale commented Feb 28, 2024

@rahulsainani Ah, false alarm: I force-stopped Firefox and cleared the cache, and now the tabs are there. This is great, thank you for this addition!

@rahulsainani
Copy link
Contributor Author

@systema-encephale good to know! Thanks for the confirmation 🙌

@Kaoxt
Copy link

Kaoxt commented Feb 29, 2024

Also as a suggestion, what if you made the tabs like the desktop? Have only the active tab with the borders. To me it looks odd visually, in light mode it's hard to distinguish which is the active tab. Also what about fading tabs when scrolling through your open tabs?

The height of tabs I know people are mentioning to make them smaller. I would mzke thr height 1-2px smaller possibly and that is it. Chrome's is too small in my opinion.

(Using a Pixel Tablet)

@NVins
Copy link

NVins commented Mar 2, 2024

Please do so if you have the opportunity:
To enable this horizontal tab bar on phones and not just on tablets.
The ability to move the tab bar down.
Context menu when long pressing on a tab.
And to have a circle or bar animation when loading tabs.
Thank you

@Seddu
Copy link

Seddu commented Mar 2, 2024

Please make it possible to enable the panel on phones as well. The ability to move the tab bar down. Context menu when long pressing on a tab. And that when loading tabs, there was a circle animation, thanks!

I second this. Brave and Vivaldi have it, don’t see an issue having the ability on firefox mobile as well. See images below.
IMG_3585
IMG_3584

@Kaoxt
Copy link

Kaoxt commented Mar 3, 2024

The New Tab button has the oddest behavior. I hope it actually opens a new tab when the fixes come in. It's confusing the way New Tab button works currently. It stays on an existing tab then opens a new tab, which makes no sense.

Firefox Table mode on iPad when opening a new tab works the way I envision the Android Firefox Tablet mode with Nav Strip should work. Is there a reason for the different behavior? I realize this is just getting started, just curious of why the behavior is different.

@marcel263
Copy link

Dear Can it be force enabled based on screen size/resolution instead of detecting the form factor?

Currently sideloaded on Android TV box and option not showing up in the Secret Settings.

@Kaoxt
Copy link

Kaoxt commented Mar 5, 2024

Will there be an option to toggle Home button on/off with this new change or to have the New Tab icon "+" open a new tab like how it is on desktop?

Currently the New Tab Button and Home button do the same thing.

@pigyee
Copy link

pigyee commented Mar 10, 2024

I think the breakpoint should be set to 8 inches, otherwise the experience on a small tablet is not good

Thanks! Please also consider adding an option to use the old (current) tab UI for users who prefer it on their smaller tablets.

@regs01
Copy link

regs01 commented Mar 11, 2024

Another linked issue - address bar panel disappears, when opening find on page panel.

@tkompis
Copy link

tkompis commented Mar 11, 2024

Tried installing again, downloaded the APK today following the steps above, I have no tab strip at all. Tried reinstalling too. Samsung S6 Lite. Do I have to change a setting somewhere? Or is it a bug?

Edit: I should really stop commenting before I've researched it thorough... Got it enabled from the secret settings now.

@rahulsainani
Copy link
Contributor Author

@regs01 The address bar panal disappearing when opening find on page is an existing behaviour, or am i misunderstanding something here?

@NVins @Seddu @marcel263 @pigyee We hear you, we need some time to think these things through, Thanks a lot for the suggestions!

@LostCursor
Copy link

An option to switch between normal tabs and this new tabbed view when folding and unfolding a folding phone, such as the Galaxy Fold 4 or Pixel Fold. Samsung Internet is capable of doing this, and so is Edge for Android.

@Kuron85
Copy link

Kuron85 commented Mar 14, 2024

There needs to be a tab animation when you click "+". It doesn't make sense to have a new tab button in the location it is in without that animation. It's confusing how the current action is and that "+" button and home button do the same action. Why would there be two buttons that do the same action?

I hope this is able to be fixed and we can finally have new tab function like desktop Firefox and all other browsers. I'm hoping with the "+" button being where it's at, it's a sign this will be fixed before going to stable. This would make so many users happy who have issues with the current use of home button and the issues it causes creating new tabs

How I envision it would be:
-Click "+"
-tab animation open a new tab and the title on tab bar says "New Tab" with the usual home items on this tab.

The current method is very confusing.

Great job so far and I really appreciate the work being done on this. Having a tab strip is much needed on Android!

@Alosleon
Copy link

Hello! MiBox Android TV installs only the mobile version of Firefox Android Nightly
Please also add "Enable Tab Strip" in order to be visible and operative for the mobile version
Thanks

aosmond pushed a commit to aosmond/gecko that referenced this pull request Mar 27, 2024
… r=android-reviewers,007

We discussed this scenario in the [[ mozilla-mobile/firefox-android#5218 (comment) | PR adding tab strip ]] and thought simply Favicon could be used, but turns out there are cases where `ContentState.icon` has the bitmap but `Favicon` doesn't return that from the cache.

This still doesn't solve addons.mozilla.org case.

Differential Revision: https://phabricator.services.mozilla.com/D205729
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Mar 28, 2024
… r=android-reviewers,007

We discussed this scenario in the [[ mozilla-mobile/firefox-android#5218 (comment) | PR adding tab strip ]] and thought simply Favicon could be used, but turns out there are cases where `ContentState.icon` has the bitmap but `Favicon` doesn't return that from the cache.

This still doesn't solve addons.mozilla.org case.

Differential Revision: https://phabricator.services.mozilla.com/D205729
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved PR that has been approved 🛬 needs landing PRs that are ready to land
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet