feat(i18n): added support for auto-setting the lang attribute

arctic-hen7 · arctic-hen7 · commit 1e3206c761a9 · 2023-02-27T09:44:23.000+11:00
This is fully tested now. Closes #261.
diff --git a/examples/core/i18n/src/templates/about.rs b/examples/core/i18n/src/templates/about.rs
@@ -4,7 +4,7 @@ use sycamore::prelude::*;
 fn about_page<G: Html>(cx: Scope) -> View<G> {
     view! { cx,
         p { (t!(cx, "about")) }
-        button(on:click = move |_| {
+        button(id = "switch-button", on:click = move |_| {
             #[cfg(client)]
             Reactor::<G>::from_cx(cx).switch_locale("fr-FR");
         }) { "Switch to French" }
diff --git a/examples/core/i18n/tests/main.rs b/examples/core/i18n/tests/main.rs
@@ -10,6 +10,9 @@ async fn main(c: &mut Client) -> Result<(), fantoccini::error::CmdError> {
     // languages is very hard, we do unit testing on the locale detection system
     // instead
     assert!(url.as_ref().starts_with("http://localhost:8080/en-US"));
+    // Make sure the HTML `lang` attribute has been correctly set
+    let lang = c.find(Locator::Css("html")).await?.attr("lang").await?;
+    assert_eq!(lang, Some("en-US".to_string()));
     // This tests translations, variable interpolation, and multiple aspects of
     // Sycamore all at once
     let text = c.find(Locator::Css("p")).await?.text().await?;
@@ -18,10 +21,22 @@ async fn main(c: &mut Client) -> Result<(), fantoccini::error::CmdError> {
     c.find(Locator::Css("a")).await?.click().await?;
     // This tests i18n linking (locale should be auto-detected)
     let url = c.current_url().await?;
-    wait_for_checkpoint!("page_interactive", 0, c);
+    wait_for_checkpoint!("page_interactive", 1, c);
     assert!(url
         .as_ref()
         .starts_with("http://localhost:8080/en-US/about"));
 
+    // Switch the locale
+    c.find(Locator::Id("switch-button")).await?.click().await?;
+    let url = c.current_url().await?;
+    wait_for_checkpoint!("page_interactive", 2, c);
+    assert!(url
+        .as_ref()
+        .starts_with("http://localhost:8080/fr-FR/about"));
+
+    // Make sure the HTML `lang` attribute has been correctly set
+    let lang = c.find(Locator::Css("html")).await?.attr("lang").await?;
+    assert_eq!(lang, Some("fr-FR".to_string()));
+
     Ok(())
 }
diff --git a/packages/perseus/src/i18n/client_translations_manager.rs b/packages/perseus/src/i18n/client_translations_manager.rs
@@ -106,6 +106,9 @@ impl ClientTranslationsManager {
                 }
             };
             self.cache_translator(translator);
+            // Only set `lang` if everything else worked (otherwise we'd end up with state
+            // inconsistencies)
+            Self::set_html_lang(locale);
         }
 
         Ok(())
@@ -115,6 +118,9 @@ impl ClientTranslationsManager {
     /// the translations from the server. This manages mutability for caching
     /// internally.
     ///
+    /// This will imperatively update the `lang` attribute on the root `<html>`
+    /// element.
+    ///
     /// # Panics
     ///
     /// This will panic if the given locale is not supported.
@@ -132,7 +138,10 @@ impl ClientTranslationsManager {
             match translations_str {
                 Some(translations_str) => {
                     // All good, turn the translations into a translator
-                    self.set_translator_for_translations_str(locale, &translations_str)?
+                    self.set_translator_for_translations_str(locale, &translations_str)?;
+                    // Only set `lang` if everything else worked (otherwise we'd end up with state
+                    // inconsistencies)
+                    Self::set_html_lang(locale);
                 }
                 // If we get a 404 for a supported locale, that's an exception
                 None => {
@@ -146,4 +155,17 @@ impl ClientTranslationsManager {
 
         Ok(())
     }
+
+    /// Sets the `lang` attribute on the root `<html>` tag to stay up-to-date
+    /// with the internal state of this system.
+    fn set_html_lang(locale: &str) {
+        let document = web_sys::window().unwrap().document().unwrap();
+        // If the `<html>` tag does not exist, and Perseus is running, this would
+        // be...interesting...
+        let html = document.document_element().unwrap();
+
+        // This is a non-critical operation, so errors are not propagated here
+        // TODO In future, this should integrate with the `PlatformError` system
+        let _ = html.set_attribute("lang", locale);
+    }
 }
diff --git a/packages/perseus/src/reactor/initial_load.rs b/packages/perseus/src/reactor/initial_load.rs
@@ -62,7 +62,7 @@ impl<G: Html> Reactor<G> {
         let slim_verdict = verdict.clone();
         match &verdict.into_full(&self.entities) {
             // WARNING: This will be triggered on *all* incremental paths, even if
-            // the serber returns a 404!
+            // the server returns a 404!
             FullRouteVerdict::Found(FullRouteInfo {
                 path,
                 entity,
diff --git a/packages/perseus/src/server/html_shell.rs b/packages/perseus/src/server/html_shell.rs
@@ -42,6 +42,18 @@ pub(crate) struct HtmlShell {
     /// Code to be inserted into the shell after the Perseus contents of the
     /// page. This is designed to be modified by plugins.
     pub after_content: Vec<String>,
+    /// The locale to be used for this page, which will be set in the `lang`
+    /// attribute on `<html>`.
+    ///
+    /// **IMPORTANT:** This *does* use `xx-XX` for denoting the locale of an
+    /// app not using i18n, but, if this value is set, no interpolation will
+    /// be performed. For apps that do use i18n, manually setting the `lang`
+    /// attribute in teh index view may lead to unpredictable errors (as
+    /// interpolation will nonetheless be performed, leading to conflicts).
+    ///
+    /// Note that `xx-XX` will be automatically set for locale redirection
+    /// pages, preventing interpolation.
+    pub locale: String,
     /// The ID of the element into which we'll interpolate content.
     root_id: String,
     /// The path prefix to use.
@@ -178,6 +190,8 @@ impl HtmlShell {
             content: "".into(),
             root_id: root_id.into(),
             path_prefix: path_prefix.into(),
+            // Assume until we know otherwise
+            locale: "xx-XX".to_string(),
         }
     }
 
@@ -187,12 +201,17 @@ impl HtmlShell {
     /// translator can be derived on the client-side. These are provided in
     /// a window variable to avoid page interactivity requiring a network
     /// request to get them.
+    ///
+    /// This needs to know what the locale is so it can set the HTML `lang`
+    /// attribute for improved SEO.
     pub(crate) fn page_data(
         mut self,
         page_data: &PageData,
         global_state: &TemplateState,
+        locale: &str,
         translations: &str,
     ) -> Self {
+        self.locale = locale.to_string();
         // Interpolate a global variable of the state so the app shell doesn't have to
         // make any more trips The app shell will unset this after usage so it
         // doesn't contaminate later non-initial loads Error pages (above) will
@@ -249,6 +268,7 @@ impl HtmlShell {
     /// Further, this will preload the Wasm binary, making redirection snappier
     /// (but initial load slower), a tradeoff that generally improves UX.
     pub(crate) fn locale_redirection_fallback(mut self, redirect_url: &str) -> Self {
+        self.locale = "xx-XX".to_string();
         // This will be used if JavaScript is completely disabled (it's then the site's
         // responsibility to show a further message)
         let dumb_redirect = format!(
@@ -316,8 +336,13 @@ impl HtmlShell {
         error_page_data: &ServerErrorData,
         error_html: &str,
         error_head: &str,
+        locale: Option<String>, // For internal convenience
         translations_str: Option<&str>,
     ) -> Self {
+        if let Some(locale) = locale {
+            self.locale = locale.to_string();
+        }
+
         let error = serde_json::to_string(error_page_data).unwrap();
         let state_var = format!(
             "window.__PERSEUS_INITIAL_STATE = `error-{}`;",
@@ -394,12 +419,19 @@ impl fmt::Display for HtmlShell {
             .replace(&html_to_replace_double, &html_replacement)
             .replace(&html_to_replace_single, &html_replacement);
 
+        // Finally, set the `lang` tag if we should
+        let final_shell = if self.locale != "xx-XX" {
+            new_shell.replace("<html", &format!(r#"<html lang="{}""#, self.locale))
+        } else {
+            new_shell
+        };
+
         // And minify everything
         // Because this is run on live requests, we have to be fault-tolerant (if we
         // can't minify, we'll fall back to unminified)
-        let minified = match minify(&new_shell, true) {
+        let minified = match minify(&final_shell, true) {
             Ok(minified) => minified,
-            Err(_) => new_shell,
+            Err(_) => final_shell,
         };
 
         f.write_str(&minified)
diff --git a/packages/perseus/src/turbine/build_error_page.rs b/packages/perseus/src/turbine/build_error_page.rs
@@ -30,10 +30,10 @@ impl<M: MutableStore, T: TranslationsManager> Turbine<M, T> {
         // Translator and translations string
         i18n_data: Option<(&Translator, &str)>,
     ) -> String {
-        let (translator, translations_str) = if let Some((t, s)) = i18n_data {
-            (Some(t), Some(s))
+        let (translator, translations_str, locale) = if let Some((t, s)) = i18n_data {
+            (Some(t), Some(s), Some(t.get_locale()))
         } else {
-            (None, None)
+            (None, None, None)
         };
 
         let (head, body) = self.error_views.render_to_string(data.clone(), translator);
@@ -43,7 +43,7 @@ impl<M: MutableStore, T: TranslationsManager> Turbine<M, T> {
             .unwrap()
             .clone()
             // This will inject the translations string if it's available
-            .error_page(&data, &body, &head, translations_str)
+            .error_page(&data, &body, &head, locale, translations_str)
             .to_string()
     }
 }
diff --git a/packages/perseus/src/turbine/export.rs b/packages/perseus/src/turbine/export.rs
@@ -180,7 +180,7 @@ impl<M: MutableStore, T: TranslationsManager> Turbine<M, T> {
                     // not using i18n
                     let full_html = html_shell
                         .clone()
-                        .page_data(&page_data, global_state, &translations)
+                        .page_data(&page_data, global_state, locale, &translations)
                         .to_string();
                     self.immutable_store
                         .write(
@@ -224,7 +224,7 @@ impl<M: MutableStore, T: TranslationsManager> Turbine<M, T> {
                 // not using i18n
                 let full_html = html_shell
                     .clone()
-                    .page_data(&page_data, global_state, "")
+                    .page_data(&page_data, global_state, "xx-XX", "")
                     .to_string();
                 // We don't add an extension because this will be queried directly by the
                 // browser
diff --git a/packages/perseus/src/turbine/server.rs b/packages/perseus/src/turbine/server.rs
@@ -223,7 +223,7 @@ impl<M: MutableStore, T: TranslationsManager> Turbine<M, T> {
                 // We can use those to get a translator efficiently
                 let translator = match self
                     .translations_manager
-                    .get_translator_for_translations_str(locale, translations_str.clone())
+                    .get_translator_for_translations_str(locale.clone(), translations_str.clone())
                     .await
                 {
                     Ok(translator) => translator,
@@ -257,7 +257,7 @@ impl<M: MutableStore, T: TranslationsManager> Turbine<M, T> {
                     .as_ref()
                     .unwrap()
                     .clone()
-                    .page_data(&page_data, &global_state, &translations_str)
+                    .page_data(&page_data, &global_state, &locale, &translations_str)
                     .to_string();
                 // NOTE: Yes, the user can fully override the content type...I have yet to find
                 // a good use for this given the need to generate a `View`
