fix: use lang attribute in the html tag #16410

czp13 · 2023-03-24T16:10:50Z

Description

(introducing language to the HTML)

<html theme=""> ...</html>

to:

<html lang="en" theme="">
 ...</html>

The actual rule/way of working is quite similar to how the Component getLocale() is working:

trying to get the locale from the UI
if there is an I18N provider then the first locale will be used
last option is to fallback to the Locale.getDefault()

Now it is added to the IndexHtmlRequestHandler class

Pros:

it will be calculated for all requests
so a dynamic new I18N provider will be
Cos:

less performant

It could be added to other places to only do this once. The benefit would be to be more performant, but also it would not be dynamic.

I have tested it locally and it is adding the attribute for all requests:

Fixes # (issue)
#13437

Type of change

Bugfix
Feature

Checklist

I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
I have added a description following the guideline.
The issue is created in the corresponding repository and I have referenced it.
I have added tests to ensure my change is effective and works as intended.
New and existing tests are passing locally with my change.
I have performed a self-review and correct misspellings.

Additional for `Feature` type of change

Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

(introducing language to the html) <html theme=""> -> <html lan="en" theme=""> The actual rule/way of working is quite similar how the Component getLocale() is working: 1. trying to get the locale from UI 2. if there is I18N provider then the first locale will be used 3. last option is to fallback to the Locale.getDefault()

…-passed-setup-locale-values-13437

…xHtmlRequestHandler)

knoobie · 2023-03-24T16:30:17Z

flow-server/src/main/java/com/vaadin/flow/server/communication/IndexHtmlRequestHandler.java

+        // similar to Component implementation:
+        // worth to extract the getLocale to LocaleUtil potentially?
+        Locale locale = getLocale();
+        indexDocument.getElementsByTag("html").get(0).attr("lang",


Should this only be done if NO lang attribute was found? It's recommended to customize the index.html and a lot of people already did so. For example it's required for a11y to define the main language of the page and therefore most application already had to deal with this in someway and either set the value static in the index.html or later in some kind of rendering hook. So I would suggest to only change / add the value from the UI if there isn't already a lang attribute present to reduce regressions and unexpected side effects for people that have e.g. used a hard coded Spanish value but the server is running e.g. on English and guessing the wrong language of the page.

IndexHtmlRequestListeners are called after this locale logic, so that will provide a possibility to modify the locale if needed. Also any existing modifications from there should work as before.

For the index.html template, maybe we should remove the hardcoded lang="en" from there and then add check to the above logic to only add locale attribute if it does not exist in the index.html in order to cover cases where there is a customized index.html with a pre-set lang attribute already in place?

(got interrupted by some other issues, problems)
Thank you both of you, it makes sense, we just need to find a solution that helps for the requesters and old customers have custom solutions in place.

I did remove the relevant burned English language attribute from index.html-s (in tests as well for consistency)

Also add the check, to make sure we do not overwrite some intentional custom solution.
Got back to the original issue, thinking through it should work for both customers asking it there, in their cases, and do not need to use that workaround anymore and they can just create an I18NProvider to overwrite the language.

I will do some small refactoring, but basically, that's it here 🙇

Refactoring idea to reduce duplicated html-tag search and getLocale-call if not needed:

var htmlElement = indexDocument.getElementsByTag("html"); // only apply local based lang if index.html does not already contain it if (!htmlElement.hasAttr("lang")) { htmlElement.attr("lang", getLocale().getLanguage()); }

Changed a tiny bit I liked the idea so added, thanks for the optimization idea plus added as well refactor for extracting and using the same logic in multiple places (component and here).

Wanna move the Locale search also in the if to reduce the 'expensive' call on the hot path? Or it is needed somewhere else?

Yes, we can do it, good catch, if we spare just some calls/executions why not? 👍

github-actions · 2023-03-24T16:43:35Z

Test Results

  976 files ±  0   976 suites ±0 1h 11m 57s ⏱️ - 7m 2s
6 186 tests +  1 6 148 ✔️ +  1 38 💤 ±0 0 ❌ ±0
6 435 runs +13 6 392 ✔️ +15 43 💤 - 2 0 ❌ ±0

Results for commit 659d0cf. ± Comparison against base commit aeb856e.

♻️ This comment has been updated with latest results.

…-passed-setup-locale-values-13437

… as from discussions seems no point to add the language as burned in (rather it will be calculated from Locale/I18NProvider and so)

- Merge remote-tracking branch 'origin/main' into fix/lan-html-tag-should-be-created-considred-passed-setup-locale-values-13437

…-passed-setup-locale-values-13437

…gUtils anymore)

- based on knoobie's suggested optimization to only once fetch html tag element, - and using the same logic/classes for Components and IndexHtmlRequestHandler for the getting the locale,

…-passed-setup-locale-values-13437

czp13 · 2023-03-31T10:24:52Z

Tested once more locally/manually with all the changes, it is adding the proper tag.

sonarcloud · 2023-03-31T10:24:59Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

vaadin-bot · 2023-03-31T12:15:49Z

Hi @czp13 and @czp13, when i performed cherry-pick to this commit to 24.0, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 1091722
error: could not apply 1091722... fix/feature/optimization: use lang attribute in the html tag (#16410)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

vaadin-bot · 2023-03-31T12:15:53Z

Hi @czp13 and @czp13, when i performed cherry-pick to this commit to 23.3, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 1091722
error: could not apply 1091722... fix/feature/optimization: use lang attribute in the html tag (#16410)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

vaadin-bot · 2023-03-31T12:15:59Z

Hi @czp13 and @czp13, when i performed cherry-pick to this commit to 23.2, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 1091722
error: could not apply 1091722... fix/feature/optimization: use lang attribute in the html tag (#16410)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

@knoobie

Fix and feature add language to HTML tags dynamically: 0. It is checking if it is already added (if not, then it will try to add it based on the following rules: 1. trying to get the locale from the UI 2. if there is an I18N provider then the first locale will be used 3. last option is to fallback to the Locale.getDefault() This is the same logic as is used in the Component class as well, to have consistency within the application. Covering tests and optimization, refactoring happened and added. Thanks to @knoobie and @tepi for reviews, ideas, and optimizations. Fixes # (issue):

@knoobie

Fix and feature add language to HTML tags dynamically: 0. It is checking if it is already added (if not, then it will try to add it based on the following rules: 1. trying to get the locale from the UI 2. if there is an I18N provider then the first locale will be used 3. last option is to fallback to the Locale.getDefault() This is the same logic as is used in the Component class as well, to have consistency within the application. Covering tests and optimization, refactoring happened and added. Thanks to @knoobie and @tepi for reviews, ideas, and optimizations. Fixes # (issue):

@knoobie

… (CP:24.0) (may not need) (#16502) * fix/feature/optimization: use lang attribute in the html tag (#16410) Fix and feature add language to HTML tags dynamically: 0. It is checking if it is already added (if not, then it will try to add it based on the following rules: 1. trying to get the locale from the UI 2. if there is an I18N provider then the first locale will be used 3. last option is to fallback to the Locale.getDefault() This is the same logic as is used in the Component class as well, to have consistency within the application. Covering tests and optimization, refactoring happened and added. Thanks to @knoobie and @tepi for reviews, ideas, and optimizations. Fixes # (issue): * Fix build error (comment format :/, hopefully)

@knoobie

… (CP:23.3) (#16500) * fix/feature/optimization: use lang attribute in the html tag (#16410) Fix and feature add language to HTML tags dynamically: 0. It is checking if it is already added (if not, then it will try to add it based on the following rules: 1. trying to get the locale from the UI 2. if there is an I18N provider then the first locale will be used 3. last option is to fallback to the Locale.getDefault() This is the same logic as is used in the Component class as well, to have consistency within the application. Covering tests and optimization, refactoring happened and added. Thanks to @knoobie and @tepi for reviews, ideas, and optimizations. Fixes # (issue): * Fix my merge/conflict work/commit discrepencies. (Did not accept all the correc changes first, but with this one it shall be good) * mvn formatter:format executed. * Fix build error (comment format:/ hopefully!)

czp13 and others added 4 commits March 24, 2023 17:03

Merge branch 'main' into fix/lan-html-tag-should-be-created-considred…

783fcd6

…-passed-setup-locale-values-13437

mvn formatter:format executed.

e313309

Cleaning up not needed parts (not adding to VaadinService but to Inde…

7add39d

…xHtmlRequestHandler)

vaadin-bot added the +0.0.1 label Mar 24, 2023

Fixed lan -> lang (typo) (thanks @mcollovati)

98dac2d

czp13 changed the title ~~fix: use lan attribute in the html tag~~ fix: use lang attribute in the html tag Mar 24, 2023

knoobie reviewed Mar 24, 2023

View reviewed changes

czp13 and others added 13 commits March 25, 2023 10:16

Merge branch 'main' into fix/lan-html-tag-should-be-created-considred…

1199338

…-passed-setup-locale-values-13437

Merge branch 'main' into fix/lan-html-tag-should-be-created-considred…

170fdc6

…-passed-setup-locale-values-13437

Removed from most of the files (source and tests) the lang attribute,…

8499165

… as from discussions seems no point to add the language as burned in (rather it will be calculated from Locale/I18NProvider and so)

Fixed Conflicts and merge:

c1324f3

- Merge remote-tracking branch 'origin/main' into fix/lan-html-tag-should-be-created-considred-passed-setup-locale-values-13437

Merge branch 'main' into fix/lan-html-tag-should-be-created-considred…

b9f381f

…-passed-setup-locale-values-13437

mvn formatter:format executed.

61a8bd8

Added the check suggested by @knoobie and @tepi.

357ca98

Import/dependency mistake, should be good now (not using apache Strin…

76c837c

…gUtils anymore)

Added covering test.

c556e66

Refactoring/Optimization

e86b90a

- based on knoobie's suggested optimization to only once fetch html tag element, - and using the same logic/classes for Components and IndexHtmlRequestHandler for the getting the locale,

One more optimization (ty @knoobie).

2d53d9d

Organize imports

b278e03

Merge branch 'main' into fix/lan-html-tag-should-be-created-considred…

659d0cf

…-passed-setup-locale-values-13437

vaadin-bot added +0.1.0 and removed +0.0.1 labels Mar 31, 2023

tepi approved these changes Mar 31, 2023

View reviewed changes

czp13 merged commit 1091722 into main Mar 31, 2023

czp13 deleted the fix/lan-html-tag-should-be-created-considred-passed-setup-locale-values-13437 branch March 31, 2023 10:42

czp13 mentioned this pull request Mar 31, 2023

lang attribute of <html> tag ignores value of default locale and defaults to "en" #13437

Closed

czp13 added target/23.2 target/23.3 labels Mar 31, 2023

vaadin-bot added the need to pick manually 24.0 label Mar 31, 2023

vaadin-bot added need to pick manually 23.3 need to pick manually 23.2 labels Mar 31, 2023

czp13 mentioned this pull request Apr 2, 2023

fix/feature/optimization: use lang attribute in the html tag (#16410) (CP:23.3) #16500

Merged

9 tasks

This was referenced Apr 2, 2023

fix/feature/optimization: use lang attribute in the html tag (#16410) (CP:24) (wrong target) #16501

Closed

fix/feature/optimization: use lang attribute in the html tag (#16410) (CP:24.0) (may not need) #16502

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use lang attribute in the html tag #16410

fix: use lang attribute in the html tag #16410

czp13 commented Mar 24, 2023 •

edited

Loading

knoobie Mar 24, 2023

tepi Mar 29, 2023 •

edited

Loading

czp13 Mar 30, 2023

knoobie Mar 30, 2023

czp13 Mar 30, 2023 •

edited

Loading

knoobie Mar 30, 2023

czp13 Mar 31, 2023

github-actions bot commented Mar 24, 2023 •

edited

Loading

czp13 commented Mar 31, 2023

sonarcloud bot commented Mar 31, 2023

vaadin-bot commented Mar 31, 2023

vaadin-bot commented Mar 31, 2023

vaadin-bot commented Mar 31, 2023

fix: use lang attribute in the html tag #16410

fix: use lang attribute in the html tag #16410

Conversation

czp13 commented Mar 24, 2023 • edited Loading

Description

Type of change

Checklist

Additional for Feature type of change

knoobie Mar 24, 2023

Choose a reason for hiding this comment

tepi Mar 29, 2023 • edited Loading

Choose a reason for hiding this comment

czp13 Mar 30, 2023

Choose a reason for hiding this comment

knoobie Mar 30, 2023

Choose a reason for hiding this comment

czp13 Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

knoobie Mar 30, 2023

Choose a reason for hiding this comment

czp13 Mar 31, 2023

Choose a reason for hiding this comment

github-actions bot commented Mar 24, 2023 • edited Loading

Test Results

czp13 commented Mar 31, 2023

sonarcloud bot commented Mar 31, 2023

vaadin-bot commented Mar 31, 2023

vaadin-bot commented Mar 31, 2023

vaadin-bot commented Mar 31, 2023

czp13 commented Mar 24, 2023 •

edited

Loading

Additional for `Feature` type of change

tepi Mar 29, 2023 •

edited

Loading

czp13 Mar 30, 2023 •

edited

Loading

github-actions bot commented Mar 24, 2023 •

edited

Loading