Parent visibility not respected on add

[HalfWhitt]

Describe the bug

#2950 fixed inherited visibility in descendant widgets... but not entirely, it turns out. That test verifies that everything behaves as expected when the visibility property is set after the widget hierarchy is already established. However, the mechanism that looks up the chain for a hidden ancestor isn't done when adding a child.

The simplest fix for this is to put child.style.apply("visibility", child.style.visibility) in Widget.add, but that seems like mixing concerns — Widget probably shouldn't be special-casing a specific Pack property like that, and it can't be tested in the current test_apply.py setup which uses dummy example nodes instead of actual widgets.

Perhaps Pack — or even BaseStyle? – could gain a hook to be called once its node has been added to a new parent. So child.style.apply("visibility", child.style.visibility) would become something like child.style._added_to_new_parent(), so the style engine can perform whatever inheritance-related checks it needs to do. This could potentially go in Node.add. Thoughts?

Steps to reproduce

def test_set_visibility_inherited_on_add():
    """Nodes should be hidden when added to an already-hidden parent."""
    parent = ExampleParentNode("parent", style=Pack(visibility=HIDDEN))
    child = ExampleNode("child", style=Pack())
    parent.add(child)
    child._impl.set_hidden.assert_called_once_with(True)

Expected behavior

A child's on-screen visibility should respect an ancestor's hidden status upon being added as its child.

Screenshots

No response

Environment

• Toga: Main branch

Logs

Additional context

No response

[freakboy3742]

Nice catch!

You mention special casing visibility when a node is added to a parent... but is there any reason this shouldn't just be a reapply() of the entire style? Or the point at which the applicator is actually added to the widget?

Although you've identified visibility as a property that is affected by this - but that's because visibility is the only property in Pack that can be affected by the parent style value. In future, when we have full CSS... cascading means that parents can impact on any style.

At the very least, I'd argue that this isn't a "visibility" feature - it's a "refresh cascading properties" feature. I guess that's what is implied by your _add_to_new_parent() API; but it seems to me that this is something we can track internally because we know when a widget has a change in parent (or we can know, because the widget parent is stored at the Travertino level), rather than needing a new API that is invoked at the Toga level.

[freakboy3742]

In the process of reviewing #2484, I've noticed a case where cascading is already an issue - background color on Winforms requires reapplying background color when the parent is changed because of how transparency is handled. So a style-wide approach would seem to make sense here.

[HalfWhitt]

You mention special casing visibility when a node is added to a parent... but is there any reason this shouldn't just be a reapply() of the entire style?

That's true, it could just be a reapply(). It would apply all properties, including non-cascading ones, but it would be simple.

However, another wrinkle I didn't consider is that this needs to propagate downward. When you add Parent to Grandparent, Child also needs to compute its cascaded properties. The way visibility is currently implemented, it searches upwards, potentially all the way to the root — but this will snowball when propagated downward.

We'd only need a single tree traversal if, instead, added_to_new_parent (cascade is probably a better name) carries the computed values downward with it as an argument. (I suppose we could accomplish the same thing by "caching" any computed cascading properties on each widget, and making its child's reapply method check there, but that seems messier.)

Or the point at which the applicator is actually added to the widget?

Hm... currently we assign an applicator when the widget is first created. Are you suggesting we avoid assigning an applicator then, and only assign it once the node is given a parent? There's nothing stopping you from adding a widget to a parent that doesn't currently have a parent or applicator of its own, and reparenting a widget that has children still needs to propagate the changes downward.

At the very least, I'd argue that this isn't a "visibility" feature - it's a "refresh cascading properties" feature. I guess that's what is implied by your _add_to_new_parent() API; but it seems to me that this is something we can track internally because we know when a widget has a change in parent (or we can know, because the widget parent is stored at the Travertino level), rather than needing a new API that is invoked at the Toga level.

I think we're on the same page here. Node.add(), in Travertino, would include a line notifying the style that its node's parent has changed, presumably by calling a cascade method on it.

In the process of reviewing #2484, I've noticed a case where cascading is already an issue - background color on Winforms requires reapplying background color when the parent is changed because of how transparency is handled. So a style-wide approach would seem to make sense here.

Interesting, I'll have to take a look at that one.

[freakboy3742]

However, another wrinkle I didn't consider is that this needs to propagate downward. When you add Parent to Grandparent, Child also needs to compute its cascaded properties. The way visibility is currently implemented, it searches upwards, potentially all the way to the root — but this will snowball when propagated downward.

We definitely need to make sure we're not setting off a notification cascade by working in both directions. However, I don't think that's a concern here.

Application in both directions is already happening. The apply method on Pack looks to the parent to establish if the parent is hidden; but when you use the applicator, the applicator iterates over children to apply the new effective hidden state. The difference is between the style value and the effective value.

We'd only need a single tree traversal if, instead, added_to_new_parent (cascade is probably a better name) carries the computed values downward with it as an argument. (I suppose we could accomplish the same thing by "caching" any computed cascading properties on each widget, and making its child's reapply method check there, but that seems messier.)

In the case you're describing, adding parent to grandparent means you need to interrogate Grandparent's visibility (which means potentially looking all the way to root); but the re-application of style is applied from Parent down. The style attribute of each node doesn't change; but the effective value passed to the applicator might. But this is already implemented; the only difference here is triggering the apply() on parent when it is re-parented.

Or the point at which the applicator is actually added to the widget?

Hm... currently we assign an applicator when the widget is first created. Are you suggesting we avoid assigning an applicator then, and only assign it once the node is given a parent? There's nothing stopping you from adding a widget to a parent that doesn't currently have a parent or applicator of its own, and reparenting a widget that has children still needs to propagate the changes downward.

Broadly speaking, that's what I was suggesting - but to be clear, it's also me noodling around looking for other ways to approach the problem, not a coherent API proposal. I'm mostly observing that when an applicator is assigned, one of the side effects is to apply the entire style... which is essentially what we're looking to do here. If we're able to combine the two requirements, that seems like a win.

At the very least, I'd argue that this isn't a "visibility" feature - it's a "refresh cascading properties" feature. I guess that's what is implied by your _add_to_new_parent() API; but it seems to me that this is something we can track internally because we know when a widget has a change in parent (or we can know, because the widget parent is stored at the Travertino level), rather than needing a new API that is invoked at the Toga level.

I think we're on the same page here. Node.add(), in Travertino, would include a line notifying the style that its node's parent has changed, presumably by calling a cascade method on it.

Bikeshed color and bugs we haven't thought of notwithstanding, 👍

[HalfWhitt]

However, another wrinkle I didn't consider is that this needs to propagate downward. When you add Parent to Grandparent, Child also needs to compute its cascaded properties. The way visibility is currently implemented, it searches upwards, potentially all the way to the root — but this will snowball when propagated downward.

We definitely need to make sure we're not setting off a notification cascade by working in both directions. However, I don't think that's a concern here.

Application in both directions is already happening. The apply method on Pack looks to the parent to establish if the parent is hidden; but when you use the applicator, the applicator iterates over children to apply the new effective hidden state. The difference is between the style value and the effective value.

But this is already implemented; the only difference here is triggering the apply() on parent when it is re-parented.

Wow, I've been staring at style so hard I forgot that the applicator already does this. Yeah, okay, this (probably) isn't as big a change as I was thinking for a second there.

Bikeshed color and bugs we haven't thought of notwithstanding, 👍

It's getting late for me here, but soon (probably tomorrow) I can see about putting together at least a proof of concept... and then we can kibitz about what color to paint it. 😉

I'm mostly observing that when an applicator is assigned, one of the side effects is to apply the entire style... which is essentially what we're looking to do here. If we're able to combine the two requirements, that seems like a win.

Ah, yeah I think I see where you're going there. I'm not sure either what that might look like, yet.