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 don't reappear when default font-size is smaller than 16px #1114

Closed
JayFoxRox opened this issue May 26, 2019 · 7 comments

Comments

@JayFoxRox
Copy link

commented May 26, 2019

Description

Top menu (navigation tabs) never reappears after scrolling down.

Expected behavior

  1. When scrolling down, the navigation tabs should disappear.
  2. When scrolling back up, the navigation tabs should reappear.

Actual behavior

The tabs remain hidden and non-clickable (expected behavior 2 does not happen).

See discussion in downstream for screenshots: xqemu/xqemu.com#40 (comment)

Steps to reproduce the bug

Steps to reproduce in downstream: xqemu/xqemu.com#40 (comment)

(seems to be related to font-sizes).

Problem does happen at (no top banners / hero teaser):

Problem does not happen at (top banners / hero teaser):

System information / Package versions

@squidfunk

This comment has been minimized.

Copy link
Owner

commented May 26, 2019

I cannot reproduce this problem on Firefox 67, macOS. I tried setting the font size to 14px as described in the linked issue and zoomed in and out - no luck. Could be specific to Firefox on Linux, which I don't have at my disposal. Are there errors in the console?

Also - did you scroll entirely to the top? There's a 5px margin where the tabs are visible. Afterwards they disappear. Did you make changes to the CSS or added new JavaScript?

@JayFoxRox

This comment has been minimized.

Copy link
Author

commented May 28, 2019

I cannot reproduce this problem on Firefox 67, macOS. I tried setting the font size to 14px as described in the linked issue and zoomed in and out - no luck.

I'll try to reproduce this on Windows somehow. I don't have a machine for macOS.

Could be specific to Firefox on Linux, which I don't have at my disposal.

I believe this is related to fonts / font-sizes, where minor changes probably affect the item placement or coordinates. So it will work with some fonts / font-sizes at some zoom levels, but not on others.

I believe that because fonts can have slightly different shapes / sizes on different platforms.

Are there errors in the console?

No.

Also - did you scroll entirely to the top?

Yes.

See the linked screenshots in downstream: when loading the page, the tabs are still there, scrolling down and back up hides them. Anything else is at exactly the same position as before - it's fully scrolled up.

I also checked window.pageYOffset and it's returning expected values. It's 0 when scrolled up.
Tabs remain as .md-tabs[data-md-state="hidden"]

Did you make changes to the CSS or added new JavaScript?

No.

I'm using a newly created Firefox profile to reproduce this issue. I'm only changing the font.size.variable.x-western from 16 to 14 to trigger this problem.

@squidfunk

This comment has been minimized.

Copy link
Owner

commented May 28, 2019

Well, this sounds like a strange edge case with your setup/environment. The tabs feature was implemented more than 2 years ago, so I would have expected that if this strange behavior was more common, somebody would've opened an issue by now.

The next step you can do is try to debug the JavaScript. In order to do this, follow the steps to set up a build process. The line in question is here:

update() {
const active = window.pageYOffset >=
this.el_.children[0].offsetTop + (5 - 48) // TODO: quick hack to enable same handling for hero
if (active !== this.active_)
this.el_.dataset.mdState = (this.active_ = active) ? "hidden" : ""
}

this.el_.children[0] will select the first child of the element targetted by [data-md-component=tabs] and check whether it overlaps more than 5px with the header bar (48px). As denoted by the comment, this is a "hack", so the same logic can be (and actually is) used for the hero, if present.

As an easy quick fix you could override the hidden state in CSS (using a higher specificity or !important), enforcing visibility, always showing the tabs.

I always wanted to refactor the whole JavaScript code to a more reactive architecture using RxJS and TypeScript, but my time is too limited right now and the JavaScript code has proven to be surprisingly stable.

@JayFoxRox

This comment has been minimized.

Copy link
Author

commented May 29, 2019

Well, this sounds like a strange edge case with your setup/environment.

A friend reproduced this on Windows Firefox with font size 11.

The tabs feature was implemented more than 2 years ago, so I would have expected that if this strange behavior was more common, somebody would've opened an issue by now.

I believe we've had a complaint for this before, where a user was unable to find certain aspects of the website.

The problem is also that most people probably assume bugs in their own code (as did I), or simply don't create upstream bug reports - especially if the bug affects a small number of users, who might not be willing to report bugs for a small website they use (which they do not own).


I've looked into this now using

  • const c = this.el_.children[0]
  • const rect = Array.from(c.getClientRects())[0]

The c.offsetTop and rect.height are 48 when font-size is 16. On Linux, at font-size 14, the c.offsetTop and rect.height are 42 (occasionally I saw 43 - I assume there's some half-pixels).

The current code in master does: this.el_.children[0].offsetTop + (5 - 48) which, at font size 14, comes out as 42 + (5 - 48) = -1.
With the old code, it's therefore impossible to have a window.pageYOffset which is lower than -1.

Said friend who reproduced it, also prototyped a fix (that did not work); I've turned it into a working hack:

  /**
   * Update visibility
   */
  update() {
    const c = this.el_.children[0]
    const rect = Array.from(c.getClientRects())[0]
    const active = window.pageYOffset >=
      c.offsetTop + (5 - rect.height)                                           // TODO: quick hack to enable same handling for hero

    if (active !== this.active_)
      this.el_.dataset.mdState = (this.active_ = active) ? "hidden" : ""
  }

The code in this code will change the calculation to 42 + 5 - 42, so it uses the expected 5 pixels.

However, I believe this code is still a dirty hack, because it assumes that md-header (which is probably the source for the hardcoded 48) has the same height as md-tabs (which is the source for rect.height).

I did also consider c.offsetTop - rect.height * 0.9 to consider zoom / font-settings when scrolling (4.8 pixels on default settings) but I also didn't want to over-complicate this hacky code - it would introduce new possible error-sources.


I'm not sure how to PR this code: my git diff includes a bunch of files.

Also I've never really written any serious javascript and would rather have someone else integrate a solution upstream.

@squidfunk

This comment has been minimized.

Copy link
Owner

commented May 29, 2019

Thanks for your thorough research! As you discovered, the hardcoded value of 48pxis the problem, as it doesn't scale with the font size which the header does. I will look into your suggestions to fix it, I also have some ideas.

One drawback of your approach is that you call getClientRects every time the update function is invoked - that will cause massive repaints. We should just read offsetHeight on the header in the constructor and use this as a value.

@squidfunk squidfunk added bug and removed not reproducible labels May 29, 2019

@squidfunk squidfunk changed the title Top menu (navigation tabs) never reappears after scrolling down Tabs don't reappear when default font-size is smaller than 16px May 29, 2019

@squidfunk

This comment has been minimized.

Copy link
Owner

commented Jun 15, 2019

Fixed in 9d1390f

@squidfunk

This comment has been minimized.

Copy link
Owner

commented Jun 15, 2019

Released as part of 4.4.0.

@squidfunk squidfunk closed this Jun 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.