Feature: possibility to hide nodes #241

New Issue

2016-11-03T08:58:51-07:00

roxlu commented

2016-11-03 08:58:51 -07:00

(Migrated from github.com)

I know this is currently by design, but it would be great if we could hide / show nodes. Any roadmap on this? Maybe we can have a chat and I can implement this?

emilsjolander commented

2016-11-03 09:20:04 -07:00

(Migrated from github.com)

What is the use case? Why not just remove the node and re-attach it, it should have the same effect unless i'm misunderstanding you.

roxlu commented

2016-11-03 09:24:12 -07:00

(Migrated from github.com)

Hey @emilsjolander yes removing would be a solution though probably less convenient that toggling the visiblity. My use case is a GUI that uses CssLayout where you often have things that you want to show/hide: for example selectboxes, menus, popups etc. Adding/removing a node dynamically (and making sure the order stays the same) means quite some extra bookkeeping compared to having a visibility toggle. Especially because it's quite common in guis to hide/show things. Do you have any concerns with adding this visibility flag?

emilsjolander commented

2016-11-03 09:28:35 -07:00

(Migrated from github.com)

I have concerns about adding things which are not needed as it bloats the API. That said I think you have made a good point regarding keeping ordering and the bookkeeping needed for that. Adding visibility to cssnode would make sense. It would be great if you can implement this as then you can also test that it works for your situation 👍 All that should be required is to skip invisible children when iterating of the list of children. This could be done by if (child->invisible) continue; at the top of each child loop. Make sure to add tests for this as well 💯

I have concerns about adding things which are not needed as it bloats the API. That said I think you have made a good point regarding keeping ordering and the bookkeeping needed for that. Adding visibility to cssnode would make sense. It would be great if you can implement this as then you can also test that it works for your situation 👍 All that should be required is to skip invisible children when iterating of the list of children. This could be done by `if (child->invisible) continue;` at the top of each child loop. Make sure to add tests for this as well 💯

👍 1

roxlu commented

2016-11-03 09:29:56 -07:00

(Migrated from github.com)

(Y) awesome, I'll look into this.

emilsjolander commented

2016-11-03 09:30:31 -07:00

(Migrated from github.com)

Thanks!

woehrl01 commented

2016-11-03 09:56:16 -07:00

(Migrated from github.com)

Not sure if this is a great feature as in pure css visibility: hidden still takes the space. The same for UIView on iOS. See 26bcc1e which explicitly reverts this from css-layout.

To not confuse users, maybe its better to not name this something like visible. Maybe it should be called something like the css display:none which leads to the desired behaviour.

Not sure if this is a great feature as in pure css `visibility: hidden` still takes the space. The same for `UIView` on iOS. See 26bcc1e which explicitly reverts this from css-layout. To not confuse users, maybe its better to not name this something like `visible`. Maybe it should be called something like the css `display:none` which leads to the desired behaviour.

roxlu commented

2016-11-03 12:43:37 -07:00

(Migrated from github.com)

@emilsjolander are you on irc or skype?

emilsjolander commented

2016-11-08 03:23:38 -08:00

(Migrated from github.com)

@roxlu totally missed this sorry. Feel free to hit me up on fb messenger (m.me/sjolander.emil)

emilsjolander commented

2016-11-08 03:25:10 -08:00

(Migrated from github.com)

@woehrl01 I agree, using visible could be very confusing

@woehrl01 I agree, using `visible` could be very confusing

roxlu commented

2016-12-01 02:14:50 -08:00

(Migrated from github.com)

@emilsjolander I did some experimenting and I think it's almost working. There is only one issue that needs to be solved: when you hide a node before calling layout() it will never be shown again. When you hide after calling layout() it works as expected.

I've pasted some test code using my fork: https://gist.github.com/roxlu/5d489941de7956c808823f85fd71ab49

It seems that things are cached. @emilsjolander shall we have a call to look into this?

@emilsjolander I did some experimenting and I think it's almost working. There is only one issue that needs to be solved: when you hide a node before calling layout() it will never be shown again. When you hide after calling layout() it works as expected. I've pasted some test code using my fork: https://gist.github.com/roxlu/5d489941de7956c808823f85fd71ab49 It seems that things are cached. @emilsjolander shall we have a call to look into this?

emilsjolander commented

2016-12-01 02:18:12 -08:00

(Migrated from github.com)

@roxlu I would suggest stepping through the code with a debugger and figuring out why your hidden node is not being shown when hiding it before calling layout()

roxlu commented

2016-12-01 02:20:30 -08:00

(Migrated from github.com)

@emilsjolander thanks for your reply, but currently I don't have time to dive into the code. It would take me some time to be able to understand the code in detail. Though ... maybe you have some ideas where to look?

emilsjolander commented

2016-12-01 02:22:34 -08:00

(Migrated from github.com)

@roxlu If it is using cached measurements you can have a look at the CSSNodeCanUseCachedMeasurement function. Perhaps make it always return false to see if this is the problem.

@roxlu If it is using cached measurements you can have a look at the `CSSNodeCanUseCachedMeasurement` function. Perhaps make it always return `false` to see if this is the problem.

roxlu commented

2016-12-01 02:43:27 -08:00

(Migrated from github.com)

@emilsjolander I just tested with a CSSNodeCanUseCacheMeasurement returning always false and this gives me these results: https://gist.github.com/roxlu/db899e390deda9e06445d2759a482fa1

Interestingly it seems that the version A and B works, but then at step C where I hide the first 5 items (see paste) it is still using the values after that were calculated at step B.

@emilsjolander I just tested with a `CSSNodeCanUseCacheMeasurement` returning always false and this gives me these results: https://gist.github.com/roxlu/db899e390deda9e06445d2759a482fa1 Interestingly it seems that the version A and B works, but then at step C where I hide the first 5 items (see paste) it is still using the values after that were calculated at step B.

emilsjolander commented

2016-12-01 02:49:49 -08:00

(Migrated from github.com)

Sounds like you may just not be reseting the nodes output in between
On Thu, 1 Dec 2016 at 10:43, @roxlu notifications@github.com wrote:

@emilsjolander https://github.com/emilsjolander I just tested with a
CSSNodeCanUseCacheMeasurement returning always false and this gives me
these results:
https://gist.github.com/roxlu/db899e390deda9e06445d2759a482fa1

Interestingly it seems that the version A and B works, but then at step C
where I hide the first 5 items (see paste) it is still using the values
after that were calculated at step B.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/facebook/css-layout/issues/241#issuecomment-264139131,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABdIpCcSZ2UceilxEF9SxGXwrAiO3997ks5rDqTQgaJpZM4Kojle
.

Sounds like you may just not be reseting the nodes output in between On Thu, 1 Dec 2016 at 10:43, @roxlu <notifications@github.com> wrote: > @emilsjolander <https://github.com/emilsjolander> I just tested with a > CSSNodeCanUseCacheMeasurement returning always false and this gives me > these results: > https://gist.github.com/roxlu/db899e390deda9e06445d2759a482fa1 > > Interestingly it seems that the version A and B works, but then at step C > where I hide the first 5 items (see paste) it is still using the values > after that were calculated at step B. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <https://github.com/facebook/css-layout/issues/241#issuecomment-264139131>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ABdIpCcSZ2UceilxEF9SxGXwrAiO3997ks5rDqTQgaJpZM4Kojle> > . >

roxlu commented

2016-12-01 02:56:53 -08:00

(Migrated from github.com)

I've set gPrintTree / gPrintChanges to true, it that what you mean?

roxlu commented

2016-12-01 03:00:52 -08:00

(Migrated from github.com)

@emilsjolander Do I need to make a parent dirty when one of it's children changes visibility? Maybe the root not (and so all children) is just skipped when I only mark a child as dirty (?).

emilsjolander commented

2016-12-01 03:02:05 -08:00

(Migrated from github.com)

Yes you do, the dirty marking function does this for you though
On Thu, 1 Dec 2016 at 11:00, @roxlu notifications@github.com wrote:

@emilsjolander https://github.com/emilsjolander Do I need to make a
parent as dirty when one of it's children changes visibility? Maybe the
root not (and so all children) is just skipped when I only mark a child as
dirty (?).

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/facebook/css-layout/issues/241#issuecomment-264142656,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABdIpPufEC6VqBQEXZcccetkB48eecFHks5rDqjlgaJpZM4Kojle
.

Yes you do, the dirty marking function does this for you though On Thu, 1 Dec 2016 at 11:00, @roxlu <notifications@github.com> wrote: > @emilsjolander <https://github.com/emilsjolander> Do I need to make a > parent as dirty when one of it's children changes visibility? Maybe the > root not (and so all children) is just skipped when I only mark a child as > dirty (?). > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <https://github.com/facebook/css-layout/issues/241#issuecomment-264142656>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ABdIpPufEC6VqBQEXZcccetkB48eecFHks5rDqjlgaJpZM4Kojle> > . >

emilsjolander commented

2016-12-01 03:11:48 -08:00

(Migrated from github.com)

What I meant by not reseting nodes is that perhaps the nodes are just skipped over in the last pass (as expected) so that their layout outputs (width/height/x/y) are never reset and they just keep the ones from before. I don't have time now to dig down into your code :/

roxlu commented

2016-12-01 03:14:39 -08:00

(Migrated from github.com)

It's okay if the width/height/x/y stay the same as before when hiding a element. It seems that the issue is related to the dirty flag. When I call CSSNodeHide() or CSSNodeShow() on a child, the parent is not marked as dirty. See here for these functions.

Also, when I set needToVisitNode = true in layoutNodeInternal(), it seems to give me the expected results. But this needToVisitNode is set to false because node->isDirty is false.

It's okay if the width/height/x/y stay the same as before when hiding a element. It seems that the issue is related to the dirty flag. When I call `CSSNodeHide()` or `CSSNodeShow()` on a child, the parent is not marked as dirty. [See here for these functions](https://github.com/roxlu/css-layout/blob/feature-hidden-nodes/CSSLayout/CSSLayout.c#L292). Also, when I set `needToVisitNode = true` in `layoutNodeInternal()`, it seems to give me the expected results. But this `needToVisitNode` is set to false because `node->isDirty` is false.

emilsjolander commented

2016-12-01 03:17:04 -08:00

(Migrated from github.com)

Look at the definition of _CSSNodeMarkDirty, it's a recursive function calling up to the parent so I don't this this is your issue.

Look at the definition of `_CSSNodeMarkDirty`, it's a recursive function calling up to the parent so I don't this this is your issue.

roxlu commented

2016-12-01 03:21:14 -08:00

(Migrated from github.com)

I agree :) that it seems wrong that it's the dirty flag. I'm printing out the condition results in layoutNodeInternal() that set the needToVisitNode, this is what I get:

node->isDirty: n
layout->generationCount != gCurrentGenerationCount: y
layout->lastParentDirection != parentDirection: n

And the code I'm using for this:

  printf("---\n");
  printf("node->isDirty: %c\n", (node->isDirty) ? 'y' : 'n');
  printf("layout->generationCount != gCurrentGenerationCount: %c\n", layout->generationCount != gCurrentGenerationCount ? 'y' : 'n');
  printf("layout->lastParentDirection != parentDirection: %c\n", layout->lastParentDirection != parentDirection ? 'y' : 'n');

When looking at gCurrentGenerationCount it makes me wonder if this could be caused by multiple "root" instances?

I agree :) that it seems wrong that it's the dirty flag. I'm printing out the condition results in `layoutNodeInternal()` that set the `needToVisitNode`, this is what I get: ```` node->isDirty: n layout->generationCount != gCurrentGenerationCount: y layout->lastParentDirection != parentDirection: n ```` And the code I'm using for this: ````c printf("---\n"); printf("node->isDirty: %c\n", (node->isDirty) ? 'y' : 'n'); printf("layout->generationCount != gCurrentGenerationCount: %c\n", layout->generationCount != gCurrentGenerationCount ? 'y' : 'n'); printf("layout->lastParentDirection != parentDirection: %c\n", layout->lastParentDirection != parentDirection ? 'y' : 'n'); ```` When looking at `gCurrentGenerationCount` it makes me wonder if this could be caused by multiple "root" instances?

emilsjolander commented

2016-12-01 03:24:07 -08:00

(Migrated from github.com)

@roxlu No idea. As I said, step through the code (mainly where you made additions and it does cache / dirty checks) and see what is going on. Try it on the most minimal tree to make it easier.

roxlu commented

2016-12-01 03:26:45 -08:00

(Migrated from github.com)

"step through the code" would be okay if I knew the internals and I had time to learn the codebase which I don't have atm. I'll revisit this issue again when I've got some more time or when it's a bit on your radar too.

roxlu commented

2016-12-01 04:08:55 -08:00

(Migrated from github.com)

@emilsjolander maybe I found what was causing it, but please tell me if this sounds reasonable. I noticed that in _CSSNodeMarkDirty() you only traverse upwards when the given node is not dirty. I guess this isDirty is set to false somewhere; but this 'somewhere' is not doing that because the node was hidden. I can fix this by settings node->isDirty = false before calling _CSSNodeMarkDirty(node) in my CSSNodeHide() and CSSNodeShow() functions. It seems to work fine now.

Thanks for your feedback before!

@emilsjolander maybe I found what was causing it, but please tell me if this sounds reasonable. I noticed that in `_CSSNodeMarkDirty()` you only traverse upwards when the given node is not dirty. I guess this `isDirty` is set to false somewhere; but this 'somewhere' is not doing that because the node was hidden. I can fix this by settings `node->isDirty = false` before calling `_CSSNodeMarkDirty(node)` in my `CSSNodeHide()` and `CSSNodeShow()` functions. It seems to work fine now. Thanks for your feedback before!

woehrl01 commented

2016-12-01 07:28:20 -08:00

(Migrated from github.com)

@roxlu you need to mark the layout as seen (isDirty = false), even so you skip the child.

e.g. here and here and the other spots where you skip them.

@roxlu you need to mark the layout as seen (```isDirty = false```), even so you skip the child. e.g. [here](https://github.com/roxlu/css-layout/blob/38ae7ea4ec31faa9582e452ad32818687a32743e/CSSLayout/CSSLayout.c#L1370) and [here](https://github.com/roxlu/css-layout/blob/38ae7ea4ec31faa9582e452ad32818687a32743e/CSSLayout/CSSLayout.c#L1443) and the other spots where you skip them.

woehrl01 commented

2016-12-01 07:34:38 -08:00

(Migrated from github.com)

@roxlu furthermore, I'd suggest changing boolean isVisible to an enum display. With flex and none as values, where flex is the default. So that it conforms more with the web.

e.g. CSSNodeStyleSetDisplay(node, CSSDisplayFlex) and CSSNodeStyleSetDisplay(node, CSSDisplayNone)

This would simplify test generation.

@roxlu furthermore, I'd suggest changing boolean ```isVisible``` to an enum ```display```. With ```flex``` and ```none``` as values, where ```flex``` is the default. So that it conforms more with the web. e.g. ```CSSNodeStyleSetDisplay(node, CSSDisplayFlex)``` and ```CSSNodeStyleSetDisplay(node, CSSDisplayNone)``` This would simplify test generation.

emilsjolander commented

2016-12-01 08:01:07 -08:00

(Migrated from github.com)

@woehrl01 I like it

woehrl01 commented

2016-12-23 14:32:08 -08:00

(Migrated from github.com)

@roxlu for a feature I try to implement externally this would be a handy thing, do you mind doing a PR ?

roxlu commented

2016-12-23 14:36:11 -08:00

(Migrated from github.com)

Yep of course. Though I would love to see this being integrated here. @emilsjolander shall I do a pull request?