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

fixes FluentPageScript with Blazor Hybrid/MAUI and WebAssembly Standalone #1633

Merged
merged 9 commits into from
Mar 18, 2024

Conversation

c0g1t8
Copy link
Contributor

@c0g1t8 c0g1t8 commented Mar 5, 2024

Pull Request

πŸ“– Description

Prevents listeners from being added when running in BlazorWebView

🎫 Issues

Event listeners are being added to collapse/expand button for FluentNavMenu when used in Blazor Hybrid/MAUI.

Note: See comment below for root cause.

πŸ‘©β€πŸ’» Reviewer Notes

This is an addtional fix related to #1560 that did not cover Blazor Hybrid/MAUI.

The fix is based on determining if the component is running in BlazorWebView.

function isRunningInWebView() {
    return navigator.userAgentData.brands.find(e => e.brand == 'Microsoft Edge WebView2') != null;
}

πŸ“‘ Test Plan

I ran all the tests that I used for #1560. I created a new Blazor Hybrid/MAUI application created from the default template and updated it with the Fluent components to match the defaults created for a server rendered webassembly application to verify the desired behaviour.

βœ… Checklist

General

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

  • I have added a new component
  • I have added Unit Tests for my new compontent
  • I have modified an existing component
  • I have validate Unit Tests for an existing component

⏭ Next Steps

@c0g1t8 c0g1t8 marked this pull request as draft March 5, 2024 02:39
@dvoituron
Copy link
Collaborator

As I've already said, I don't think it's a good idea to test all cases in this way: isRunningInWebView,
isRunningInWASM, ... This way, we will have dozens of tests in few days with all browser possibilities.

I think it would be better to look at the source of the problem: what's going wrong?
Is an event not detected under MAUI? If so, why isn't it detected?
Is it something else?

const element = document.getElementById("my-element");
if (typeof element.onclick === "function") {
    console.log("An onclick event handler exists for this element.");
} else {
    console.log("No onclick event handler found.");
}

@dvoituron dvoituron self-requested a review March 5, 2024 09:03
@c0g1t8
Copy link
Contributor Author

c0g1t8 commented Mar 5, 2024

@dvoituron

I was debating submitting this PR but decided to draft one so the behaviour would consistent across the different hosting/rendering options. I agree that this definitely 'smells'.

I've also discovered additional quirks related UI state not being preserved when transitioning from static rendered to Blazor rendered.

  • If you collapse the menu as soon as the UI appears, it will suddenly expand when Blazor rendering/interactivity is available.
  • There's actually an additional transition when the interactivity option is WebAssembly. Once for the server static rendering, again when WebAssembly starts the page, and finally when completely running in WebAssembly.

This also violates this note in the documentation regarding not coupling to render mode.

All of this to provide the collapsing and expanding functionality of the FluentNavMenu when statically rendered.

As for the source of problem, I'm still trying to figure that out. πŸ˜„. I've learned a lot about the rendering process as I've looked at this issue. I've been unable to find any detailed documentation on the transition from static rendered to interactive rendered.

I've also considered but not tried:

  • Removing the event listener added in JavaScript in the OnInitializedAsync. Thus, no testing of the environment would be required. This is likely going to exhibit the quirky behaviour I mentioned above as well.
  • Moving the desired behvaiour into Javascript only so that the collapse and expand behaviour is only handled in JavaScript. This would be my next candidate to explore more thoroughly.

@c0g1t8
Copy link
Contributor Author

c0g1t8 commented Mar 6, 2024

I've been doing some further research looking for a root cause as per Denis's comments.

I found

Data is a property inherited from Microsoft.FluentUI.AspNetCore.Components.FluentComponentBase. It is being used to prevent the script from being run

@if (Data is null)
{
<FluentPageScript Src="./_content/Microsoft.FluentUI.AspNetCore.Components/Components/NavMenu/FluentNavMenu.razor.js"></FluentPageScript>
}

An alternative to making any code changes to the FluentUI-Blazor library would be to explicity do this in the NavMenu component on a per application basis for Blazor Hybrid/MAUI and WebAssembly hosted applications.

@dvoituron @vnbaaij Thoughts??

Note: This still does not address the UI state transition issues. See #1633 (comment)

@vnbaaij
Copy link
Collaborator

vnbaaij commented Mar 6, 2024

I'm using the Data parameter to prevent the script from being included multiple times when the menu hase a couple of layers. It does not prevent the script form being run.

@c0g1t8
Copy link
Contributor Author

c0g1t8 commented Mar 6, 2024

I'm using the Data parameter to prevent the script from being included multiple times when the menu hase a couple of layers. It does not prevent the script form being run.

That makes sense. I forgot to include the following screenshot above😞.

image

I made this change to the NavMenu component in the Blazor Hybrid/MAUI and WebAssembly hosted test applications and it effectively gave me the results I wanted. The javascript listeners were not added. Both these applications defaulted to using the Blazor provided event handler.

It's basically a temporary work-around for 4.4.1. I'm currently using it in my production WebAssembly hosted application where I originally ran across this issue. πŸ˜„

I'm still looking into a better long term fix that would also address UI state change issue.

@dvoituron
Copy link
Collaborator

An alternative to making any code changes to the FluentUI-Blazor library would be to explicity do this in the NavMenu component on a per application basis for Blazor Hybrid/MAUI and WebAssembly hosted applications.

Indeed, I prefer to have an explicit property (for example by adding an option to AddFluentUIComponents) and if it's possible to define a default value using the detected environment (MAUI, Web, ...).

@c0g1t8
Copy link
Contributor Author

c0g1t8 commented Mar 9, 2024

An alternative to making any code changes to the FluentUI-Blazor library would be to explicity do this in the NavMenu component on a per application basis for Blazor Hybrid/MAUI and WebAssembly hosted applications.

Indeed, I prefer to have an explicit property (for example by adding an option to AddFluentUIComponents) and if it's possible to define a default value using the detected environment (MAUI, Web, ...).

@dvoituron I think the explicit declaration is the way to go.

As I reflect on how this started and on @vnbaaij comment, Enhanced navigation should not have applied. I think this is a Blazor bug.

When a Blazor Web App is first pre-rendered/statically rendered, the javascript for static SSR is sent to the browser where it's executed on the enhancedload event. When the application is ready, another render occurs but this time there is no enhancedload event.

In Blazor Hybrid/MAUI and WebAssembly hosted applications, no pre-rerendering is done but the enhancedload event is still being triggered on first render. Preventing the enhancedload event from occuring would make the component behaviour consistent no matter the hosting environment.

If this is truly a bug, by explicitly declaring it would make it easier to deprecate in the future when it's fixed.

Assuming that using an explicit property as the approach, how should it be implemented?

I see two options:

  1. Add an option called 'UseStaticSsr' with a default value of true for AddFluentUIComponents. UseStaticSsr would need to be explicitly set to false for Blazor Hybrid/MAUI and WebAssembly hosted applications. Add logic that conditionally checks for this option before including fluent-page-script in the FluentPageScript.razor file.
  2. Add a property to FluentNavMenu named UseStaticSsr with a default value of true. Then conditionally check this in FluentNavMenu.razor.

Note:

  • Currently only the FluentNavMenu component is using the FluentPageScript component that adds the javascript for static SSR. Adding an option to AddFluentUIComponents could be considered overkill.
  • As for detecting the environment, the only method I've found so far is in javascript so far.

I can see pros and cons for either option. Thoughts?

@vnbaaij
Copy link
Collaborator

vnbaaij commented Mar 13, 2024

In Blazor Hybrid/MAUI and WebAssembly hosted applications, no pre-rerendering is done but the enhancedload event is still being triggered on first render. Preventing the enhancedload event from occuring would make the component behaviour consistent no matter the hosting environment.

Yes, that is the ultimate goal.

Assuming that using an explicit property as the approach, how should it be implemented?

I see two options:

  1. Add an option called 'UseStaticSsr' with a default value of true for AddFluentUIComponents. UseStaticSsr would need to be explicitly set to false for Blazor Hybrid/MAUI and WebAssembly hosted applications. Add logic that conditionally checks for this option before including fluent-page-script in the FluentPageScript.razor file.

I think this is the way to go.

  1. Add a property to FluentNavMenu named UseStaticSsr with a default value of true. Then conditionally check this in FluentNavMenu.razor.

This seems like a too specific solution for me.

Note:

  • Currently only the FluentNavMenu component is using the FluentPageScript component that adds the javascript for static SSR. Adding an option to AddFluentUIComponents could be considered overkill.

The NavMenu is the only place I've added it on purpose. I anticipate that we would like to have more components that offer enhanced functionality (other than just displaying) when used in a SSR scenario. Because of the issues this is giving, I have not yet looked at which components we could use this. If we do, I expect we will be using the FluentPageScript again.

  • As for detecting the environment, the only method I've found so far is in javascript so far.

I believe extra/new/better functionality is on the roadmap. I'll sk the Blazor team what is the est way to do this now.

@c0g1t8
Copy link
Contributor Author

c0g1t8 commented Mar 17, 2024

Background

I thought the root cause of the issue was that the enhancedload event was the cause of this issue so I filed an issue. After some feedback from @MackinnonBuck and attempting to create a minimal repro, I discovered a difference between how PageScript and FluentPageScript was implemented. The latter was a pretty faithful implementation in Typescript. The primary difference is was when the handler was added to the enhancedload event.

In PageScript we have in BlazorPageScript.lib.module.js:

export function afterWebStarted(blazor) {
    customElements.define('page-script', class extends HTMLElement {
        static observedAttributes = ['src'];


        // We use attributeChangedCallback instead of connectedCallback
        // because a page-script element might get reused between enhanced
        // navigations.
        attributeChangedCallback(name, oldValue, newValue) {
            if (name !== 'src') {
                return;
            }


            this.src = newValue;
            unregisterPageScriptElement(oldValue);
            registerPageScriptElement(newValue);
        }


        disconnectedCallback() {
            unregisterPageScriptElement(this.src);
        }
    });


    blazor.addEventListener('enhancedload', onEnhancedLoad);
}

In FluentPageScript, this initialization is done in the index.ts file.

if (typeof blazor.addEventListener === 'function' && mode === 'web') {
blazor.addEventListener('enhancedload', onEnhancedLoad);
}

and

export function beforeStart(options: any) {
customElements.define("fluent-design-theme", DesignTheme);
customElements.define("split-panels", SplitPanels);
customElements.define('fluent-page-script', FluentPageScript);
beforeStartCalled = true;
}

Why is happening?

customElement fluent-page-script is always defined in index.ts. This means that attributeChangedCallback in FluentPageScript.ts is called in for any fluent-page-script tag present. This in turns calls registerPageScriptElement that calls initializePageScriptModule which then calls the onLoad in FluentNavMenu.razor.js.

onLoad is triggered by attributeChangedCallback NOT by the enhancedload event. And because the customElement fluent-page-script is always defined. The Javascript handlers were always added to the toggle.

Fix

The fix is to move line 307 to after line 297, so the custom element is only defined for Blazor Web Apps.

image

This fix also addresses the root cause of #1560 as well.

@c0g1t8
Copy link
Contributor Author

c0g1t8 commented Mar 17, 2024

@vnbaaij

The NavMenu is the only place I've added it on purpose. I anticipate that we would like to have more components that offer enhanced functionality (other than just displaying) when used in a SSR scenario. Because of the issues this is giving, I have not yet looked at which components we could use this. If we do, I expect we will be using the FluentPageScript again.

With this fix, enhanced functionality will be controlled from FluentPageScript. 🀞

@MackinnonBuck made a comment with regards to PageScript being designed for with with @page. Seems like FluentPageScript should be renamed to describe it's intent - FluentComponentScript, FluentSSRScript.

@dvoituron

πŸ™ for pushing to find a root cause.

@c0g1t8 c0g1t8 marked this pull request as ready for review March 17, 2024 19:01
@c0g1t8 c0g1t8 changed the title fix FluentNavMenu.razor.js for Blazor Hybrid/MAUI fixes FluentPageScript with Blazor Hybrid/MAUI and WebAssembly Standalone Mar 17, 2024
@vnbaaij vnbaaij enabled auto-merge (squash) March 18, 2024 10:06
@vnbaaij
Copy link
Collaborator

vnbaaij commented Mar 18, 2024

Thanks Gary for perservering in finding the root cause!

@vnbaaij vnbaaij merged commit 1f8b7d5 into microsoft:dev Mar 18, 2024
4 checks passed
@c0g1t8 c0g1t8 deleted the fixSSR2 branch March 18, 2024 17:31
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

Successfully merging this pull request may close these issues.

None yet

3 participants