satnaing / astro-paper

A minimal, accessible and SEO-friendly Astro blog theme
https://astro-paper.pages.dev/
MIT License
2.52k stars 522 forks source link

Navigation bar color flickers on page change #138

Open HenestrosaDev opened 1 year ago

HenestrosaDev commented 1 year ago

When changing pages on a mobile device, the color of the navigation bar changes from white to the background color of the body, which is annoying and detrimental to the UX. I've tried to fix this with no luck yet, but if I manage to solve it, I'll make a pull request.

The problem might be in this code of the toggle-theme.js file, because the value of the theme-color is only changed if the body exists. That is, when the page is loaded.

function reflectPreference() {
  document.firstElementChild.setAttribute("data-theme", themeValue);

  document.querySelector("#theme-btn")?.setAttribute("aria-label", themeValue);

  // Get a reference to the body element
  const body = document.body;

  // Check if the body element exists before using getComputedStyle
  if (body) {
    // Get the computed styles for the body element
    const computedStyles = window.getComputedStyle(body);

    // Get the background color property
    const bgColor = computedStyles.backgroundColor;

    // Set the background color in <meta theme-color ... />
    document
      .querySelector("meta[name='theme-color']")
      ?.setAttribute("content", bgColor);
  }
}
HenestrosaDev commented 1 year ago

Not the most elegant solution, but I ended up doing the following:

  1. Instead of leaving the content attribute empty in the theme-color meta tag, I set the predefined value to the light theme color base of my site: <meta name="theme-color" content="rgb(246, 238, 225)" />

  2. I added this code to toggle-theme.js. Note that I've only included the changed methods, but you can check the whole file of my website if there is any doubt:

    
    function setPreference(isThemeBtnClicked) {
    localStorage.setItem("theme", themeValue);
    
    reflectPreference();
    changeThemeColorContent(isThemeBtnClicked);
    toggleGiscusTheme();
    }

function changeThemeColorContent(isThemeBtnClicked) { const themeColorElement = document.querySelector("meta[name='theme-color']")

if (themeValue === "dark") { themeColorElement?.setAttribute("content", "rgb(16, 23, 42)"); } else if (isThemeBtnClicked && themeValue === "light") { themeColorElement?.setAttribute("content", "rgb(246, 238, 225)"); } }

window.onload = () => { function setThemeFeature() { // set on load so screen readers can get the latest value on the button reflectPreference(); changeThemeColorContent();

// now this script can find and listen for clicks on the control
document.querySelector("#theme-btn")?.addEventListener("click", () => {
  themeValue = themeValue === "light" ? "dark" : "light";
  setPreference(isThemeBtnClicked = true);
});

}

setThemeFeature();

// Runs on view transitions navigation document.addEventListener("astro:after-swap", setThemeFeature); };


It's true that the navigation bar still flickers when loading a new page in dark mode on mobile, but at least now it won't flicker when the light theme is set. Another solution would be to use `prefers-color-scheme`, but since the theme can be changed independently of the user's preferred color scheme, it's hard to get it to work properly:


I won't make a pull request with these changes, as I don't think they are the most optimal solution. I wish it would be valid  to set the value of the content attribute to `currentcolor`, as it would change dynamically without requiring any JavaScript code.
satnaing commented 1 year ago

Unfortunately, I cannot reproduce this issue on my browsers. So, if possible, can you also provide me the browser you tested and the OS. Maybe getting the color of the document body in toggle-theme.js is somehow blocking and causing the problem. If it is so, we can update the reflectPreference function inside toggle-theme.js to something like this

function reflectPreference() {
  document.firstElementChild.setAttribute("data-theme", themeValue);

  document.querySelector("#theme-btn")?.setAttribute("aria-label", themeValue);

  const metaThemeColor =
    themeValue === "light" ? `rgb(251, 254, 251)` : `rgb(33, 39, 55)`; // here, you can get these colors from base.css in your project
  document
    .querySelector("meta[name='theme-color']")
    ?.setAttribute("content", metaThemeColor);
}

In the above function, you can hard-code rgb(251, 254, 251) and rgb(33, 39, 55) without any issue. You can get these values from base.css file in your project.

/* file: base.css */
  html[data-theme="light"] {
    --color-fill: 251, 254, 251; /* 👈🏻 get this value for light theme */
    --color-text-base: 40, 39, 40;
    --color-accent: 0, 108, 172;
    --color-card: 230, 230, 230;
    --color-card-muted: 205, 205, 205;
    --color-border: 236, 233, 233;
  }
  html[data-theme="dark"] {
    --color-fill: 33, 39, 55; /* 👈🏻 get this value for dark theme */
    --color-text-base: 234, 237, 243;
    --color-accent: 255, 107, 1;
    --color-card: 52, 63, 96;
    --color-card-muted: 138, 51, 2;
    --color-border: 171, 75, 8;
  }

In AstroPaper, I didn't hard-code those values intentionally. This is because AstroPaper, as a template for various users, has no ability to know which color schemes the user would configure and customize. So, it is better to get the theme-color value from the body using JavaScript. It'd be best if theme-color meta tag supports currentColor, but sadly it doesn't.

If theme-color meta tag bothers you, you can omit that meta from the code; and it would be completely fine without any problem. You can read more about theme-color meta tag here.

BTW, your updated code would also be fine I guess.

HenestrosaDev commented 1 year ago

You can reproduce the issue using Chrome with the light theme on Android.

EDIT: Here is a video showing the reported behavior:

https://github.com/satnaing/astro-paper/assets/60482743/3655cbbb-c4c4-4fe5-9ddf-343af498eea9

satnaing commented 1 year ago

It's a bit hard to reproduce since I don't find anything like that on iOS. I'll try and reproduce it using an Android phone. BTW, did my comment solve this issue? Or does the flickering still exist?

HenestrosaDev commented 1 year ago

The flickering is still present with the code you provided. Let me know if I can help with anything else.

saliksik commented 1 year ago

I can confirm the issue. I tested this on my Samsung A52s 5G phone, and the navigation bar color flickers when pages change.