Refine tab's element position

[cezaraugusto]

• Submitted a ticket for my issue if one did not already exist.
• Used Github auto-closing keywords in the commit message.
• Ran git rebase -i to squash commits (if needed).

Auditors: @bsclifton, @bbondy, @bradleyrichter, @jkup

Fix #5431
Fix #6511

This PR follows this @bradleyrichter spec for tabs:

Test Plan:

• All previous tab's test must pass:

npm run test -- --grep="tab tests"

Manual Tests

• Open 20 tabs with the following included:
• new session with audio
• new session without audio
• private with audio
• private without audio
• default with audio
• default no audio
• about:newtab or about:blank
• Add any other 2 tabs
• Resize window's width
• At each random breakpoint, tab's content should work based on above spec
• Icons must be hidden/shown gracefully (no icons overflowing tab's container)
• There should be no UI inconsistency

[cezaraugusto]

Setting as WIP since it doesn't fit well when user has 20 tabs open.

[bsclifton]

While reviewing #6031, I had some feedback for this PR

Next time we look at this (perhaps after 0.13.0 is closed out), we can consider making the tab size responsive like we had talked about 😄 For example, there could be 4 "breakpoints" for a tab:

• (smallest) - only show the favicon (not private/session, audio, or close button). This will then look like a pinned tab.
• (smaller) - show both the favicon and the audio. Similar to how a pinned tab works.
• (larger) - show favicon, audio, and close button
• (full size) - show favicon, audio, private or session, and close button

cc: @bradleyrichter

Some other interesting food for thought 😄

• Allow for unlimited tabs (having tabs continue to resize) #879
• Tabs don't resize with window #100

[cezaraugusto]

@bsclifton agree. My suggestion is more like to use throttle for window resize and grab each tab's size using .getBoundingClientRect based on that. This way we can add a class for each view we want, and can also easily scale if some viewport is not at its best. I'm pretty sure it will be a definitive fix for all our tab-resizing issue.

[bsclifton]

@cezaraugusto you'll need to work out the conflicts (#6031 was just merged), but you should be good to finish the rest of your work now 😄

Here's the image that @bradleyrichter shared in the other issue (and over Slack):

[cezaraugusto]

PR updated, ready for review

[cezaraugusto]

Rebased but tagged as WIP for further improvements.

[bsclifton]

@cezaraugusto what's left on this one? have you gotten the feedback you need?

cc: @bradleyrichter, @jkup

[cezaraugusto]

@bsclifton I would like some UI feedback from @bradleyrichter and @jkup first. I rebased this PR and cherry-picked on auto-suggest-sites branch, so it's good to test both here or there.

From my part, WIP as I'll need to tweak it again to keep it working with ImmutableComponent.

[jkup]

Taking a look now!

[bradleyrichter]

I'll paste some snaps of little issues I am finding. But overall, it's awesome! Really great work here @cezaraugusto !

[bradleyrichter]

First, to see these remaining issues, set the tabs per set to 20 and reduce the window width to the minimum allowed.

With 20 tabs, the favicons disapear until you grab the window and resize it again, followed by reducing back to minimum. The icons then reappear.

Next, after they repaint, you can see a white oval present in the selected tab.

Another issue is that a tab which contains no favicon shows the close button instead. While it does work, it should only appear on mouse hover, regardless of the tab width.

(fine print: in order to get NPM to run, I merged master into the PR which could be causing issues.)

[bsclifton]

Great job, @cezaraugusto 😄 Lots of small details, but you're getting really close on this one. I think these changes will have a huge positive impact on our users 😄

[cezaraugusto]

thanks @bradleyrichter @bsclifton. It would need another set of eyes after ready since I'm migrating all tab component to Aphrodite, but I took note of those issues and will be fixed on the next request.

[luixxiul]

@bradleyrichter @cezaraugusto

Another issue is that a tab which contains no favicon shows the close button instead

How about hiding the close button at all under the certain minimum width of the tab?

[cezaraugusto]

@luixxiul I like that. I think for the smallest possible tab width we should replicate pinned tabs. If needed we could resize favicon (i.e. from 16px to 14px) just to make sure it fits their container.

[cezaraugusto]

Having closeTab icon on hover would make it difficult to select the tab if we have a lot of them open. Mimicking pinned seems a good solution for that as well. @bradleyrichter WDYT?

[bradleyrichter]

We do have a problem to solve which is how to display the entire tab set of tabs when the user minimizes the window. Setting min-width will cause overflow but we should deal with it anyway.

[bradleyrichter]

Upon reviewing the latest version, it is nearly perfect!

Hhere are some remaining UI related issues that would be good to solve:

1. Close button overlap at small tab widths.

before hover:

on hover:

zoomed view:

proposed fix: tell string to truncate further on mouse hover.

2. Favicon moves over at time of mouse over.

Not sure if this only happens below a certain width. I think so...but it only happened with a fresh set of tabs.

3. We need to solve the colored-tab hiding the favicon issue.

proposed solution: place a 16x background square behind the favicon when the tab text is white. The color can be 50% of the current tab color. This would only be applied with tab theme colors enabled.

[cezaraugusto]

Closed in favour of #6900