fix(module: overlay): positioning moved to js

[anddrzejb]

🤔 This is a ...

• New feature
• Bug fix
• Site / documentation update
• Demo update
• Component style update
• Bundle size optimization
• Performance optimization
• Refactoring
• Code style optimization
• Test Case
• Branch merge
• Other (about what?)

🔗 Related issue link

Fixes #1557
Fixes #1836
Fixes #1880
Fixes #1672
Fixes #1613
Fixes #1388
Fixes #1075
Fixes #1565
Fixes #1414
Fixes #886
Fixes #1928

💡 Background and solution

The goal of this PR is to improve performance for ServerSide without affecting Wasm. Major change has happened in the Overlay. All code that was responsible for overlay positioning, size change detection & scroll has been removed. Instead, a new class Overlay has been created in the file overlay.ts. That class handles:

• calculation of the position
• recalculation on container size change (uses ResizeObserver)
• recalculation on trigger mutation: either size or position (by monitoring style attribute, uses MutationObserver) - direct example is shown in the slider demo, when tooltip has to "follow" the slider's edge (it should be much more performant now)
• recalculation on scroll - for now any scroll causes recalculation, but this could be optimized to actually detect if recalculation is needed
  The calculation logic has been changed. Originally, all calculations were done from the perspective of top and left coordinates. The Overlay now is using all coordinates for its position calculation (no all at once, but depending on the selected placement, a pair is selected, ex: top & right, bottom & left, top&left`, etc). This addresses the changes proposed by @anranruye in PR fix(module:overlay): use right positioning for bottom-right placement #1799 (I also remebered about fix(module:overlay): fix overlay position when container element has border #1797).

The logic of the new calculation is also explained in this miro diagram(title "overlay calculation logic") - you may need to zoom-out.
A big challenge for me was also to include typescript tests with dom support, mocking & theories. This can serve as an example as well for future contributors on how ts tests might be done. I am sure the tests could be optimized, but I decided to leave them as such - maybe this simpler approach will be more understandable to someone who just wants to start with tests.

There are 2 breaking changes - I switched from PlacementType (class) to Placement enum in [Parameter] usage, while still using the PlacementType class and its logic. I also switched the TriggerType class to Trigger enum in [Parameter] usage. There are 2 reasons for changes:

1. According to Performance best practices #992, for performance purposes we should use primitive types to avoid unnecessary rendering of component subtrees
2. It is easier to move an enum between C# and JS. This is however a breaking change that will affect a lot of people.

What is missing:

• currently the adjustment strategy is the same as previously - if overlay does not fit, switch sides; the difference is that the sides are switched independently of a direction; what possibly is missing is to attempt to fit the overlay by disconnecting it from the trigger (in a way that it can even obstruct the trigger)
• more adjustment scenarios - when simple switching sides is not working, continue with all other possible placements
• scroll optimization - run only when it is needed; a sample is done in the ts file (commented out code in onScroll function)
• for RangePicker there probably should be more functionality available that handles situation (for example on a mobile device) when RangePicker just does not fit (width is larger than device screen width);

Testing experience - I found that Visual Studio Code is actually best to develop & test typescript. The test runner there actually picks up the tests and they can be run individually (and most important, debugged - no need to deal with god-awful debugging in browsers). It even picks up separate theory scenarios:

However Visual Studio Code does not come with all the goodies to run the tests. Maybe it would be beneficial to create a wiki entry on how to configure dev env for ts tests.

Other stuff:

1. Overlay & OverlayTrigger were optimized to reduce number of rerenderings
2. MenuItem had been changed not to render Tooltip every time but only on a compact & root menu (previously it was always rendering it)
3. Select, DatePicker & RangePicker have now available [Parameter] BoundaryAdjustmentMode

📝 Changelog

Language	Changelog
🇺🇸 English	TriggerType switched to AntDesign.Trigger enum. PlacementType switched to AntDesign.Placement enum. Autocomplete, Cascader, PaginationOptions, Select, DatePicker & RangePicker have now available [Parameter] BoundaryAdjustmentMode
🇨🇳 Chinese

☑️ Self Check before Merge

⚠️ Please check all items below before review. ⚠️

• Doc is updated/provided or not needed
• Demo is updated/provided or not needed
• Changelog is provided or not needed

[github-actions]

[ElderJames]

Awesome @anddrzejb ! It helps a lot!

[anddrzejb]

@ElderJames I forgot to mention in the PR comment: I checked with the same VPN setup as described in #1557 this comment. The execution time dropped to 273ms in comparison to 39ms locally. So that is significant improvement to previous 1.65sec (which was already optimized by my other attempts) and around 3-4 sec on the unaltered version.

[codecov]

Codecov Report

Merging #1848 (d6b9f07) into master (de2e668) will increase coverage by 2.18%.
The diff coverage is 56.04%.

@@            Coverage Diff             @@
##           master    #1848      +/-   ##
==========================================
+ Coverage   23.36%   25.54%   +2.18%     
==========================================
  Files         483      507      +24     
  Lines       31484    23994    -7490     
  Branches        0      229     +229     
==========================================
- Hits         7355     6129    -1226     
+ Misses      24129    17829    -6300     
- Partials        0       36      +36     

Impacted Files	Coverage Δ
components/auto-complete/AutoComplete.razor	0.00% <ø> (ø)
components/auto-complete/AutoComplete.razor.cs	0.00% <0.00%> (ø)
components/avatar/AvatarGroup.razor	0.00% <ø> (ø)
components/avatar/AvatarGroup.razor.cs	0.00% <0.00%> (ø)
components/breadcrumb/BreadcrumbItem.razor	0.00% <ø> (ø)
components/cascader/Cascader.razor	83.22% <ø> (+9.30%)	⬆️
components/core/Component/Overlay/Overlay.razor	85.71% <ø> (+28.57%)	⬆️
...onents/core/Component/Overlay/OverlayTrigger.razor	80.00% <ø> (+13.33%)	⬆️
...re/JsInterop/ObservableApi/intersectionObserver.ts	0.00% <0.00%> (ø)
...ents/core/JsInterop/ObservableApi/observableApi.ts	100.00% <ø> (ø)
... and 457 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de2e668...d6b9f07. Read the comment docs.

[ElderJames]

That's great improvement!

Is there any chance that those adjustment strategies will be added?

currently the adjustment strategy is the same as previously - if overlay does not fit, switch sides; the difference is that the sides are switched independently of a direction; what possibly is missing is to attempt to fit the overlay by disconnecting it from the trigger (in a way that it can even obstruct the trigger)
more adjustment scenarios - when simple switching sides is not working, continue with all other possible placements
scroll optimization - run only when it is needed; a sample is done in the ts file (commented out code in onScroll function)
for RangePicker there probably should be more functionality available that handles situation (for example on a mobile device) when RangePicker just does not fit (width is larger than device screen width);

[anddrzejb]

That's great improvement!

Is there any chance that those adjustment strategies will be added?

A chance exists, but we need to discuss which strategies should be used and in what order. And maybe we could be smart about it and not just try every possible scenaio. Also, some components should not apply disconnect scenarios (for example tooltip). I will try to prepare some kind of description of what I foresee as a problem and maybe we could discuss what is the preferred solution. Do you want to make these solution part of this PR?

[ElderJames]

@anddrzejb Of course not, you can bring them to another PR.

I think we could refer other libraries, such as:

• https://material.angular.io/cdk/overlay/overview
• https://popper.js.org/

[anddrzejb]

I am on a leave now, but I will think about the improvements.
I did have a look at popper before. I liked it approach to scrolling behavior when the arrow is involved, but I am not sure if this is what we want. It seems to me that we already surpass the react antD when it comes to overlay behaviour on boundaries detection. And I have other performance improvements on my mind that I think should be addressed before any other new visual features. I know the performance changes are not as flashy as visual and really hard to advertise, but nevertheless still very needed.

[ElderJames]

Totally agree.
Let's review this PR first and I hope to merge it this weekend if it goes well。

[ElderJames]

I think it also needs to follow the resize of the trigger to adjust the positioning.

[anddrzejb]

I think it also needs to follow the resize of the trigger to adjust the positioning.

@ElderJames you are absolutely right. Somehow I missed that. Please check my commit 0cecc3c. It was really easy with ResizeObserver.

[ElderJames]

The dropdown of submenu will be repeatedly opened in some cases

[anddrzejb]

The dropdown of submenu will be repeatedly opened in some cases

I think this is not repeatedly shown, but sometimes overlay stays open instead of closing. I just tried to recreate it and failed. This is a bug that is coming from original implementation. Overlay hiding is triggered either by JS call or by overlay trigger (as far as I remember) and I did not change that. I already noticed that when there is a lot of things happening, some js events seem to be lost - I guess this may happen, since js is only single threaded. I don't know - maybe a web worker could solve us this problem? This is something I never worked with (there is always a first time though...). What do you think?

[anddrzejb]

The dropdown of submenu will be repeatedly opened in some cases

So I had a closer look and indeed I can see that this problem is coming from trigger event onMouseLeave. Actually, js event fires properly. The problem happens in the Overlay. I believe the underlaying problem is indeed single threaded nature of js. Anyway, the issue is that sometimes the Show method of the Overlay does not manage to finish (it has few awaits inside of it and the js itself can sometimes take a bit of time to execute) when the onMouseLeave event triggers Hide method of the Overlay. During Hide method, a check is done if _isOverlayShow == true. The problem is that sometimes it is still false because Show method hasn't finished processing and did not set it to true. The code to handle this scenario is basically a loop (up to 100 times) that waits for _isOverlayShow to become true. I probably would prefer using a while loop, but I am afraid there is something I overlooked and we will end up with an endless one. Please have a look at the gif and the code snippets I used during the gif generation. I added this piece of code in the begining of Show method:

        internal async Task Show(int? overlayLeft = null, int? overlayTop = null)
        {
            Console.WriteLine($"Showing {Trigger.Id}");
...

and this code in the beginning of Hide:

        internal async Task Hide(bool force = false)
        {
            Console.WriteLine($"Hiding {Trigger.Id}");
            if (_isOverlayDuringShowing)
            {
                for (int i = 0; i < 100; i++)
                {
                    Console.WriteLine($"Hiding {Trigger.Id}: waiting {i+1} time...");
                    await Task.Delay(10);
                    if (_isOverlayShow)
                    {
                        break;
                    }
                }
            }
            if (!_isOverlayShow)
            {
                Console.WriteLine($"Hiding {Trigger.Id}: stopped due to _isOverlayShow: {_isOverlayShow}");
                return;
            }
...

And the result:

As you can see on the gif, sometimes the loop repeats up to 16 times. In my other tests, I had it up to 26 times. 100 times x 10ms gives a moderate 1 second wait time. At this point I am waiting for suggestions.

I applied the above changes to commit 708797a.

[anddrzejb]

Commit 670c829 is extracting the for loop into a WaitFor function (the loop I described in my previous comment). I found out that in case of Slider with visible tooltip on the ServerSide the same wait is needed - I guess because of the overlay is being rendered before trigger gets back the element reference. Without the wait, overlay throws an exception, because Trigger.Ref.Id is null.

[anddrzejb]

@ElderJames no worries, I was waiting for that PR to be merged. Now I need to figure out how it works, so I can apply it properly 😄 but I think it is a great improvement. Kudos to @tonyyip1969!

[ElderJames]

Thank you @anddrzejb ! This is a huge improvement!

[anddrzejb]

@ElderJames I am very happy it was approved. Now I will be waiting for devs complaints about breaking changes 😨