Skip to content

MudDataGrid : Fix Resize column when neighboring is hidden#9643

Merged
ScarletKuro merged 9 commits intoMudBlazor:devfrom
vernou:9585-datagrid-bug-resize
Aug 19, 2024
Merged

MudDataGrid : Fix Resize column when neighboring is hidden#9643
ScarletKuro merged 9 commits intoMudBlazor:devfrom
vernou:9585-datagrid-bug-resize

Conversation

@vernou
Copy link
Member

@vernou vernou commented Aug 16, 2024

Description

When a column is resized and the neighboring column is hidden, then the application crash.

Fixes #9585

How Has This Been Tested?

I added a test case that reproduce the bug.

Type of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (fix or improvement to the website or code docs)

Checklist

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

@github-actions github-actions bot added bug Unexpected behavior or functionality not working as intended PR: needs review labels Aug 16, 2024
@vernou vernou marked this pull request as draft August 16, 2024 20:51
@vernou
Copy link
Member Author

vernou commented Aug 17, 2024

The fix work in a demo project, but not in the test.

In test, ElementReferenceExtensions.MudGetBoundingClientRectAsync return always null for HeaderCell.
I don't know why.


I just see a difference in the tag of the header :

In the demo (where it works) :

<th scope="col" class="mud-table-cell" style="" colspan="1" _bl_4cb3ad82-3a91-4780-838d-12652c54752e="">

In the demo (where it don't work) :

<th scope="col" class="mud-table-cell" style="" colspan="1" blazor:elementReference="">

Sound like blazor:elementReference isn't translated in the test.


Does anyone have any hints?

@vernou
Copy link
Member Author

vernou commented Aug 17, 2024

The test display a data grid with 3 columns. When a column is hidden, the element reference on <th> disappears :

...
// Open column menu
var columnMenu = comp.FindAll("th .mud-menu button").ElementAt(1);
columnMenu.Click();

// Click on 'Hide' menu item
comp.WaitForAssertion(() => comp.FindAll(".mud-list-item").ElementAt(1));
var hideMenuItem = comp.FindAll(".mud-list-item").ElementAt(1);
hideMenuItem.InnerHtml.Contains("Hidde");

var thBeforeHidde = comp.Find("th").OuterHtml;
// <th scope="col" class="mud-table-cell" style="" colspan="1" blazor:elementreference="101ba4cb-9363-4b36-bfe1-7a904c3349a8">
hideMenuItem.Click();
var thAfterHidde = comp.Find("th").OuterHtml;
// <th scope="col" class="mud-table-cell" style="" colspan="1" blazor:elementreference="">

@ScarletKuro
Copy link
Member

In test, ElementReferenceExtensions.MudGetBoundingClientRectAsync return always null for HeaderCell.
I don't know why.

MudGetBoundingClientRectAsync calls a JavaScript method mudElementRef.getBoundingClientRect, since bUnit doesn't support Js invocation, you need to mock it to return whatever you desire:

Context.JSInterop.Setup<int>("mudElementRef.getBoundingClientRect").SetResult(new BoundingClientRect(...?));

More info here: https://bunit.dev/docs/test-doubles/emulating-ijsruntime.html

@vernou
Copy link
Member Author

vernou commented Aug 18, 2024

@ScarletKuro, thank. It's worked and it progress.

@vernou
Copy link
Member Author

vernou commented Aug 18, 2024

Now, I need to simulate the mouse move.
The datagrid listen the global event document.pointermove by EventListener.

Have you a hint/example to mock it?

@ScarletKuro
Copy link
Member

ScarletKuro commented Aug 18, 2024

I don't think you can really simulate the pointer* events in bUnit: bUnit-dev/bUnit#1497

You need to do it on a different level, either on the DataGridColumnResizeService or the IEventListener level.

As I understand the datagrid subscribes to event:

await _eventListener.SubscribeGlobal<PointerEventArgs>(EventPointerMove, 0,
eventArgs => OnApplicationPointerMove(eventArgs, rightToLeft));

And then the EventListener fires the OnEventOccur when the even acquires:

[JSInvokable]
public async Task OnEventOccur(Guid key, string @eventData)

And it invokes this lambda OnApplicationPointerMove(eventArgs, rightToLeft).
Technically you could just simulate the OnEventOccur similarly on how I'm doing it with the IBrowserViewportObserver:

await component.InvokeAsync(async () => await browserViewportService.RaiseOnResized(new BrowserWindowSize { Height = 720, Width = 1280 }, Breakpoint.Lg, subscription.JavaScriptListenerId));

I'm just invoking the method directly in the test and the RaiseOnResized is also an [JSInvokable].

@codecov
Copy link

codecov bot commented Aug 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.63%. Comparing base (28bc599) to head (b8eb58a).
Report is 436 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #9643      +/-   ##
==========================================
+ Coverage   89.82%   90.63%   +0.80%     
==========================================
  Files         412      406       -6     
  Lines       11878    12786     +908     
  Branches     2364     2474     +110     
==========================================
+ Hits        10670    11588     +918     
+ Misses        681      640      -41     
- Partials      527      558      +31     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vernou vernou marked this pull request as ready for review August 18, 2024 19:41
@vernou vernou marked this pull request as draft August 18, 2024 19:48
@ScarletKuro
Copy link
Member

I think last commit contains things that it shouldn't.
I see you have a separate project for testing, I think in future you can use the MudBlazor.UnitTests.Viewer directly, if you add a test component with XYZTest naming, you can start this project and find this test and play with it.

@vernou vernou force-pushed the 9585-datagrid-bug-resize branch from 959d9b4 to bba2dc6 Compare August 18, 2024 20:17
@vernou
Copy link
Member Author

vernou commented Aug 18, 2024

I think last commit contains things that it shouldn't.

My mistake... I forget to select files before commit... so demo projects were pushed. It's cleaned.

you can use the MudBlazor.UnitTests.Viewer directly

Thank for the tip.

@vernou
Copy link
Member Author

vernou commented Aug 18, 2024

Finally, the test reproduces the bug.
@ScarletKuro, thank for the help.

@ScarletKuro
Copy link
Member

@ScarletKuro, thank for the help.

Thanks for the fix. That fix makes sense, all read and writes should be done via the hidden state not the parameter, and apparently there is one more place where parameter is used instead of the state.

@vernou vernou marked this pull request as ready for review August 18, 2024 21:35
@vernou
Copy link
Member Author

vernou commented Aug 19, 2024

@ScarletKuro, nice catch.

Column.Hidden is also used in SelectColumn.razor and HierarchyColumn.razor.
The razor template need to use the parameter property or the parameter state field?

@ScarletKuro
Copy link
Member

Column.Hidden is also used in SelectColumn.razor and HierarchyColumn.razor.

This ones are fine.

@ScarletKuro ScarletKuro merged commit d05aef5 into MudBlazor:dev Aug 19, 2024
@ScarletKuro
Copy link
Member

ScarletKuro commented Aug 19, 2024

Thanks.

It's interesting that EventListener is transient. I would make it a singleton so that if two components subscribe on same event, you could have one subscription on the JavaScript side and two on the C# side, rather than a 1:1 ratio. Something similar does the IBrowserViewportObserver.
It would allow to use:

var eventListener = (EventListener)Context.Services.GetRequiredService<IEventListener>();

in tests.
But that's a totally different topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Unexpected behavior or functionality not working as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MudDataGrid crashes with both Hideable and ColumnResizeMode="ResizeMode.Column"

2 participants