Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"nth-last-child" is getting broken (not applying on the elements) after you scroll with scrollbar thumb. #9997

Closed
maxkatz6 opened this issue Jan 17, 2023 · 6 comments · Fixed by #10055

Comments

@maxkatz6
Copy link
Member

Describe the bug

"nth-last-child" is getting broken (not applying on the elements) after you scroll with scrollbar thumb.

To Reproduce
Steps to reproduce the behavior:

  1. ListBox page, control catalog
  2. Scroll with scrollbar thumb.
  3. See error.
  4. Scroll with a wheel.
  5. Error is gone.

Desktop (please complete the following information):

demo.mp4
@maxkatz6 maxkatz6 added the bug label Jan 17, 2023
@robloo
Copy link
Contributor

robloo commented Jan 19, 2023

This issue is more than just the nth-last-child (although that one never works). It also isn't applying all the nth-child selectors on extremely fast scrolling.

In the screenshot:

  1. Scroll very fast to the bottom of the ListBox in ControlCatalog (at bf85f4d)
  2. Notice the blue foreground brush is never applied
  3. Scroll slowly using mouse wheel
  4. Blue foreground brush is applied immediately

cap

@maxkatz6
Copy link
Member Author

This issue is more than just the nth-last-child (although that one never works)

Surprised it never worked for you. I don't remember having any issues before with old items presenter.

@robloo
Copy link
Contributor

robloo commented Jan 19, 2023

This issue is more than just the nth-last-child (although that one never works)

Surprised it never worked for you. I don't remember having any issues before with old items presenter

I should have been more clear: I meant the same as you said. It was only broken in the items presenter refactoring that was just merged. It worked before but now never works (even in the screen capture above).

@grokys
Copy link
Member

grokys commented Jan 20, 2023

This is caused because NthChildActivator reevaluates all matches when the selector is reversed.

ItemsControl has a bit of a hack in it which records the container currently being prepared when calling ChildIndexChanged. This is to work around the fact that during a measure pass, it's hard to say what a control's index is (as it may be subsequently recycled and reused in that same measure pass).

What happens is:

  • A container (let's call it index n) is realized in the ItemsControl
  • ChildIndexChanged is called for that container
  • NthChildActivator calls IChildIndexProvider.GetChildIndex and the "hack" above takes effect
  • Index n matches the nth-last-child selector, style is activated
  • Container n+1 is realized
  • ChildIndexChanged is called again
  • Because the selector is reversed, it is again evaluated on index n as well as n+1
  • NthChildActivator for index n calls IChildIndexProvider.GetChildIndex but this time the hack doesn't take effect as it's not the control currently being prepared. The measure pass hasn't finished yet so no containers are considered realized, so GetChildIndex returns -1.
  • The style is deactivated

@grokys
Copy link
Member

grokys commented Jan 20, 2023

There are a few ways to fix this, but I can't help feeling that the IChildIndexProvider API could be improved, as there are a couple of problems I can see:

  • ChildIndexChangedEventArgs can hold the child whose index is changed, but doesn't hold the index - this means that an additional call to GetChildIndex is needed. As described above, knowing the answer to that for arbitrary controls in a virtualizing panel during measure can be difficult, whereas it's known definitively for the control that the event was raised for.
  • There's no separate event for "TotalCount changed", so when using nth-last-child all controls have to be reevaluated when any one of the indexes changes. This could be inefficient when taking about a virtualizing list as indexes change when recycling containers.

@maxkatz6
Copy link
Member Author

@grokys In both cases, I think it was unnecessary api oversimplification from my side. Probably assuming that TotalCount/GetIndex calls are supposed to be cheap O(1).
These suggestions make sense to me.

grokys added a commit that referenced this issue Jan 20, 2023
And a passing test for `nth-child` selector.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants