When objects are combined their drag handles sometimes appear behind other cards

[marlitas]

The z-order of a combined object needs to be pushed to the front on drag ended. This is probably related to the object being dropped into, not the object being dragged.

This is seen in both NumberCompare and NumberPlay

[marlitas]

This is fixed, but I don't fully understand why I needed two moveToFronts(), ( without both the z-orders of objects do not match in the play area when switching between linked and grouped modes ).

If it looks good to @chrisklus feel free to close, but would definitely appreciate a short explanation to fill in my logic gap. Thanks!

[chrisklus]

Thanks @marlitas! I have yet to dig in and review but just noticed this is maybe causing a bug on the compare screen where you can't select different object types after combining two counting objects.

[samreid]

Should this be "blocks-sim-publication" (only blocks publication for this sim), or "blocks-publication" (blocks publication for all sims)? Based on the remarks above, it seems sim-specific, so I will update the label. Please correct me as necessary.

[chrisklus]

Thanks @samreid, that's correct. Must have missed when trying to add status:blocks-sim-publication.

[chrisklus]

@marlitas and I were pairing a ways back on this issue. We found two cases that needed to be fixed: (1) layering when dropping a card that combines with another (and the result can end up behind a neighbor), and (2) serializing/deserializing the linked play area.

For the linked play area case, we came up with an approach that tracked the z-order of the cards in the model. These would listen to view position and then update their model to match. When time to serialize/deserialize, they would use their model Property to recreate themselves in the right order. While this would work, we thought it was weird to have a model Property listening to a view position. So I consulted with @jonathanolson to see what he thought, and he recommended changing this pattern to have the model z-index be the ground truth, and the view reorder itself based on the model. This would mean no longer using moveToFront() on view nodes when they are grabbed, for example, but calling a model function instead. When I was trying to implement this, I found that it wouldn't work with our current Lab screen pattern because the Lab screen combines many play areas into one. Meaning, there is a play area for each type of object on the lab screen, and they all "play nice" together as one apparent play area because all of the object nodes live in one shared node layer. If we moved the z-ordering to the model, the play areas would no longer work together in the view. This work will have to come after we change the Lab screen to just have one play area that supports multiple play object types in it.

For the dropping case, I still need to investigate to see if this is an easy fix. The current fix that is solving that is in a bit too general of a place (in updateNumber, which is called whenever an object changes number or object type or grouping state, and we just want when an object is added to another).

[KatieWoe]

For https://github.com/phetsims/qa/issues/917, would you say this is the same issue, or a new one?

[Nancy-Salpepi]

Related to https://github.com/phetsims/number-suite-common/issues/44#issuecomment-1470916432, If 2 ten frames are stacked on top of each other, it appears as though it is possible to stack different objects on top of one another.

https://user-images.githubusercontent.com/87318828/225961315-c695e254-2f15-4386-a67f-384d74093d6e.mp4

[chrisklus]

@marlitas and I reviewed the state of this issue together.

We first discussed how (2) can't be fixed with a model-driven approach as described in https://github.com/phetsims/number-suite-common/issues/44#issuecomment-1462310615.

Then we mentioned that we need to look back into the best way to fix (1).

Then, we talked about the problems that @KatieWoe and @Nancy-Salpepi demonstrated on the Lab Screen in their comments above (let's label this one (3). We think that they are the same issue, and will be a separate fix from (1) but are conceptually the same as (1) because the "receiving" node (either the counting object that is being added to or the ten frame that is being added to) is not moving to front when something is dropped on it.

Here we go!

[chrisklus]

In the above commit, @marlitas and I took care of (3). Still to do for this issue: (1).

[chrisklus]

@samreid @zepumph and I took care of (1) from above, thanks!

[chrisklus]

@zepumph @samreid and I also figured out a way to fix (2)! We did not change the z-index ordering to be powered by the model, but instead moved the serialization function of the countingObjects to the view. This allowed us to capture the state of the z-indexes without breaking model -> view ordering. Thanks!

[chrisklus]

I believe all z-index problems for this issue have been fixed. @Nancy-Salpepi or @KatieWoe can you please test these fixes on phettest? Problems 1 and 2 two are briefly described in https://github.com/phetsims/number-suite-common/issues/44#issuecomment-1462310615 and 3 was labeled in https://github.com/phetsims/number-suite-common/issues/44#issuecomment-1476792024. It might be easiest to discuss 2 or any clarifications needed over Zoom, let me know (also, 2 is just Number Play). Thanks!

[Nancy-Salpepi]

@chrisklus 1 and 3 look good in both Number Play and Number Compare. Let me know on slack when we can zoom to discuss 2. 🙂

[chrisklus]

@Nancy-Salpepi thanks! can you meet around 2:30 MT today?

[Nancy-Salpepi]

@chrisklus I can! Then me know where 🙂

[chrisklus]

Awesome thanks! Will Slack you

[chrisklus]

@marlitas and I found a bug from changes to fix (3), where when you click the return button, the removed object immediately goes behind the tenFrame instead of staying in front as it goes to its home. We fixed this in the above commit.

[chrisklus]

We created yet another bug from the last bug fix above that we just committed a fix for and we really hope we didn't make anymore problems. We think the ten frames could use a nice little re-test after @Nancy-Salpepi and I meet to make sure all is good after all these changes.

[chrisklus]

@Nancy-Salpepi and I are meeting about this at 9:30 tomorrow

[Nancy-Salpepi]

(1) still looks good to me. (2) looks good as well. (3) looks good except for this https://github.com/phetsims/number-compare/issues/30#issuecomment-1483221122, but I don't think that is something that will be changed.

Closing. 🤞🏻I didn't miss anything.