Fix XAML Visibility for display:none

[agniuks]

As mentioned in #6355, components with display:none still participate in focus and UIA. This adds new logic in FrameworkElementViewManager to change visbility to collapsed when display: is set to none.

I tested the behavior through a new example in RNTester View component page using TextInput to confirm that focus is disabled when display:none.

Fixes #6355, also likely fixes react-navigation #9109.

Microsoft Reviewers: Open in CodeFlow

[agniuks]

Question: from this snapshot and inspecting component properties from VS, it seems like the visibility remains as "visibile," despite the behavior changing as if it was collapsing(and from setting breakpoints in FrameworkElementViewManager I can see that the right code is getting hit and the Visibility value gets set to "Collapsed" - but I never see this in XAML) - any idea what's going on?

[asklar]

Question: from this snapshot and inspecting component properties from VS, it seems like the visibility remains as "visibile," despite the behavior changing as if it was collapsing(and from setting breakpoints in FrameworkElementViewManager I can see that the right code is getting hit and the Visibility value gets set to "Collapsed" - but I never see this in XAML) - any idea what's going on?

if the tree dump says it's not collapsed then it's probably not collapsed : ) (and something else may be re-setting the property?)
Do you see the collapsed element in the UI? What does the Live Visual Tree and Property panes say?

[agniuks]

Question: from this snapshot and inspecting component properties from VS, it seems like the visibility remains as "visibile," despite the behavior changing as if it was collapsing(and from setting breakpoints in FrameworkElementViewManager I can see that the right code is getting hit and the Visibility value gets set to "Collapsed" - but I never see this in XAML) - any idea what's going on?

if the tree dump says it's not collapsed then it's probably not collapsed : ) (and something else may be re-setting the property?)
Do you see the collapsed element in the UI? What does the Live Visual Tree and Property panes say?

The "collapsed" element is no longer seen in the UI, but the Live Visual Tree and accompanying properties do not seem to change when display:none is toggled. If I undo the code change I added, same thing happens - component goes out of view when display is set to none, but you are still able to tab into those components - when I add the change back in, I'm no longer able to tab into components which are set to display:none, but according to the properties the visibility is still "visible"...

[rectified95]

Overall looks good. Just make sure you figure out the xaml dump issue

[rectified95]

Also, the JS checks are failing. You need to run yarn lint:fix - it'll resolve formatting issues. Also run yarn flow-check (with/without '-', dont remember)

[agniuks]

Also, the JS checks are failing. You need to run yarn lint:fix - it'll resolve formatting issues. Also run yarn flow-check (with/without '-', dont remember)

Will do, I ran yarn lint:fix but missed the latter.

[rectified95]

I've looked in the CI logs, and there are excessive white spaces - you must have run lint:fix earlier but it didn't get committed.

[asklar]

@AgneLukoseviciute looks like the test was removed, was that intentional?

[agniuks]

@AgneLukoseviciute looks like the test was removed, was that intentional?

Yep, I filed an issue(#7392) that will need to get fixed before I could get the tests working. The way the tests search in the e2e app isn't filtering the results down enough, at least in the case of View.

[asklar]

Thanks @AgneLukoseviciute ; in the meantime, can we add the test to a different/new page instead?

[agniuks]

Thanks @AgneLukoseviciute ; in the meantime, can we add the test to a different/new page instead?

Yep that should work - I'll add it.

[asklar]

@AgneLukoseviciute Looks like your snapshots are missing the Left/Top attached properties which the tests recently started requiring. Can you rebase your changes onto the latest master and re-generate the snapshot?

[rectified95]

Remember to remove the duplicate file

[agniuks]

Question: from this snapshot and inspecting component properties from VS, it seems like the visibility remains as "visibile," despite the behavior changing as if it was collapsing(and from setting breakpoints in FrameworkElementViewManager I can see that the right code is getting hit and the Visibility value gets set to "Collapsed" - but I never see this in XAML) - any idea what's going on?

if the tree dump says it's not collapsed then it's probably not collapsed : ) (and something else may be re-setting the property?)
Do you see the collapsed element in the UI? What does the Live Visual Tree and Property panes say?

The "collapsed" element is no longer seen in the UI, but the Live Visual Tree and accompanying properties do not seem to change when display:none is toggled. If I undo the code change I added, same thing happens - component goes out of view when display is set to none, but you are still able to tab into those components - when I add the change back in, I'm no longer able to tab into components which are set to display:none, but according to the properties the visibility is still "visible"...

Final comment before merging: Confirmed the elements are getting collapsed through Live Visual Tree and Property panes however the snapshot tests do not reflect this. Might require filing an issue - and if/when this gets fixed this test will fail and will just need updated snapshots to go through.

Additionally, I had to hard code the width for the view containing 2 Text and 2 Switch components as there was a weird width mismatch from running locally vs in the CI checks. To confirm the display:none property change was not the cause, I made sure update property for display was not getting hit when the prop was not used and commit a2ea3df confirmed the view width issue persists even when display:none is not used.

[asklar]

Question: from this snapshot and inspecting component properties from VS, it seems like the visibility remains as "visibile," despite the behavior changing as if it was collapsing(and from setting breakpoints in FrameworkElementViewManager I can see that the right code is getting hit and the Visibility value gets set to "Collapsed" - but I never see this in XAML) - any idea what's going on?

if the tree dump says it's not collapsed then it's probably not collapsed : ) (and something else may be re-setting the property?)
Do you see the collapsed element in the UI? What does the Live Visual Tree and Property panes say?

The "collapsed" element is no longer seen in the UI, but the Live Visual Tree and accompanying properties do not seem to change when display:none is toggled. If I undo the code change I added, same thing happens - component goes out of view when display is set to none, but you are still able to tab into those components - when I add the change back in, I'm no longer able to tab into components which are set to display:none, but according to the properties the visibility is still "visible"...

Final comment before merging: Confirmed the elements are getting collapsed through Live Visual Tree and Property panes however the snapshot tests do not reflect this. Might require filing an issue - and if/when this gets fixed this test will fail and will just need updated snapshots to go through.

Additionally, I had to hard code the width for the view containing 2 Text and 2 Switch components as there was a weird width mismatch from running locally vs in the CI checks. To confirm the display:none property change was not the cause, I made sure update property for display was not getting hit when the prop was not used and commit a2ea3df confirmed the view width issue persists even when display:none is not used.

@AgneLukoseviciute: filed #7586