Add delay before showing Reconnection UI

[SQL-MisterMagoo]

Summary of the changes (Less than 80 chars)

• Add a short delay before showing the default ReconnectDisplay

Addresses #19050

[captainsafia]

Thanks for submitting this PR, @SQL-MisterMagoo! Feel free to move it from draft to review-ready whenever you'd like us to take a look at it.

[SQL-MisterMagoo]

Thanks for submitting this PR, @SQL-MisterMagoo! Feel free to move it from draft to review-ready whenever you'd like us to take a look at it.

@captainsafia I was waiting to see if the tests passed but it looks like some failed - - I couldn't tell for sure if that was related to this change or something else as it looks platform related which doesn't feel like a code issue?

I'll start the review process anyway :)

[captainsafia]

@SQL-MisterMagoo One test failure that looks suspicious is that of the Microsoft.AspNetCore.Components.E2ETest.ServerExecutionTests.CircuitGracefulTerminationTests.ReloadingThePage_GracefullyDisconnects_TheCurrentCircuit test. Does this test pass locally for you?

[SQL-MisterMagoo]

@captainsafia
You are right - I had misunderstood the messages from the test run and it is indeed failing - and definitely because of this change.
I will have a look at the tests and see what can be done to allow for the delayed UI.

FAIL tests/DefaultReconnectDisplay.test.ts
    ÔùÅ DefaultReconnectDisplay ÔÇ║ adds element to the body on show

      expect(received).toBe(expected) // Object.is equality

      Expected: "block"
      Received: "none"

        14 |         expect(element).toBeDefined();
        15 |         expect(element!.id).toBe('test-dialog-id');
      > 16 |         expect(element!.style.display).toBe('block');
           |                                        ^
        17 |
        18 |         expect(display.message.textContent).toBe('Attempting to reconnect to the server...');
        19 |         expect(display.button.style.display).toBe('none');

        at Object.<anonymous> (tests/DefaultReconnectDisplay.test.ts:16:40)

    ÔùÅ DefaultReconnectDisplay ÔÇ║ updates message on fail

      expect(received).toBe(expected) // Object.is equality

      Expected: "block"
      Received: "none"

        46 |         display.failed();
        47 |
      > 48 |         expect(display.modal.style.display).toBe('block');
           |                                             ^
        49 |         expect(display.message.innerHTML).toBe('Reconnection failed. Try <a href=\"\">reloading</a> the page if you\'re unable to reconnect.');
        50 |         expect(display.button.style.display).toBe('block');
        51 |     });

        at Object.<anonymous> (tests/DefaultReconnectDisplay.test.ts:48:45)

    ÔùÅ DefaultReconnectDisplay ÔÇ║ updates message on refused

      expect(received).toBe(expected) // Object.is equality

      Expected: "block"
      Received: "none"

        58 |         display.rejected();
        59 |
      > 60 |         expect(display.modal.style.display).toBe('block');
           |                                             ^
        61 |         expect(display.message.innerHTML).toBe('Could not reconnect to the server. <a href=\"\">Reload</a> the page to restore functionality.');
        62 |         expect(display.button.style.display).toBe('none');
        63 |     });

        at Object.<anonymous> (tests/DefaultReconnectDisplay.test.ts:60:45)

  Test Suites: 1 failed, 2 passed, 3 total
  Tests:       3 failed, 9 passed, 12 total

[captainsafia]

@SQL-MisterMagoo Sounds good! We might end up needing to modify that E2E test to account for the delay and only check for the element on the page after a certain amount of time.

[dnfadmin]

All CLA requirements met.

[SQL-MisterMagoo]

@captainsafia I discovered (new stuff!) jest useFakeTimers and that seems to have resolved the timing issues - and I found a copy-paste issue so the DefaultReconnectDisplay tests are passing for me.

However, that are still test failures in the E2E tests - but to my eye they appear to be unrelated to the code changes I made.

I am going to switch back to the master branch locally and run the tests again to see if those same E2E tests are failing.

I cannot see any evidence of ReloadingThePage_GracefullyDisconnects_TheCurrentCircuit failing locally in this commit, but we'll have to see what the CI tests show.

thanks for you help!

[SteveSandersonMS]

Thanks for contributing this!

Is there a reason to prefer implementing the delay in JS and not in CSS?

I was thinking a CSS approach would be simpler and more robust, because it means we don't have to worry about scenarios like cancelling the delayed appearance of the UI if we happen to reconnect during the delay period.

[SQL-MisterMagoo]

@SteveSandersonMS That is the kindest way of putting it 😄 you are, of course, right - I had missed that little problem.

I opted for using setTimeout because I couldn't see a nice way to make use of CSS here - you mentioned keyframes previously, I think - but at the moment, the only styling is inline on the modal.style (div) and of course, keyframes can't go in there.

I could add a style element to the modal just for the keyframes, or I could look into whether something like
transition: visibility can be coerced into working?

[SteveSandersonMS]

The reconnection model already has an ID (components-reconnect-modal by default) so the template's .css file can match it like #components-reconnect-modal { ... }.

[SQL-MisterMagoo]

Any pointers on where to find the template css would be greatly appreciated 😄

[captainsafia]

@SQL-MisterMagoo The CSS for the Blazor server template is located at https://github.com/dotnet/aspnetcore/tree/master/src/ProjectTemplates/Web.ProjectTemplates/content/BlazorServerWeb-CSharp/wwwroot/css.

[simonziegler]

All, many many thanks for this. @SQL-MisterMagoo in particular you're a star here. The idea from @SteveSandersonMS makes crystal clear sense. This is actually something I may be able to assist with @SQL-MisterMagoo if you want. Let me know on Gitter please if that's the case.

[SQL-MisterMagoo]

@SteveSandersonMS

Could I have your thought on this - we could simply modify the template CSS - but I think it involves this substring selector:

#components-reconnect-modal[style*="display: block"],
#components-reconnect-modal[style*="display:block"] {
    animation: VISIBILITY 500ms linear;
}

@keyframes VISIBILITY {
    0%,99% { visibility: hidden; }
    100% { visibility: visible; }
}

An alternative I was considering would be to add/remove a CSS class to control the animation delay :

#components-reconnect-modal.show {
    animation: VISIBILITY 500ms linear;
}

And then the code in the DefaultReconnectDisplay would need to change to toggle the class show at the same time as the style display.
This would also mean a change to the tests to check for the class show, something like expect(element!.classList).toContain('show');

In either case, I have yet to assess how a test would work for a CSS animation - my initial googling suggests that testing for the state of the modal at the end of the animation would be difficult - any thoughts on this?

[captainsafia]

An alternative I was considering would be to add/remove a CSS class to control the animation delay :

I like the testability and explicitness of this pattern.

In either case, I have yet to assess how a test would work for a CSS animation - my initial googling suggests that testing for the state of the modal at the end of the animation would be difficult - any thoughts on this?

I don't think you actually need to test the animation if the pattern above is used. We'd just need to test if the show class was applied to the element. Let me know if I am missing something here.

[SQL-MisterMagoo]

@captainsafia Thanks for the response, I'll switch over to setting the class.

I'm a little cautious about updating the PR right now as I haven't been able to run successful tests on the master branch for a few days - it seems like something is broken in the repo - I've pulled the latest and done a clean, a restore, a build - they all work but build -test fails every time.

[captainsafia]

@SQL-MisterMagoo Trying running the tests from inside the VS test explorer. If you want to run them from the command line, you'll probably have better luck using dotnet run in the target project instead of running the top-level tests.

[SQL-MisterMagoo]

Gah I thought I'd rebased recently enough, I'll try to do it again to fix the conflict

[SQL-MisterMagoo]

@captainsafia Could you please review

[captainsafia]

Looks good. We have two follow-ups to this:

• Submit a patch with this change to 3.1 (I'll take care of this)
• Notify users running into this issue of this change so they can implement it themselves in their codebases

[captainsafia]

@SQL-MisterMagoo Oof. Looks like you need to regenerate the blazor.server.js file one last time to resolve the merge conflicts.

[SQL-MisterMagoo]

@SQL-MisterMagoo Oof. Looks like you need to regenerate the blazor.server.js file one last time to resolve the merge conflicts.

@captainsafia I don't get any diff from rebuilding that file - is this conflict because you are going to patch against a different branch? Should I rebase onto a different branch?

[captainsafia]

The rebase should happen against master. You can try running:

$ git fetch origin
$ git rebase origin master

This should pull in new changes to some of the files in Web.JS folder. It should also prompt you about a conflict in the blazor.server.js file as seen above. Usually, I just call yarn build inside the Web.JS directory to regenerate the built JS file after resolving the merge conflicts in the TS files, if there are any.

[SQL-MisterMagoo]

@captainsafia I had to try a couple of times - I got unlucky and another commit updated the JS file after I rebased but before I pushed !

[ghost]

Hello @captainsafia!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.