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

Programmatically adding new NavigationViewItems to an existing NavigationView.MenuItems collection results in the added items rendering inside a generated NavigationViewItem node #1

Closed
GlitterHorn opened this issue Sep 16, 2018 · 3 comments
Assignees
Labels
bug Something isn't working team-Controls Issue for the Controls team

Comments

@GlitterHorn
Copy link

GlitterHorn commented Sep 16, 2018

Example:

XAML:

<ct:NavigationView x:Name="NavMain" ItemInvoked="NavMain_ItemInvoked">
    <ct:NavigationView.MenuItems>
        <ct:NavigationViewItem Content="Home"/>
    </ct:NavigationView.MenuItems>
</ct:NavigationView>

C#:

private void RenderMenu()
{
    NavMain.MenuItems.Add(new NavigationViewItemSeparator());

    foreach (TarotSuit suit in deck.Suits)
    {
        // make a nav menu item for the suit
        NavigationViewItem newMenu = new NavigationViewItem();
        newMenu.Content = suit.Suit;
        newMenu.Icon = new SymbolIcon(Symbol.OutlineStar);

        NavMain.MenuItems.Add("menu");
    }
}

The resulting UI and live tree:
2018-09-15_14-48-29

It looks like NavigationView.MenuItems.Add(object) is setting the value passed as the Content property of a generated NavigationViewItem, resulting in an added NavigationViewItem set as the Content property of a container NavigationViewItem.

Rendering really goes sideways on this:

2018-09-15_18-19-34

If I pass a string to NavigationView.MenuItems.Add(object), the result is a single NavigationViewItem with the Content property set to the string. Not only that, but if I return all the items after they've been created, the returned list doesn't contain these extra, auto-generated NavigationViewItem controls, so I think this is happening when the control is rendered, rather than how the XAML controls are created before rendering.

@jesbis jesbis added the bug Something isn't working label Sep 17, 2018
@GlitterHorn
Copy link
Author

I think this issue may be related to my windows build. This happened on 18231, but since updating to 18240, NavigationViewItem controls are being generated properly.

I'll leave this open though, since there might still be something else going on.

@kmahone
Copy link
Member

kmahone commented Sep 17, 2018

@ClairelyClaire that you for reporting this issue.

This behavior happens when a Windows.UI.Xaml.Controls.NavigationViewItem is added to a Microsoft.UI.Xaml.Controls.NavigationView. You only get the correct behavior when a Microsoft.UI.Xaml.Controls.NavigationViewItem is added to a Microsoft.UI.Xaml.Controls.NavigationView. The controls in the Microsoft.UI.Xaml namespace are from the NuGet package. The controls in the Windows.UI.Xaml namespace are the ones that are from the OS.

If you update your code to create Microsoft.UI.Xaml.Controls.NavigationViewItems, you should get the correct behavior.

Because the MenuItem property is typed to collection of object, the type system doesn't really help here.

Thanks for the feedback.

@kmahone kmahone closed this as completed Sep 17, 2018
@GlitterHorn
Copy link
Author

Thanks! I added this to my using:

using NavigationView = Microsoft.UI.Xaml.Controls.NavigationView;
using NavigationViewItem = Microsoft.UI.Xaml.Controls.NavigationViewItem;
using NavigationViewItemInvokedEventArgs = Microsoft.UI.Xaml.Controls.NavigationViewItemInvokedEventArgs;

I don't think I had that added when I posted this issue, but things look to be working now.

RBrid added a commit that referenced this issue Jan 18, 2019
RBrid added a commit that referenced this issue Jan 25, 2019
…d/IsChildAvailableHeightConstrained with ContentOrientation (#214)

* Step #1: Introduce Scroller/ScrollViewer.ContentOrientation.

* Step #2: Replace IsChildAvailableWidthConstrained/IsChildAvailableHeightConstrained usage with ContentOrientation and remove them.

* Updating ScrollerDynamicPage test UI.

* Fixing test ValidateSupportForScrollerConfigurationChanges.
ranjeshj added a commit that referenced this issue Jan 25, 2019
* Update localization workflow for breaking changes (#162)

* Adding instrumentation for AcrylicBrush.TintLuminosityOpacity propert… (#149)

* Adding instrumentation for AcrylicBrush.TintLuminosityOpacity property change

* Changing telemetry logging of property change as a member instead of class

* Oops.  Forgot to remove the old id.

* Back-porting external changes from the OS repo. (#175)

* Avoid Narrator pronouncing the MenuFlyoutItem's keyboard accelerator. (#181)

* Avoid Narrator pronouncing the MenuFlyoutItem's keyboard accelerator.

* Expanding fix to pre-19H1 releases.

* Add screen capture and improved logging to helix tests (#176)

Enable /screencaptureonerror for TAEF tests and upload the screenshots to helix storage.
Also upload te.wtl log file.

Report uploaded urls of both of these to the Azure DevOps test report so that someone investigating a test failure can link directly to them instead of having to go to through the Mission Control site.

Here is an example run showing this in action:
https://dev.azure.com/ms/microsoft-ui-xaml/_build/results?buildId=1332&view=ms.vss-test-web.test-result-details

* Update the bug report issue template to not use checkboxes (#183)

* Renaming a few Scroller/ScrollViewer APIs (#185)

* Fixing Scroller's SnapPointsWithInvalidArgsThrow test.

* Renaming enum ScrollerState to InteractionState.
Renaming its enum values Interacting to Interaction and CustomAnimation to Animation.

* Updating ScrollBarVisibility enum. Replacing Collapsed with Hidden.

* Fix OS ports (#186)

* Update feature request issue template (#191)

* Make CalendarViewCalendarItemBackground the same as RS3 (#189)

* add SystemControlBackgroundAltHighRevealBackgroundBrush

* lift CalendarViewCalendarItemRevealBackground

* move CalendarViewCalendarItemRevealBackground to difference file

* Update TwoPaneView for API changes  (#193)

* MUX NavigationView should throw if WUXC NavViewItems are added to it (#69)

MUX NavigationView should throw if WUXC NavViewItems are added to it

Use IsItemItsOwnContainerOverride as a hook to approximate list contents change events and deny any WUX items in the MUX list.

[Internal Issue](https://microsoft.visualstudio.com/OS/ft_xamlcon/_workitems/edit/19017881)

* Applying some of the changes that came out of the API review meetings: (#200)

- Renaming enumerations
    ScrollerScrollMode --> ScrollMode
    ScrollerZoomMode --> ZoomMode
    ScrollerChainingMode --> ChainingMode
    ScrollerRailingMode --> RailingMode
    ScrollerInputKind --> InputKind
- Removing the ScrollMode.Auto enum value and make ScrollMode.Enabled the new default. I am preserving the old code with #ifdef USE_SCROLLMODE_AUTO since the Auto value may be re-introduced later.

* Adding telemetry for XLayout types (#169)

* Adding telemetry for XLayout types

* Update localization workflow for breaking changes (#162)

* Adding instrumentation for AcrylicBrush.TintLuminosityOpacity propert… (#149)

* Adding instrumentation for AcrylicBrush.TintLuminosityOpacity property change

* Changing telemetry logging of property change as a member instead of class

* Oops.  Forgot to remove the old id.

* Adding telemetry for XLayout types

* Undoing noisy merge

* Undoing another noisy merge

* Changes for Mux Acrylic and Reveal fallback is islands (#192)

* Keyboard Navigation: 'Back' button cannot be reached using tab order  (#204)

* Keyboard Navigation: 'Back' button cannot be reached using tab order
Remove special case code that disabled tab navigation on the NavigationView back button.
[Internal Issue](https://microsoft.visualstudio.com/OS/ft_xamlcon/_workitems/edit/20149845)
* Remove test case that assumes tab navigation is disabled on the back button
* Remove no longer used m_buttonHolderGettingFocusRevoker

* Add hints to cause GUID calculations to happen during precompile (#209)

* CommandBarFlyout Narrator fix (#199)

* Adding FlowsTo and FlowsFrom to ensure that the primary commands are connected to the secondary commands.

* Disabling new test to cut down on noise until it's fixed to work on all versions (#213)

[GitHub Issue](#211)

* Renaming Scroller.Child to Scroller.Content (#212)

* Scroller.Child renamed to Scroller.Content.

* Merging with master.

* Fix an animation bug of NavigationViewList. (#188)

*  Fix an animation bug of NavigationViewList.

* modify code to avoid /0 exception.

* When in mutiselect make sure that the treeview list supports selection pattern (#210)

* When in mutiselect make sure that the treeview list supports selection pattern

* Update Helix SDK version (#215)

This should improve PR validation from forks.

Also publish binlog from RunTestsInHelix.proj to aid debugging.

* Fix RatingControl scaling (#187)

* Fix spacing

* Fix margins

* Update comment

* Update calculations

* Remove inline

* Update comments

* Update dev/RatingControl/RatingControl.cpp

Co-Authored-By: kaiguo <[email protected]>

* Remove margin changes in markup

* Fix crash when scrolling nested ItemsRepeaters (#152)

* fix add pin bug
* update fix and also fix unrealiable canbringintoviewelements test
* fix unpinning case

* Pane Title is completely not visible in HC mode (#177)

* The pane title was being obscured by the pane toggle button. Outside of High Contrast that works
fine... but in HC the button background is opaque. I could split out resources for that background
to be transparent in HC... but fundamentally this is a weird layout to have the text _under_ the
button. It means that any non-transparent background is actually doing the wrong thing with regards
to layering on the text. So... I opted to change how this was built.

The key to fixing this was to have the button support both an icon and a piece of text. Since the
control already has a forked template this was pretty easy. Move the ContentPresenter to be to the
right of the fixed icon and update the layout accordingly.

The search button was riding on the same style as the toggle button for some reason. That wasn't
really necessary, as with a couple tweaks it could use a lightly styled standard Button template.

* Fix the behaviour of TreeViewItem.HorizontalContentAlignment (#203) (#218)

Remove the StackPanel containing the ContentPresenter and move layout
and indentation to the containing Grid.

Fix #203

* Remove workaround made for a midl bug. (#121)

* Fixing ScrollViewer name to turn on its tests in Helix. (#224)

* Replacing Scroller and ScrollViewer's IsChildAvailableWidthConstrained/IsChildAvailableHeightConstrained with ContentOrientation (#214)

* Step #1: Introduce Scroller/ScrollViewer.ContentOrientation.

* Step #2: Replace IsChildAvailableWidthConstrained/IsChildAvailableHeightConstrained usage with ContentOrientation and remove them.

* Updating ScrollerDynamicPage test UI.

* Fixing test ValidateSupportForScrollerConfigurationChanges.
StephenLPeters pushed a commit that referenced this issue Feb 9, 2019
…d/IsChildAvailableHeightConstrained with ContentOrientation (#214)

* Step #1: Introduce Scroller/ScrollViewer.ContentOrientation.

* Step #2: Replace IsChildAvailableWidthConstrained/IsChildAvailableHeightConstrained usage with ContentOrientation and remove them.

* Updating ScrollerDynamicPage test UI.

* Fixing test ValidateSupportForScrollerConfigurationChanges.
llongley added a commit that referenced this issue Jul 26, 2019
This implements a number of improvements to our test reporting, based on things both suggested to me and observed by me.  Namely:

- We now always report all test sub-results, even when a test failed all ten times - previously we'd just mark it as a single failure in that case and publish the first error message.  We already had in place the ability to mark a test as failed if more than half of the sub-results failed, so we can simply always report tests tentatively as skipped and then update them later when processing test results.

- Additionally, this adds the ability to generate a report on unreliable tests from CI builds X days before the present day that shows which tests were the most unreliable and which gives build links to see how they failed.  This also changes things so we no longer mark a build as "succeeded with issues" when a test was unreliable - this is because we have enough unreliable tests that it's currently always being marked as such, such that that designation is losing meaning at the moment.

- This also adds a new YAML build definition that outputs a report on the last 30 days every Sunday, which can be used to create issues for tests that are extremely unreliable.  You can see an example of this in the "Output unreliable test report" step of this build:

https://dev.azure.com/ms/microsoft-ui-xaml/_build/results?buildId=24460

- Our scripts now include parameters instead of strictly running based off of environment variables, which allows them to be run and debugged locally in addition to working in the lab.

- Finally, this contains a few cleaning up of things, such as making it so we properly start at "Attempt #1" when listing test sub-results, instead of starting at "Attempt #0".
@jevansaks jevansaks added the team-Controls Issue for the Controls team label Nov 7, 2019
jesbis added a commit that referenced this issue Jan 14, 2020
* RadialGradientBrush Initial Commit (#1)

Initial commit of RadialGradientBrush implementation
RBrid added a commit that referenced this issue Jan 26, 2021
…isual updates (#3991)

* ListViewItem/GridViewItem Visual Updates - Phase #1

* Incremental work for new insider Windows SDK support

* Incremental work #2 for new insider Windows SDK support

* Incremental work #3 for new insider Windows SDK support

* Incremental work #4 for new insider Windows SDK support

* Incremental work #5 for new insider Windows SDK support

* Incremental work #6 for new insider Windows SDK support

* Upgrading to insider Windows SDK version 10.0.21296.0

* Switching to use more accurate UseInternalSDK instead of UseInsiderSDK

* ListViewItem/GridViewItem: moving High Contrast resources from ResourceKey='AccentAAFillColorDisabledBrush' type to Color='{ThemeResource SystemColorWindowColor}' type.

* Simplifications: no more LatestWithVisualUpdate, LatestWithoutVisualUpdate options, _vu suffix, exception catch, RSx changes

* Deleting unneeded GridViewItem_themeresources.xaml

* Using Insider SDK again

* Using Insider SDK again

* Upgrading CustomTasks.dll version from 1.0.70 to 1.0.71 after its publication

* Also using 21h1_generic.xaml on 21H1 OS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working team-Controls Issue for the Controls team
Projects
None yet
Development

No branches or pull requests

4 participants