Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable massive amount of tabs tray customization options! #65

Conversation

abhijitvalluri
Copy link
Collaborator

This PR enables a totally insane amount of customization options for the tabs tray. For those who don't know, the "Tabs Tray" is the UI element that pops up (as a Bottom Sheet by default) with the list of open tabs, a button to save them to a collection and (if enabled in the secret settings) your synced tabs from other devices.

Customization options

This PR enables the following customization options from the "Customize" menu item in settings:

  • Enable compact tabs
    • Tabs are narrow and arranged in a grid, similar to how they were in the old Fennec version (v68). This was recently made mandatory by my earlier PR, but now it is configurable! This resolves issue Tab tray: let people disable compact tabs #56!
  • Enable top tabs tray
    • Tab tray opens from the top of the screen. This can be useful when the user chooses the toolbar to be at the bottom, so that the new tab button is easier to reach. My previous PR on this made this the mandatory layout when toolbar is placed at the bottom. But now it is independently configurabe and also resolves [Bug] tab tray: wrong side with bottom nav bar #55!
  • Reverse tab order in tray
    • This reverses the order of the tabs in the tray so that new tabs appear towards the start of the tray. Note that the start of the tray means bottom of the tray when "top tabs tray" is enabled. This brings back the default ordering of tabs in Fenix!
  • Enable FAB (Floating action button) for new tab
    • This brings back the FAB that was originally present in Fenix that I removed in my previous PR related to this.
  • Place FAB at the top
    • This places the FAB at the top of the screen! A totally mad idea! Completely against material UI guidelines! But oh well, you gotta give users the power to do it! That is the firefox iceweasel (pending name change) way!

Note that this PR incorporates all the customization options implemented in #58 by @miDeb and adds a lot more options on top of that!

Default configuration

The default configuration on a fresh install is:

  • Disable compact tabs
  • Disable top tabs tray
  • Enable reverse tab order in tray
  • Enable FAB
  • Place FAB at the bottom

This setup gives you the original Fenix layout, hence it makes sense to use this configuration to make the UI similar to Fenix for the least disruption.

In addition to these customization options, I updated the layout in the list view (i.e. non-compact tabs view) to show the site favicon as well.

Finally, I tweaked the "Customize" settings page to add dividers between sections so that it is easier to read and navigate. Given that we are adding a lot of settings in this page, it becomes very busy and difficult to follow without these dividers.

This is the most configuration I can enable and willing to enable. There is not much more to do, and this was already quite a pain! 😅

NOTE: I disabled a bit of code in TabTrayView.kt that was causing a crash when accessibilty settings (such as TalkBack) is enabled. This crash is related to this issue on the upstream Fenix repo that I created: mozilla-mobile#14540. When the issue is resolved upstream, we will need to put some effort to merge it as it will very likely cause lot of merge conflicts!

Screenshots

Customization options (with default configuration enabled)

Various tabs tray layouts (not exhaustive)

@miDeb
Copy link

miDeb commented Sep 4, 2020

Two issues with the top tab tray (not directly related to this pr):

  • The line of the selected tab (normal vs private tabs) should be rendered above, along the divider. I think that would look better.
  • The enter/exit animation looks wrong. It still fades in from the bottom.

@miDeb
Copy link

miDeb commented Sep 4, 2020

There's an spacing issue with compact tabs + reverse tab order:
Screenshot_20200904-120212_Iceweasel Preview

@interfect interfect merged commit 7d2591e into fork-maintainers:fork Sep 5, 2020
@abhijitvalluri
Copy link
Collaborator Author

abhijitvalluri commented Sep 5, 2020

Two issues with the top tab tray (not directly related to this pr):

* The line of the selected tab (normal vs private tabs) should be rendered above, along the divider. I think that would look better.

* The enter/exit animation looks wrong. It still fades in from the bottom.

@miDeb the first point I will address in a new PR. The second one I am not going to (at least for now). I can see that in a few scenarios the animation is still going to the bottom. I feel that because this is a hack of the BottomSheetBehavior, some of the behavior in TopSheetBehavior still corresponds to the original Bottom sheet one. Not sure where the issue is. It may be possible to fix but I could not find out when I investigated it, so unsure if it is fixable.

There's an spacing issue with compact tabs + reverse tab order:

Good catch! I always tested with 3 or more tabs, so this issue never occured to me. Unfortunately this is most likely impossible to fix. The GridLayoutManager does not support stackFromEnd. So, originally Mozilla used a LinearLayoutManager that can do both stackFromEnd and reverseLayout. They use both to basically make the order of the tabs reverse (by reverse layout) and then make sure that the placement of the items is from the end. The second thing makes sure that the tabs are always at the top even in when the layout is reversed, because when you reverse the layout the top of the screen becomes the end, so stacking from the end makes sure that the tabs are at the top.

Because GridLayoutManager does not support stack from end this is impossible to fix.

Now, you may wonder why not just reverse the list of tabs and do nothing else by doing something like tabList.reverse() in the tabsAdapter? Unfortunately this is not a good solution. While visually it works perfectly, it totally breaks the tab closing functionality. Mozilla does something quite intricate for closing a tab that I could not fully follow. But basically what happens is when you close tab number n, it removes the nth item from the layout. If you reverse the layout, then the item order is reversed. So, for example, if you close the last tab as it shows in the layout, it removes the first tab from the layout (as it is technically first in the list)!! So visually you see the wrong tab getting closed even though the correct tab is closed.

So, due to the above limitations I am simply going to disable reverse layout for compact tabs unfortunately.

@abhijitvalluri
Copy link
Collaborator Author

#69 addresses the issues raised above (except the top sheet behavior issue).

@miDeb
Copy link

miDeb commented Sep 5, 2020

@abhijitvalluri I figured out a way to fix the top tab tray animation! #72 It wasn't related to the custom TopSheetBehavior, but rather just the default dialog opening animation I think...

@sheikh-azharuddin
Copy link

You people are doing really great job better than Firefox paid devs👍👍
Keep up your good work... Once it is released officially in play store people will prefer this than fenix

@gabrielluong
Copy link

gabrielluong commented Sep 18, 2020

Hey @abhijitvalluri, I am working on Grid View on Fenix in mozilla-mobile#15115. Would you be interested in upstreaming your changes to Fenix to match the designs? We are slightly time sensitive to finish the issue in roughly less than a week to make the October Release with enough time to QA and user test. In the meantime, I want to acknowledge your amazing work here and make sure you know I will be basing it off your PR, adding you as a co-author, and will do anything else in my power to make sure we acknowledge your contribution. Thanks!

@abhijitvalluri
Copy link
Collaborator Author

@gabrielluong that sounds great! Feel free to base your PR off my code here. I'll be happy to see this feature in the official firefox as well! Do you mean to say that you will make these changes in the upstream Fenix code or are you requesting me to make those changes? If you are working on it, I will let you continue working on the code and make the changes. You may be more aware of how to incorporate this properly in a mozilla-compatible fashion. I am happy to help and explain any of the code I wrote. 🙂

@gabrielluong
Copy link

gabrielluong commented Sep 19, 2020

@gabrielluong that sounds great! Feel free to base your PR off my code here. I'll be happy to see this feature in the official firefox as well! Do you mean to say that you will make these changes in the upstream Fenix code or are you requesting me to make those changes? If you are working on it, I will let you continue working on the code and make the changes. You may be more aware of how to incorporate this properly in a mozilla-compatible fashion. I am happy to help and explain any of the code I wrote. 🙂

@abhijitvalluri My initial request was to see if you wanted to upstream your changes (by you). Otherwise, I wanted to let you know that I would be willing to do it with all the kudos given. Also, I wanted to convey that we're looking to release this feature for the October release with enough time for it to be in the build for QA and user testing time. So, there is a bit of a time pressure for the work (needed by either of us), and I wanted to get that across because if you chose to upstream it. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] tab tray: wrong side with bottom nav bar
5 participants