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

Bug: Page Refresh Needed To View Different Themes On Different Pages (Static Server Rendering - 5.1.0) #3868

Closed
thabaum opened this issue Feb 21, 2024 · 26 comments

Comments

@thabaum
Copy link
Contributor

thabaum commented Feb 21, 2024

Issue

Description

After changing the theme on a page, the user is not shown the new page theme or site theme, it will be one or the other like it is cached.

Steps to reproduce

  1. Install Two themes
  2. Set a page to use one theme.
  3. Switch between pages in the navigation menu
  4. notice the theme behavior described and shown in this issue.

Below are screenshots of the behavior.

I am using two themes, the Oqtane Default Theme and Oqtane Theme Creator Template Theme.

My Page has the theme creator template theme, while the rest should use the stock default Oqtane theme.

Screenshots

image

image

image

Hit refresh:
image
image
hit refresh:
image

The theme is not updated with current theme provided by the page.

@sbwalker
Copy link
Member

sbwalker commented Feb 21, 2024

This is a strange issue. When I navigate to the page which contains a theme which is different than the default site theme, the CSS styling is broken... but all of the stylesheet references in the head look correct:

image

There is an error in the browser console however:

image

And after refreshing the page, the styling is correct and there is no browser console error... but the stylesheet references look the same:

image

Perhaps it is browser specific? https://stackoverflow.com/questions/76195866/firefox-none-of-the-sha384-hashes-in-the-integrity-attribute-match-the-conte

EDIT: It is not browser specific - on Edge it has even stranger behavior ie. it seems to retain only the last Bootstrap CSS library loaded but not the one in the actual page head. And a similar browser console error:

image

Perhaps this could be a browser caching issue for stylesheets? Or maybe a Blazor "enhanced navigation" issue.

@thabaum
Copy link
Contributor Author

thabaum commented Feb 21, 2024

I also noticed how the loading of the menu actually changes to the site theme as the color of the nav item text is white so you cannot see it.

image
image

Could it be a load order priority (one needs removed)
issue?

I also see these errors and behavior mentioned in the browser developer tools console.

@sbwalker
Copy link
Member

I don’t think so. If you look in the browser dev tools you can see the head elements and the stylesheets are in the correct order

@thabaum
Copy link
Contributor Author

thabaum commented Feb 22, 2024

maybe it is a theme creator theme issue in combination with a caching issue coupled onto another issue because from a fresh download/install it seemed to load the theme creator theme in my browser until i hit f5 to refresh, then it seemed to switch between blazor and oqtane page/site themes I had set visually as expected, however the errors in console of browser shown previously still existed.

@thabaum
Copy link
Contributor Author

thabaum commented Feb 22, 2024

It is working for me right now switching between the Blazor and Oqtane Theme. I am going to refresh Oqtane files and build a theme.

The issue is probably both theme files are trying to be merged together, the Page theme if set should become the site theme on that page visit, and go back to the other site theme. This would shutdown the conflicts. A refresh clears the old theme. The page theme is being loaded with the site theme first, it should load either the site theme OR the page theme, not both.

Also caching maybe to blame, in browser or in Oqtane. My guess in Oqtane.

If we add ?refresh to the end of the navigation page links going to pages that have themes different then site it resolves the issue sort of... or in other words NavigateUrl(true)

@sbwalker
Copy link
Member

@thabaum we definitely cannot add navigateurl(true) on normal site navigations - that would be a massive performance issue as it would be doing double the work. I think the reason why it is working now is because you updated the Blazor theme to use the same version of Bootstrap - so there is no need to load a different version of Bootstrap (which is what was causing the integrity error in the browser console). Either way, I am not prioritizing this issue for 5.1.0.

@thabaum
Copy link
Contributor Author

thabaum commented Feb 22, 2024

Sorry, nevermind with what I just said... it would be a headache.
Blazor theme page navigating from Oqtane theme page (combined themes using dark background from Oqtane theme)
image

Hit refresh notice it looks as it should with a light background and then navigate the Oqtane Theme pages which now look like the theme creator theme with also a light background.

image

Maybe if theme is set on a page, it needs to reload itself, or some conditional check in a OnParametersSet override method potentially rather than what maybe set OnAfterRender for interactive mode as described in other issues as we have encountered this and is what is missing potentially? Maybe this logic isnt getting executed sitting in a method that isnt supported by static ssr to make the update to the stylesheets loading as needed? This works in interactive perfectly...

I was hoping to review this logic yesterday but nothing I did changed the outcome as of yet. 2 challenges I have is trying to be sure I locate where these css styles get applied both in page and for site. I have been in the App.razor and ThemeBuilder locations mainly reviewing any code I can find relating.

@sbwalker
Copy link
Member

@thabaum Interactive render mode makes Oqtane run as a SPA - which means that stylesheets are managed by JS Interop - a completely different approach. Static render mode makes Oqtane run as a standard web app - no JS Interop.

@thabaum
Copy link
Contributor Author

thabaum commented Feb 22, 2024

@sbwalker I am curious if the use of headers Cache-Control and/or Clear-Site-Data may help resolve the issue here?
image

Maybe we can add a header value that can be supported in Oqtane Framework knowing if it should do something extra to support clearing the browser cache on page changes only when using static ssr? Or visit how Cache-Control can be used and set it differently for static ssr if not the framework itself?

Verify the presence of correct caching headers
The correct caching headers should be present on responses to resource requests. This includes ETag, Cache-Control, and other caching headers. The configuration of these headers is dependent on the hosting service or hosting server platform. They are particularly important for assets such as the Blazor script (blazor.webassembly.js) and anything the script downloads.

Incorrect HTTP caching headers may also impact service workers. Service workers rely on caching headers to manage cached resources effectively. Therefore, incorrect or missing headers can disrupt the service worker's functionality.

Use Clear-Site-Data to delete state in the browser
Consider using the Clear-Site-Data header to delete state in the browser.

Usually the source of cache state problems is limited to the HTTP browser cache, so use of the cache directive should be sufficient. This action can help to ensure that the browser fetches the latest resources from the server, rather than serving stale content from the cache.

You can optionally include the storage directive to clear local storage caches at the same time that you're clearing the HTTP browser cache. However, apps that use client storage might experience a loss of important information if the storage directive is used.

Just a thought, might be easier to control if this works. I have not tried this out yet.

@sbwalker
Copy link
Member

I tried adding some logic to the router to do a full page reload if the page theme is not the same as the site theme:

    if (PageState != null && PageState.RenderMode == RenderModes.Static && !string.IsNullOrEmpty(PageState.Page.ThemeType) && !querystring.ContainsKey("reloaded"))
    {
        NavigationManager.NavigateTo(_absoluteUri + "?reloaded", true);
        return;
    }

However even a full page refresh does not resolve the issue with the stylesheets - the integrity error message is still present in the browser console.

So then I tried disabling the data-enhance-nav attribute on the links in the horizontal menu:

data-enhance-nav="false"

This resolved the problem. So the problem is caused by Blazor's "enhanced navigation" in .NET 8. This is a good article to explain what is happening under the covers:

https://www.telerik.com/blogs/blazor-enhanced-navigation-fully-explained

We do not want to disable enhanced navigation on all navigations so I will need to figure out a way to selectively apply this attribute in the menu for pages where the theme overrides the site theme.

sbwalker added a commit that referenced this issue Feb 23, 2024
fix #3868 - static rendering support for page themes within site
@sbwalker
Copy link
Member

@thabaum PR #3881 resolves the issue by using data-enhance-nav="false" - however it is important to understand that Blazor performs a full page refresh in this scenario, which means the page "flashes" because it is completely reloading the page content from the server (essentially doing the same thing as if you clicked the refresh button in your browser).

@thabaum
Copy link
Contributor Author

thabaum commented Feb 23, 2024

@sbwalker Very cool you found a simple way to resolve this issue. This flicker should only happen in Static Render Mode correct? I will review this shortly. This is expected since static server rendering should be a fresh server load each time.

It is good to note that if you are not allowing pages to use different themes then allowing data-enhance-nav="true" could be preferred. I will have to play with this to understand it more.

Thanks for finding a solution! 🚀

@sbwalker
Copy link
Member

sbwalker commented Feb 23, 2024

@thabaum yes, the data-enhance-nav is only applicable for static rendering. Note that in static rendering, "enhanced navigation" is enabled by default which means that the framework in not actually doing a full page refresh (which is why the stylesheets were not loading correctly for different page themes). You can read more about "enhanced navigation" in this article:

https://www.telerik.com/blogs/blazor-enhanced-navigation-fully-explained

When enhanced navigation is enabled, the Blazor app will perform the following actions:

  • Interception: When a request is made to a Blazor-enabled resource, Blazor intercepts it. If the destination is a non-Blazor endpoint, enhanced navigation doesn’t apply, and the client-side JavaScript retries as a full-page load. This ensures no confusion to the framework about external pages that shouldn’t be patched into an existing page.
  • Fetch request: Instead of triggering a full-page reload, Blazor performs a fetch request using the browser’s Fetch API.
  • DOM patching: The rendered response content is then handled by Blazor patching it into the browser’s Document Object Model (DOM). Internally blazor.web.js implements a Levenshtein distance calculation to determine the most efficient method of patching the the DOM (Keep, Update, Insert or Delete). DOM patching allows Blazor to update the page without redrawing the entire screen minimizing rendering time.

@thabaum
Copy link
Contributor Author

thabaum commented Feb 23, 2024

@sbwalker After testing, wow, blink of an eye don't even notice, page changes themes as expected. Good to know all of this better now and see how data-enhance-nav works with this issue example to review the behavior. Thank you.

@thabaum
Copy link
Contributor Author

thabaum commented Feb 23, 2024

@sbwalker I am not sure why, but if I am not logged in, there is no issue it switches between themes, but after I login I actually continue to see this behavior. I log out and things work as expected.

I also ran into an error prior to this in the Maui app when I went to Site Settings to change the appearance through it I am investigating if I can reproduce to report.

image

image

image

Something I noticed is this only happens in the web browser when logged in, the Oqtane.Maui app works as expected logged in or not.

image

@sbwalker
Copy link
Member

sbwalker commented Feb 24, 2024

@thabaum you are mixing a bunch of different unrelated concepts in your note above. .NET MAUI is fully interactive so it would ignore data-enhance-nav even if it was present on a link (as it only applies to static rendering).

So let's focus on static rendering... there is a logic flaw in my earlier pull request - which you identified correctly:

Basically if a page in a site is using a different theme (which the framework warns you is not fully supported) then the ThemeType for the Page will be populated with a value. For most Pages this value is not populated which means that the value is inherited from the Site (ie. DefaultThemeType).

So when a site starts up let's assume the Home page uses the site theme and the Private page uses a different theme. The idea is that the navigation menu can set the data-enhance-nav in those scenarios where the page is using a different theme from the current page (as if a user navigates to the page, a full page refresh will be necessary to load the CSS properly). However relying solely on the existence of a ThemeType on the Page object is not correct, as this will only work when you navigate to the page with the new theme - it will not work when you then decide to navigate to another page which is using the default site theme.

The iteration logic in the Menu needs to compare the theme for the current page with the theme for each page which will be part of the menu and if they are different then it needs to assign the data-enhance-nav attribute.

So the other challenge you were running into is that the PageState.Page.ThemeType does not contain the raw value from the database - it is actually set by the SiteRouter based on fallback logic so that other areas of the framework can quickly determine the ThemeType for the page without having to implement their own fallback logic. So there needed to be some extra logic in the menu to account for this.

#3886 resolves this (and is similar to what you came up with) and is working for me fine regardless of whether I am logged in or not.

I should also mention that fixing this issue in the menu is not a complete solution. Basically ANY link which navigates to a page using a different theme needs to have the data-enhance-nav attribute. So the link from the logo does not work. The module actions link does not work. And if you assigned a custom theme to the Login page, the Login link would not work. And if you included links in your Html/Text content they would not work.

So although data-enhance-nav can resolve these types of issues, the solution is very invasive. This again emphasizes why Oqtane needs to include the warning message when people try to use multiple themes in a site - as it can cause problems with the user experience. I do not believe we should invest any more effort to focus on this problem as it should be considered an edge case and "not fully supported" (which is the case for multiple themes on interactive rendering as well)

sbwalker added a commit to sbwalker/oqtane.framework that referenced this issue Feb 24, 2024
sbwalker added a commit that referenced this issue Feb 24, 2024
removing fix for #3868 as it causes other performance issues
@sbwalker
Copy link
Member

@thabaum I just merged #3888 which reverses the data-enhance-nav solution. This is because it can cause performance issues on a site with a single theme. A theme can contain multiple theme components which contain different layouts or content ie. Home.razor, Default.razor, etc.. Each of these components has a different TypeName. So on a site where you have multiple theme components assigned to various pages, the data-enhance-nav "fix" will do a full page refresh when navigating between these pages because they have different theme typenames. This is not necessary and impacts the user experience and performance of the site. So this is not a valid solution. Instead, the themename would need to be derived from the themetype to determine if the themetypes are part of the same theme, or associated to a different theme. This would require different logic than what was included in #3886... and even if it was included, there are a number of other scenarios (described above) which would not work correctly.

@sbwalker sbwalker reopened this Feb 24, 2024
@thabaum
Copy link
Contributor Author

thabaum commented Feb 27, 2024

@sbwalker Shouldn't we compare pages by theme ID's set on pages? Should we introduce something like a ThemeTypeThemeId to compare? I am heading in this direction myself currently to see if I can work something out.

@sbwalker
Copy link
Member

sbwalker commented Feb 27, 2024

@thabaum I am fairly certain that the integrity attribute hash issue is related to Blazor's DOM patching algorithm which is part of enhanced navigation. I say this because when I originally developed the includeLink JavaScript method which is in Oqtane's Interop.js library I ran into issues relating to how browser's react to modifications to link elements.

If you want to modify a link element in the DOM which already has integrity and/or origin attributes assigned to it, you need to ensure that you remove those attributes first before you change the href attribute. If you don't, the browser will immediately notice the change to the href and it will use the existing the integrity and origin attributes to validate the resource - which will fail, which means the browser will not load the CSS document. And it does not matter if your logic then updates the integrity and crossorigin values to the correct values - the browser will ignore them because it only monitors changes to the href attribute. The Oqtane includeLink JavaScript logic is included below and demonstrates how this needs to be done to work correctly.

I believe Blazor's DOM patching algorithm is not following this logic which is causing the issues with CSS.

           if (link.href !== this.getAbsoluteUrl(href)) {
               link.removeAttribute('integrity');
               link.removeAttribute('crossorigin');
               link.setAttribute('href', href);
           }
           if (integrity !== "") {
               if (link.integrity !== integrity) {
                   link.setAttribute('integrity', integrity);
               }
           } else {
               link.removeAttribute('integrity');
           }
           if (crossorigin !== "") {
               if (link.crossOrigin !== crossorigin) {
                   link.setAttribute('crossorigin', crossorigin);
               }
           } else {
               link.removeAttribute('crossorigin');
           }

Perhaps one way to validate this is to generate link elements with the integrity and origin attributes at the beginning of the element and href at the end. Assuming the Blazor DOM patching algorithm is stupid and just processes attributes from left to right, this may process the attributes in the right order for the browser to load the CSS.

@sbwalker
Copy link
Member

sbwalker commented Feb 27, 2024

@thabaum yes, that proved my assumption. I modified ManageStyleSheets in App.razor so that the href attribute is included at the end of the link tag:

    private void ManageStyleSheets(List<Resource> resources)
    {
        if (resources != null)
        {
            string batch = DateTime.UtcNow.ToString("yyyyMMddHHmmssfff");
            int count = 0;
            foreach (var resource in resources.Where(item => item.ResourceType == ResourceType.Stylesheet))
            {
                count++;
                string id = "id=\"app-stylesheet-" + ResourceLevel.Page.ToString().ToLower() + "-" + batch + "-" + count.ToString("00") + "\" ";
                //_styleSheets += "<link " + id + "rel=\"stylesheet\" href=\"" + resource.Url + "\"" + (!string.IsNullOrEmpty(resource.Integrity) ? " integrity=\"" + resource.Integrity + "\"" : "") + (!string.IsNullOrEmpty(resource.CrossOrigin) ? " crossorigin=\"" + resource.CrossOrigin + "\"" : "") + " type=\"text/css\"/>" + Environment.NewLine;
                _styleSheets += "<link " + id + "rel=\"stylesheet\"" + (!string.IsNullOrEmpty(resource.Integrity) ? " integrity=\"" + resource.Integrity + "\"" : "") + (!string.IsNullOrEmpty(resource.CrossOrigin) ? " crossorigin=\"" + resource.CrossOrigin + "\"" : "") + " href=\"" + resource.Url + "\" type=\"text/css\"/>" + Environment.NewLine;
            }
        }
    }

and it works perfectly... home page has Oqtane theme assigned:

image

navigate to Private page with Blazor theme assigned:

image

@thabaum
Copy link
Contributor Author

thabaum commented Feb 27, 2024

right on, so you saying this is put to bed? I really feel like this should not be an issue so hoping that is what you just said ;) yes I see what your saying. I had a feeling it was something simple giving us a hard time....

@sbwalker
Copy link
Member

sbwalker commented Feb 27, 2024

@thabaum since I do not expect that Microsoft will modify the Blazor DOM patching algorithm, I guess this is the right solution (even if it seems rather strange). I should probably log this issue in the aspnetcore repo as well, as it is REALLY obscure but could affect how other DOM elements behave in enhanced rendering scenarios. Literally, I would have never even thought of this had I not developed the includeLink() JavaScript method ~3 years ago.

@thabaum
Copy link
Contributor Author

thabaum commented Feb 27, 2024

@sbwalker I agree this should not have been an issue... awesome! I am happy we did not give up on this and MS can get a little added love from it as well. Nice work!

@thabaum
Copy link
Contributor Author

thabaum commented Feb 27, 2024

@sbwalker Oqtane page loading with static SSR is sure snappy now ;)

@sbwalker
Copy link
Member

Logged here: dotnet/aspnetcore#54250

sbwalker added a commit that referenced this issue Feb 27, 2024
fix #3868 - multiple themes within a site
@sbwalker
Copy link
Member

@thabaum the nice thing about the above solution is that Oqtane can help developers avoid this problem through its CSS abstraction layer. Developers declare their Resources within the ThemeInfo.cs or theme components... and Oqtane constructs the link elements in the correct format which works with the Blazor DOM patching algorithm. So this helps developers avoid running into this problem in their own custom themes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants