-
Notifications
You must be signed in to change notification settings - Fork 636
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
Don't instanciate ListView items that are just above the viewport when scrolling down #7424
base: master
Are you sure you want to change the base?
Conversation
Thanks for the patch. For example, consider this: import { AboutSlint, Button, VerticalBox } from "std-widgets.slint";
export component Demo {
height: 300px;
width: 300px;
Flickable {
VerticalBox {
AboutSlint { }
for i in 50 : Text {
text: "Hello " + i;
}
}
}
} Then all the Text are going to be instantiated regardless if they are visible or not. I think it might make sense to not generate the item if it is out even if it is by one pixel. |
I agree, I think we need to perform a visibility check. We do this for the testing backend as well, perhaps accessible_descendants() needs to be changed and perform a visibility check like here ? |
My initial change was indeed very specific. I've implemented a general fix but we're now back to the point where listviews are excluded from the focus chain, even if no scrolling is performed. My understanding is that it is due to this hack used throughout the codebase:
Shouldn't there be another property on |
d024198
to
829ea6f
Compare
internal/core/item_tree.rs
Outdated
&& clip.min.x <= geometry.max.x | ||
&& clip.min.y <= geometry.max.y | ||
&& clip.min.x < geometry.max.x | ||
&& clip.min.y < geometry.max.y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems incorrect to me. For example max.x
is x + width
, so it's basically one pixel outside the rectangle. So <
seems correct to me, while <=
would make the rectangle go from x
to x + width
inclusive.
@ogoffart is my interpretation correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me there still was a bug in this function but on the lines above that I didn't touch previously.
I think the I wonder why this is used. @ogoffart @FloVanGH do you remember why, or how tab focus could be implemented? |
To clarify the coordinates/dimensions point, here is a test that passes on this branch but not on master:
Do we all agree that this test is correct and should pass?
I also think this is the case, but an element with a zero width should not be considered visible right? So the way |
The intent of
Probably better than that hack, yes.
According to the current semantic of
Yes, this is a bit controversial indeed. The question is why do we do visibility check at all? But this had the IMHO unwanted side effect to prevent focus to go to item which are clipped because they are scrolled out of a ScrollView/Flickable. This is outside of the scope of this PR however. |
@DataTriny We're slightly unsure how to proceed. On the one hand it seems natural to limit the accessibility tree to match what's on the screen, on the other hand it also feels wrong: Say I have a list view that shows 100 contacts, but only 10 are visible: I want to be able to go through all of them via keyboard navigation and I suppose that the accessibility tree for such a view should also contain 100 entries always? Would you have time for a call some time next week? (teams, mattermost, jitsit, - we can organise). Or are you at FOSDEM by chance? @ogoffart will be there, that might be an opportunity to meet in person and clarify. |
a4cb81f
to
5af4d4d
Compare
I have removed all commits except the first one, which should be enough to make progress on list views. I have reached out to both of you via email to setup a call. |
Fixes #7381
If an item can just fit above the viewport while scrolling, it is now not instanciated anymore as it is fully hidden. The issue occurs frequently when using the arrow keys to jump through the items, since the scroll delta will usually be the height of exactly one item. This previously meant that an extra node would be added at the start of
ListView
s accessibility tree.