wsick / Fayde

Inspired by Silverlight; XAML engine using Javascript and rendering to the HTML5 Canvas.
MIT License
189 stars 27 forks source link

ListBox scroll issue. #268

Open dp901 opened 7 years ago

dp901 commented 7 years ago

When a listbox contains items that are not of type "ListBoxItem" then scrolling causes item selection. There is an issue that demonstrates that (IssueTests/listbox.fap). The above change fixes the problem but I am not very sure if it is the right way to handle it.

BSick7 commented 7 years ago

Good find. Give me some time to dig through this.

dp901 commented 7 years ago

Thanks Brad.

BSick7 commented 7 years ago

Looks like you're checking to see if the current item is a UIElement as to whether to reuse a recycled container.

This doesn't feel right.

I would guess that selection problems would be in proximity to the selection code.

I'm open to being proved wrong with tests.

dp901 commented 7 years ago

I'm struggling to put together a test that demonstrates the problem. The reason I'm checking if the current item is a UIElement before setting it to a recycled item is because the cache is only supposed to contain UIElement items. My listbox does not contain items of ListBoxItem type and this is only when the error occurs. The selection code seems to be working as expected, it's like we are telling it to select an item from list when we shouldn't.

dp901 commented 7 years ago

A few lines above you are also checking for a UIElement before setting generator.Current to a UIElement. if (generator.CurrentItem instanceof UIElement) generator.Current = generator.CurrentItem; If the cache only contains UIElements I don't understand why the UIElement check is not valid in that case.

BSick7 commented 7 years ago

I see where you're headed.

In the following, we are testing to see whether the user's "Item" is already of type UIElement (usually ListBoxItem). If it is, we just use that for the current visual.

                    if (ic.IsItemItsOwnContainer(generator.CurrentItem)) {
                        if (generator.CurrentItem instanceof UIElement)
                            generator.Current = <UIElement>generator.CurrentItem;
                        generator.IsCurrentNew = true;
                    }

The point of the following is to pull a container out of the recycled cache instead of generating a new one. We would want to do this if the current "Item" was not a UIElement. If generator.CurrentItem is a UIElement, the if block would satisfy.

                    } else if (cache.length > 0  && generator.CurrentItem instanceof UIElement) {
                        generator.Current = cache.pop();
                        generator.IsCurrentNew = true;
                    }