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

[Tabs] TabIndicator width is being calculated before fonts are loaded #7187

Closed
zachwolf opened this issue Jun 19, 2017 · 8 comments
Closed
Assignees
Labels
bug 🐛 Something doesn't work component: tabs This is the name of the generic UI component, not the React module!

Comments

@zachwolf
Copy link
Contributor

Hello!

I believe I've found a small bug. I'm happy to take a look at fixing this, but I could use some direction on what would be the best approach.

Problem description

The width of TabIndicators seem to be calculated before fonts are loaded. Here's an example of what I'm seeing:

Indicator Issue

Notice the indicator is wider than the radial background. Here is an example of the background on Tabs using Helvetica:

Correct Tabs

Both changing tabs and resizing the window fires events that cause the indicator to resize itself correctly.

changing tabs
4b

Link to minimal working code that reproduces the issue

https://github.com/zachwolf/tab-spacing-demo

Versions

  • Material-UI: 1.0.0-alpha.19
  • React: 15.6.0
  • Browser: Chrome 59.0.3071.86

Thanks!

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: tabs This is the name of the generic UI component, not the React module! v1.x.x labels Jun 19, 2017
@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 19, 2017

I have noticed that issue many times. Any idea how to fix it at the library level?

I thought that if this was going to be a painful issue, I would have make so that the default font (not the one loaded) would match the spacing of the loaded one as much as possible.
I'm suggesting a userland fix, a fix with a benefit much broader than just the Tab ink.

@zachwolf
Copy link
Contributor Author

For the Tabs, there's two possible solutions that I can think of.

  1. Restructure the html. It would be possible (although maybe difficult) to give each Tab an indicator and wait until it's interacted with to create another indicator to handle the animation.

  2. Blindly call the getTabsMeta.

An idea for fixing this globally would be to create a component that listens for a font to be loaded. It could broadcast a global event which other components could listen to.

What are your thoughts on these?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 20, 2017

Blindly call the getTabsMeta.

Yeah, we could try calling it a few time right after the first mount hoping for the better (the font to be loaded). This is a naive and poorly performant solution. Let's see if we can do better.

An idea for fixing this globally would be to create a component that listens for a font to be loaded.

I'm not sure we can find something that works across our supported browsers. Oh yes, there is fontfaceobserver.

This issue makes me think that we need a strong story around font loading. This article is a must-read on the topic. This tool is interesting regarding reducing width jump. It can be used to make is imperceptible.

Terminology:

  • FOUT: (Flash of Unstyled Text) browsers display a fallback font in the font stack until the custom one loaded.
  • FOIT: (Flash of Invisible Text) browsers detect if the text was set in a custom font that hasn't loaded yet, and made it invisible until the font did load.

@petermikitsh
Copy link
Contributor

petermikitsh commented Jul 22, 2017

I've thought about this problem for a bit, and upon weighing my options, I decided that implementing an 'initial' indicator (for the first render) on the tab itself, and then using an absolutely positioned indicator on subsequent renders (once the tab index changes). The two indicators are mutually exclusive: once the tab index changes, you transition from using the 'initial' indicator to the one absolutely positioned with respect to all the tabs.

By relying on the width of the tab, the 'initial' indicator automatically reflows when content changes -- so the timing FOUT issue fizzles out.

The strongest upside to this approach is that you don't have to watch fonts or add in some external library that handles watching resize events on DOM elements. Trying to implement watchers for either of those is bound to be filled with corner cases, especially when you're trying to support many different browsers.

You do need to keep an internal boolean in the Tabs component state to keep track of if you have a changed tab index or not.

Before:

screen shot 2017-07-22 at 8 55 35 am

After:

screen shot 2017-07-22 at 8 55 22 am

Here's my implementation but it'd be pretty easy to adapt the concepts:

Initially shown indicator:

https://github.com/collegepulse/material-react-components/blob/master/src/Tabs/Tab.js#L22-L27

New state to keep track of:

https://github.com/collegepulse/material-react-components/blob/master/src/Tabs/Tabs.js#L21-L25

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 22, 2017

@petermikitsh Thanks for sharing your approach but I don't think that we want to go into that direction. We need to address the root of the issue. It's not ok to have the font "jump" that way. Once we solve it, the tab issue is solved for free.

@petermikitsh
Copy link
Contributor

petermikitsh commented Jul 22, 2017

@oliviertassinari To my understanding (and please correct if I'm wrong here) the indicator width is calculated before the custom font is loaded. So suppose this is how you have your DOM configured, with respect to getting your fonts initialized:

<html>
  <head>
    <link href="https://fonts.googleapis.com/css?family=Roboto:300,400,500" rel="stylesheet">
    <style type="text/css">
      html, body {
        font-family: 'Roboto', sans-serif;
      }
    </style>
  </head>
  <body>
    ...
  </body>
</html>

The browser will first render with sans-serif, and once Roboto is downloaded (presuming here that it's not installed locally on the machine), Roboto loads and then the browser reflows.

But since you calculated the width of the indicator when sans-serif was used, you end up with the incorrect indicator width. The browser reflowed, and the width of the tab changed, but the width of the indicator didn't.

I think it ultimately comes down to being a limitation of the browser. The browser asynchronously reflowing when a font loads is something you cannot control. But you can control when and how you calculate the indicator width.

I think you'd have to be watching for when the font is loaded, and then recalculate the indicator styles. That would probably mean having an interface for consumers to tell you what custom font they use in their Tabs as well. Fontfaceobserver appears to be a solid cross-browser solution, but on a slow network connection, you be waiting up to 3 seconds (fontfaceobserver's default timeout) before calculating the indicator width.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 23, 2017

the indicator width is calculated before the custom font is loaded.

Yes, that's what is happening.

The browser asynchronously reflowing when a font loads is something you cannot control.

I don't agree, following my previous link regarding how to handle font loading: #7187 (comment)
There is a variety of solutions, basicaly, you either 1. make the font loading before rendering the React tree or 2. you make is so you minimize the font "jump" by tweaking the default font size dimensions.

  1. One can already be implemented on userland
  2. Two can now be implemented with [core] Improve styling solution #7461 by updating the theme font once the Roboto font is loaded.

I need to implement one of these solutions at work to be sure but I believe we can close the issue with: fix the font loading, no need for extra code complexity on the tabs component (that have a cost, like extra payload)

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 9, 2018

We went with changing the implementing to work server side. For the first server side render, we are using a width: 100% under the active tab. When the client side reconcile, we switch back to measuring the tab position.

As raised by #13165, we could push the approach one step further. Instead of using the tab position for positioning the indicator as soon as possible on the client side, we could wait for an index change event, providing more time for the fonts to load.

The current workaround is to trigger an indicator position update with the action property.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: tabs This is the name of the generic UI component, not the React module!
Projects
None yet
Development

No branches or pull requests

3 participants