@media rule thresholds do not account for browser font size setting

KosRud commented 1 year ago

Have you read the Contributing Guidelines on issues?

[X] I have read the Contributing Guidelines on issues.

Prerequisites

[X] I'm using the latest version of Docusaurus.
[X] I have tried the npm run clear or yarn clear command.
[X] I have tried rm -rf node_modules yarn.lock package-lock.json and re-installing packages.
[X] I have tried creating a repro with https://new.docusaurus.io.
[X] I have read the console error message carefully (if applicable).

Description

Most things in docusaurus scale with rem units, but the thresholds for @media rules are set in px. Which means setting a large font size in the browser effectively "zooms in" the page, but the @media rules do not trigger like they would with actual zoom.

Screenshots below are from the page https://docusaurus.io/docs/styling-layout with the same zoom level (100%) and viewport size (~1000px). The large font size breaks the layout.

normal font size	large font size

Reproducible demo

https://docusaurus.io/docs/styling-layout

Steps to reproduce

Go to https://docusaurus.io/docs/styling-layout
Set viewport width to ~1000px, just before the @media rule triggers
Set large font size in the browser
Observe the broken layout

Expected behavior

IMHO it would make sense to use rem unit for @media thresholds as it would represent the "actual" scale of the content (accounting for both zoom and browser font size).

Actual behavior

TOC on the right was unreasonably squished and navbar at the top has overlapping text. Normally with the content being enlarged this much a @media rule would trigger, but it does not happen because the threshold is tied to px and is not aware of browser font size setting.

Your environment

Link: https://docusaurus.io/docs/styling-layout Browser: Brave 1.58.137 OS: Windows 11

Also tried locally with npx create-docusaurus@latest my-website classic (pulled version 2.4.3), same result.

Self-service

[ ] I'd be willing to fix this bug myself.

Piyush-onGIT commented 1 year ago

Please assign me this issue, I would like to work on this.

OzakIOne commented 1 year ago

Please assign me this issue, I would like to work on this.

Docusaurus maintainers don't assign issue to people, if you want to contribute, just send a PR

slorber commented 1 year ago

Isn't this a Brave-only issue?

Because despite using px I see Chrome, Firefox and Safari being able to switch to the mobile layout on zoom.

https://github.com/facebook/docusaurus/assets/749374/59d966ed-b38a-4389-a08f-4fac6cf806e3

KosRud commented 1 year ago

Isn't this a Brave-only issue?

Because despite using px I see Chrome, Firefox and Safari being able to switch to the mobile layout on zoom.

CleanShot.2023-10-12.at.11.14.40.mp4

Have you set large font size?

slorber commented 1 year ago

I don't use Brave. Can you show me where to find this option on other browsers and tell me if you see the bug in other browsers too?

KosRud commented 1 year ago

I don't use Brave. Can you show me where to find this option on other browsers and tell me if you see the bug in other browsers too?

I just installed Chrome and it does appear there too. Below is screenshot of the bug, as well as how to reach the setting.

KosRud commented 1 year ago

The bug occurs because the size of elements on the page is tied to rem units, which are affected by font size setting in (any) browser. By default 1rem = 16px so these units are in a sense equivalent. However, setting large font size in the browser will change this ratio (e.g. 1rem = 32px).

This effectively zooms in the page, because everything is tied to rem in docusaurus CSS (e.g. 1rem margin used to be 16px and now it is 32px). However, @media rules are tied to px units which are unaffected by font size and so they are "unaware" of the layout getting compressed.

Josh-Cena commented 1 year ago

I'm not sure what to do. Sites like GitHub and nextjs.org seem to have fixed font size so their sites don't rescale at all. Vuepress only scales the navbar but not the content, and its screen size breakpoint is also based on pixels. Do you have a site showing ideal behavior?

KosRud commented 1 year ago

I'm not sure what to do. Sites like GitHub and nextjs.org seem to have fixed font size so their sites don't rescale at all. Vuepress only scales the navbar but not the content, and its screen size breakpoint is also based on pixels. Do you have a site showing ideal behavior?

I will try to find an example (or make one myself). But by simply experimenting with developer tools in browser, replacing

@media (max-width: 996px) {

with

@media (max-width: 62.25rem) {

seems to do the trick. As long as the default setting 1rem = 16px remains in place, these rules are equivalent. If the font size (ratio rem to px) is increased, the rule triggers earlier (which is desired to prevent the layout from breaking).

The only issue is that I can't be sure this change won't unexpectedly create problems elsewhere (theoretically I don't think it should).

Unless I'm missing something, simply replacing all media thresholds defined in px with 1/16 of that value in rem should provide ideal behavior, and look exactly the same as before with default font setting.

KosRud commented 1 year ago

Speaking of which, searching for @media rules in the repo I found thresholds of 996px and 997px being used seemingly interchangeably. Not sure if this is intentional. Either way this is unlikely to cause serious issues but IMHO is confusing and could lead to weird situations in edge-case scenarios where the viewport is exactly 996px and only half the page is in compact mode.

https://github.com/facebook/docusaurus/blob/ae3191654cc8022da39c5c425bc3dca85bf0314f/packages/docusaurus-theme-classic/src/theme/TOC/styles.module.css#L15

https://github.com/facebook/docusaurus/blob/ae3191654cc8022da39c5c425bc3dca85bf0314f/packages/docusaurus-theme-classic/src/theme/DocRoot/Layout/Sidebar/styles.module.css#L17

https://github.com/facebook/docusaurus/blob/ae3191654cc8022da39c5c425bc3dca85bf0314f/packages/docusaurus-theme-classic/src/theme/BlogSidebar/Desktop/styles.module.css#L41

https://github.com/facebook/docusaurus/blob/ae3191654cc8022da39c5c425bc3dca85bf0314f/packages/docusaurus-theme-classic/src/theme/DocSidebar/Desktop/Content/styles.module.css#L8

Josh-Cena commented 1 year ago

@KosRud max-width: 996px is equivalent to min-width: 997px, because the breakpoint is inclusive :)

KosRud commented 1 year ago

@Josh-Cena

@KosRud max-width: 996px is equivalent to min-width: 997px, because the breakpoint is inclusive :)

My bad, you are right. I missed that one is max and another is min.

Regarding the potential fix, does replacing the px thresholds with 1/16 in rem sound reasonable?

Josh-Cena commented 1 year ago

It makes sense to me, but I'm not sure if it will cause problems elsewhere. For example, we also have JavaScript code that use the 996 boundary (soon to be configurable, but pixel-based nonetheless):

https://github.com/facebook/docusaurus/blob/ae3191654cc8022da39c5c425bc3dca85bf0314f/packages/docusaurus-theme-common/src/hooks/useWindowSize.ts#L20-L29

Therefore, I'd still be curious to know what the best practice is and examples of such.

KosRud commented 1 year ago

@Josh-Cena

I found a few examples of pages that use em or rem for @media rules (as well as for most sizes/margins, like docusaurus does). It works as expected, the @media rules being aware of extra scaling introduced by altered font size setting in the browser.

https://www.ada.gov/resources/web-guidance/