Setting the visibility of UI nodes is confusing and redundant

[alice-i-cecile]

What problem does this solve or what need does it fill?

The visibility of UI entities can be controlled in two distinct ways: via the Style component's field, and by setting Visibility (you could also muck with ComputedVisibility but please don't).

This is both confusing, and results in inconsistent behavior (see #5360 and the resulting #5361).

What solution would you like?

Remove the Display field of Style, and let users set this via the Visiblity component instead. In the interface with taffy, construct the taffy::Style struct information based on the combination of these components.

This is simple, allows for direct querying and is much more consistent with the rest of the engine.

What alternative(s) have you considered?

1. Use Display as the canonical representation. This is confusing, and ties us tightly to CSS's model rather than being modular. It's also very inconsistent with the way visibility is handled elsewhere.
2. Automatically sync these. This has non-zero computational costs and serious complexity (and bug-risk) costs, for no apparent benefit.

Additional context

Prompted by @mockersf complaining about this in #5361 (comment) <3

[hymm]

Just for some context. In CSS changing the display property removes the node from layouting, while visibility just makes it not visible, but doesn't affect the layout.

Not sure if this is similar in bevy.

[mockersf]

Not sure if this is similar in bevy.

Oh... then that's how Bevy currently work... do you know if events are still triggered by a not visible element?

[hymm]

do you know if events are still triggered by a not visible element?

they don't get interaction events.

https://codepen.io/hymm/pen/MWVpKXR?editors=1111

[alice-i-cecile]

@mockersf what do you think the right path forward is then?

1. Merge [Merged by Bors] - Disable UI node Interaction when ComputedVisibility is false #5361 as is for reduced footgunnery and consistency with the web. Then clean up this duplication in a separate more controversial PR.
2. Just jump straight to deduplication.

[sixfold-origami]

Not sure if this is similar in bevy.

Oh... then that's how Bevy currently work... do you know if events are still triggered by a not visible element?

In general, you can't trigger interaction on them because you can't see them, but you can still trigger them mechanically (through javascript). Also, because of the inspector, you can just... unhide them. This was actually the source of a few bugs that I've encountered in the wild.

[alice-i-cecile]

WRT triggering them mechanically, users will be able to achieve the same effect by manually setting the Interaction component values.

That inspector stuff seems like a nasty security issue...

[mockersf]

@mockersf what do you think the right path forward is then?

1. Merge Disable UI node Interaction when ComputedVisibility is false #5361 as is for reduced footgunnery and consistency with the web. Then clean up this duplication in a separate more controversial PR.
2. Just jump straight to deduplication.

If we merge #5361 we would work completely like CSS... I think that would be what I prefer, but it would need documentation to explain. But it should be feasible. Mentioning on the visibility component that it hides and stop events, but don't remove from layout, and mention the display field.

Keeping the two separated (visibility / computed in layout) can be useful to allow for invisible nodes in layout, so that everything stays in the same place when something else becomes visible / invisible.

[Nilirad]

#5361 has been merged. What's the status of this?

[alice-i-cecile]

To close this, we either need

a) consensus that we want both + #5380 fixed
b) removal of either Display::None or UiNode visibility
c) autosynchronization (plz no)

[Nilirad]

I would like the (a) scenario, just like in CSS. Also, the visibility vs display has been well documented. Implementing #5380 will also consolidate the pattern.

[inodentry]

I would also like the (a) scenario. In fact, I have deliberately made use of that distinction in my own UIs before. The ability to choose between "don't render, but keep it in the layout" and "remove it from the layout", is a useful feature to have.

[doup]

I also like (a), given how Bevy already follows CSS it feels natural (although it's surprising for those not familiar with it). It has its use as pointed by inodentry.

Should #7629 mention that it closes this issue?

[alice-i-cecile]

Agreed, I think that #7629 will resolve this.