Fixed IsEnabled Property not works on Tabs#26728
Fixed IsEnabled Property not works on Tabs#26728NirmalKumarYuvaraj wants to merge 21 commits intodotnet:mainfrom
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
/azp run |
This comment was marked as outdated.
This comment was marked as outdated.
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue5161.cs
Outdated
Show resolved
Hide resolved
This comment was marked as off-topic.
This comment was marked as off-topic.
src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellItemRenderer.cs
Outdated
Show resolved
Hide resolved
|
/azp run |
This comment was marked as outdated.
This comment was marked as outdated.
1 similar comment
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/rebase |
2f4d28b to
b984797
Compare
|
/azp run |
This comment was marked as outdated.
This comment was marked as outdated.
|
/azp run |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
b984797 to
3de2419
Compare
|
Azure Pipelines successfully started running 3 pipeline(s). |
rmarinho
left a comment
There was a problem hiding this comment.
Some little changes, we try not use the private keyword, also we try to use the brackets for making sure we don't missing something that should be in the if statement.
When making the changes please rebase on top of main too.
src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellItemRenderer.cs
Outdated
Show resolved
Hide resolved
src/Controls/src/Core/Handlers/Shell/ShellItemHandler.Windows.cs
Outdated
Show resolved
Hide resolved
|
Azure Pipelines successfully started running 3 pipeline(s). |
375da23 to
9cd908a
Compare
🤖 AI Summary📊 Expand Full Review🔍 Pre-Flight — Context & Validation📝 Review Session — Fixed CI failure for windows platform ·
|
| File | Reviewer | Feedback | Status |
|---|---|---|---|
| rmarinho | Remove keyword | Applied | |
| rmarinho | Remove keyword | Applied | |
| rmarinho | Use brackets for if statements | Applied | |
| MartyIX | Fix constructor indentation | Applied | |
ShellItemRenderer.cs:317 |
StephaneDelcroix | "start at 1?" (loop index No explicit response | question) |
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #26728 | Map IsEnabled to TabBar.Items on iOS; Add IsEnabled ViewModel + handler on PENDING (Gate) | ShellItemRenderer.cs, ShellItemHandler.Windows.cs, TabbedPageStyle.xaml, TabbedPage.Windows.cs, NavigationViewItemViewModel.cs |
Original PR | Windows |
🚦 Gate — Test Verification
📝 Review Session — Fixed CI failure for windows platform · 9cd908a
** FAILEDResult:**
Platform: ios
Mode: Full Verification
- Tests FAIL without NO (tests PASSED without fix)fix:
- Tests PASS with fix YES:
Root Cause of Gate Failure:
The tests pass WITHOUT the PR's fix because the iOS bug was already fixed in the base branch (a6129347). The base branch contains SetTabItemsEnabledState() which handles the same initial IsEnabled state that the PR's UpdateCellsEnabled() adds. The PR's iOS changes are therefore redundant.
The base branch already has:
SetTabItemsEnabledState()- sets initial IsEnabled state inCreateTabRenderersOnShellSectionPropertyChanged()- handles runtime IsEnabled changes- iOS 26-specific handling in a separate code path
The Windows changes (new IsEnabled property in NavigationViewItemViewModel, TabbedPageStyle.xaml binding, and OnShellItemPropertyChanged) are genuinely new - the base branch does NOT have Windows IsEnabled support. However, the test is PlatformAffected.iOS only and does not cover Windows.
🔧 Fix — Analysis & Comparison
📝 Review Session — Fixed CI failure for windows platform · 9cd908a
Status: SKIPPED - Gate did not pass
Gate failed because the iOS bug was already fixed in the base branch (SetTabItemsEnabledState() in a6129347). try-fix exploration was not performed as it requires Gate to pass first.
Selected Fix: N/A (Gate not passed)
Exhausted: N/A
📋 Report — Final Recommendation
📝 Review Session — Fixed CI failure for windows platform · 9cd908a
Final Recommendation: REQUEST CHANGES##
Summary
PR #26728 fixes ShellContent.IsEnabled not being respected for tabs in Shell on iOS, Windows, and macOS. The PR is a community contribution (Syncfusion/NirmalKumarYuvaraj) targeting issue #5161.
Gate outcome: Tests pass both with and without the PR's iOS fix because the iOS bug was already fixed in the base branch via SetTabItemsEnabledState(). The Windows fix is genuinely new and valid, but contains an event subscription bug that needs correction.
Root Cause (Original Bug)
ShellSection.IsEnabled = false was not applied to native tab items when tabs were first created, and not mapped to the Windows NavigationView at all. On iOS, the initial enabled state was never applied to UITabBarItem.Enabled. On Windows, NavigationViewItemViewModel had no IsEnabled property.
Code Review Findings
File: src/Controls/src/Core/Handlers/Shell/ShellItemHandler.Windows.cs
MapMenuItems() is called from multiple places (initial ConnectHandler, OnItemsChanged, UpdateTitle(), UpdateCurrentItem()). Each call adds item.PropertyChanged += OnShellItemPropertyChanged without first removing it. Since DisconnectHandler only removes one subscription per item, this creates dangling event handlers and duplicate OnShellItemPropertyChanged calls.
Fix needed: Unsubscribe before subscribing:
item.PropertyChanged -= OnShellItemPropertyChanged;
item.PropertyChanged += OnShellItemPropertyChanged;File: src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellItemRenderer.cs
The base branch (a6129347) already contains SetTabItemsEnabledState() which applies the initial IsEnabled state to tab items in both CreateTabRenderers() and OnItemsCollectionChanged(). The PR's UpdateCellsEnabled() does the same thing and is now redundant. Having both called in sequence in CreateTabRenderers() is confusing.
The PR's MaxTabs constant (replacing the magic number 5) is a valid cleanup, but UpdateCellsEnabled() itself should be removed.
File: src/Controls/tests/TestCases.HostApp/Issues/Issue5161.cs
The test host app class is marked PlatformAffected.iOS only, but the PR fixes Windows (and potentially Android/macOS) as well. No test validates the Windows behavior.
What's Good About This PR
- Windows
IsEnabledsupport is genuinely new and a real fix NavigationViewItemViewModel.IsEnabledproperty follows existing patterns (IsSelected)- XAML binding approach (
IsEnabled="{Binding IsEnabled}") is clean and idiomatic OnShellItemPropertyChangedcorrectly handles runtimeIsEnabledchanges on WindowsDisconnectHandlerproperly unsubscribes (the subscription leak is fixable)- Reviewer feedback (remove
privatekeyword, add brackets) was addressed
Requested Changes
-
[Critical] Fix duplicate event subscription in
MapMenuItems():item.PropertyChanged -= OnShellItemPropertyChanged; // Add this line item.PropertyChanged += OnShellItemPropertyChanged;
-
[Important] Remove redundant
UpdateCellsEnabled()fromCreateTabRenderers()on iOS. TheSetTabItemsEnabledState()call (already there) handles the same thing. Remove both the method call and the method definition, or consolidate intoSetTabItemsEnabledState(). -
[Recommended] Expand test to cover Windows: add
PlatformAffected.Allor at minimum document which platforms the test covers, and consider adding a Windows-specific test.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #26728 | Map IsEnabled to TabBar.Items (iOS, already in base) + NavigationViewItemViewModel.IsEnabled + OnShellItemPropertyChanged ( Gate FAILED (iOS redundant) | 5 impl files | Windows fix is valid; iOS fix is redundant; subscription bug | Windows) |
📋 Expand PR Finalization Review
Title: ✅ Good
Current: Fixed IsEnabled Property not works on Tabs
Description: ⚠️ Needs Update
✨ Suggested PR Description
[!NOTE]
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Root Cause
On iOS, Tab.IsEnabled was never mapped to UITabBarItem.Enabled, so all tabs were always interactive regardless of the property value. On Windows (both Shell and TabbedPage), BaseShellItem.IsEnabled was never passed to the NavigationViewItemViewModel, so the NavigationViewItem binding had no IsEnabled source and tabs could not be disabled.
Android was already handling this correctly and required no changes.
Description of Change
iOS — ShellItemRenderer.cs
- Extracted the magic number
5into aconst int MaxTabs = 5(minor cleanup). - Added
UpdateCellsEnabled(), called at the end ofCreateTabRenderers(), which iterates over the visible tab bar items (excluding the "More" overflow tab) and setsUITabBarItem.Enabled = ShellSection.IsEnabled.
Windows — NavigationViewItemViewModel.cs
- Added
IsEnabledproperty withINotifyPropertyChangedsupport, backed by_isEnabledfield.
Windows — Shell (ShellItemHandler.Windows.cs)
MapMenuItems()now setsvm.IsEnabled = bsi.IsEnabledviaSetValues()during initial construction.- Added
OnShellItemPropertyChangedhandler registered on eachBaseShellItem.PropertyChangedto propagate runtime changes ofIsEnabled(andTitle) to the correspondingNavigationViewItemViewModel. - Handler is unregistered in
DisconnectHandlerto prevent leaks.
Windows — TabbedPage (TabbedPage.Windows.cs)
MapItemsSourcenow setsvm.IsEnabled = page.IsEnabledwhen constructing view models for each page.
Windows — TabbedPageStyle.xaml
- Added
IsEnabled="{Binding IsEnabled}"binding to theNavigationViewItemstyle so the platform control reflects the view model value.
Issues Fixed
Platforms Tested
- Android (no changes needed — already working)
- iOS
- Mac Catalyst
- Windows
Code Review: ✅ Passed
Code Review — PR #26728
🟡 Functional Concern
iOS: UpdateCellsEnabled() only handles initial state — runtime IsEnabled changes are not reflected
File: src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellItemRenderer.cs
Problem: UpdateCellsEnabled() is called once inside CreateTabRenderers(). If a ShellSection.IsEnabled changes at runtime (e.g., the user's "Enable SecondTab" button in the test), the iOS UITabBarItem.Enabled will not be updated. The Windows implementation correctly subscribes to PropertyChanged for runtime updates, but iOS has no equivalent mechanism in this PR.
Looking at the existing UpdateMoreCellsEnabled(), it is also only called once, so this may be a pre-existing pattern in the codebase — but the test exercises a runtime enable action, which would fail on iOS after this PR unless some other code path handles it.
Recommendation: Add a PropertyChanged observer on each ShellSection (similar to the Windows OnShellItemPropertyChanged) that calls UpdateCellsEnabled() when IsEnabled changes. At minimum, document that runtime changes are not yet supported on iOS if that is intentional.
🟡 Minor Issues
1. [Issue] attribute scoped to iOS only, but fix covers Windows too
File: src/Controls/tests/TestCases.HostApp/Issues/Issue5161.cs
[Issue(IssueTracker.Github, 5161, "ShellContent IsEnabledProperty does not work", PlatformAffected.iOS)]The fix also covers Windows. The attribute should reflect all affected platforms:
[Issue(IssueTracker.Github, 5161, "ShellContent IsEnabledProperty does not work", PlatformAffected.All)]2. ThirdPage.OnButtonClicked accesses tab by hardcoded index
File: src/Controls/tests/TestCases.HostApp/Issues/Issue5161.cs
var secondTab = shell.CurrentItem?.Items[1];Accessing by index [1] is fragile. If tab order changes, this breaks silently. Consider looking up the tab by title or AutomationId instead:
var secondTab = shell.CurrentItem?.Items.FirstOrDefault(t => t.AutomationId == "SecondTab");3. Missing blank line before OnPropertyChanged in NavigationViewItemViewModel.cs
File: src/Core/src/Platform/Windows/NavigationViewItemViewModel.cs
After the IsEnabled property closing brace, there is no blank line before void OnPropertyChanged(...). Minor style inconsistency with the rest of the file.
✅ Looks Good
- Windows implementation is complete: initial value set, runtime changes handled via
PropertyChanged, and handler properly unregistered inDisconnectHandlerto prevent leaks. - The
TabbedPageStyle.xamlbinding is correct and consistent with other bindings in that file. - The "More" tab exclusion in
UpdateCellsEnabled()is correctly handled with a comment explaining the intent. const int MaxTabs = 5refactor is a clean improvement over the previous magic number.- UI test covers both the initial disabled state and runtime re-enabling.
|
@NirmalKumarYuvaraj looks like this is probably already fixed by #33369 and or #33337 can you confirm? |
|
Hi @@NirmalKumarYuvaraj. We have added the "s/pr-needs-author-input" label to this issue, which indicates that we have an open question/action for you before we can take further action. This PRwill be closed automatically in 14 days if we do not hear back from you by then - please feel free to re-open it if you come back to this PR after that time. |
1 similar comment
|
Hi @@NirmalKumarYuvaraj. We have added the "s/pr-needs-author-input" label to this issue, which indicates that we have an open question/action for you before we can take further action. This PRwill be closed automatically in 14 days if we do not hear back from you by then - please feel free to re-open it if you come back to this PR after that time. |
Issue Details
In the Shell, when the Tab.isEnabled property is set to false, the tab should be non-interactive and prevent user interaction. the current behavior allows the tab to be clicked.
Root Cause
The IsEnabled property is not correctly mapped to the TabBar.Items.IsEnabled property on iOS and macOS. In Windows, implementation property for IsEnabled property is not handled.
Description of change
A new method, UpdateCellEnabled, has been created to map the ShellSection.IsEnabled value to the Tabbar.Items.IsEnabled property on the iOS platform.
For the Windows platform, a new property was added to the NavigationViewItemViewModel and registered within the NavigationViewItem in the TabbedPageXaml.Windows page. The BaseShellItem.IsEnabled property is now mapped to the newly introduced IsEnabled property in the NavigationViewItemViewModel. Additionally, the IsEnabled property has been updated to respond to runtime changes through the property-changed event.
Validated the Behavior in the following Platforms
Issue fixed
Fixes #5161
Fixes #32996
Screenshots
BeforeFix-5161.1.mov
AfterFix-5161.1.mov