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

ElementDocument::UpdateLayout Resets Scrollbars #452

Closed
Dakror opened this issue Apr 21, 2023 · 12 comments
Closed

ElementDocument::UpdateLayout Resets Scrollbars #452

Dakror opened this issue Apr 21, 2023 · 12 comments
Labels
bug Something isn't working layout Layout engine issues and enhancements

Comments

@Dakror
Copy link
Contributor

Dakror commented Apr 21, 2023

I have a max-height constrained container inside a tabset panel and scroll progress is always reset to 0 after one context update.
I tested this even with a completely unstyled naked tabset. When commenting out the tabset, scrolling works normally.

@Dakror
Copy link
Contributor Author

Dakror commented Apr 22, 2023

Additionally, im observing some odd behaviour with having my scroll container inside a padded flex box container.

Do we have some sort of infrastructure for a minimal test example? like some Rml-Player to quickly share and evaluate documents? That would be neat for situations like this

@Dakror Dakror changed the title Scroll bars always jump back to the top when inside a tabset panel ElementDocument::UpdateLayout Resets Scrollbars Apr 22, 2023
@Dakror
Copy link
Contributor Author

Dakror commented Apr 22, 2023

I found the real issue, ElementDocument::UpdateLayout resets scrollbar positions in unrelated elements. Building a test case now

@Dakror
Copy link
Contributor Author

Dakror commented Apr 22, 2023

Here is a minimal example, run it for instance using the player i made in #453 . The issue only seems to occur with scroll containers inside of flex boxes. Additionally the error is different depending on if the list is in a tabset or not.
Left side is tabset: hovering the button jumps the scrollbar to the top
Right side is flex only: hovering the button resets the scrollbar from the very bottom back up the amount of padding on the container.

<rml>

    <head>
        <style>
            * {
                box-sizing: border-box;
            }

            body {
                font-family: LatoLatin;
            }

            scrollbarvertical {
                width: 15px;
            }

            scrollbarvertical sliderbar {
                width: 15px;
                background-color: white;
            }

            .outer {
                display: flex;
                width: 100vw;
                height: 200px;
            }

            .container {
                width: 50%;
                display: flex;
                flex-direction: column;
                padding: 15px;
                border: 1px white;
            }

            .list {
                display: block;
                height: 100%;
                overflow-y: auto;
            }

            .list>div {
                background-color: blue;
                display: block;
            }

            p {
                line-height: 40px;
            }

            button {
                display: block;
                padding: 10px;
                background-color: red;
            }

            button:hover {
                padding: 20px;
            }
        </style>
    </head>

    <body>
        <div class="outer">
            <div class="container">
                <tabset>
                    <tab>tab</tab>
                    <panel>
                        <div class="list">
                            <div>
                                <p>elem1</p>
                            </div>
                            <div>
                                <p>elem2</p>
                            </div>
                            <div>
                                <p>elem3</p>
                            </div>
                            <div>
                                <p>elem4</p>
                            </div>
                            <div>
                                <p>elem5</p>
                            </div>
                            <div>
                                <p>elem6</p>
                            </div>
                        </div>
                    </panel>
                </tabset>
            </div>
            <div class="container">
                <div class="list">
                    <div>
                        <p>elem1</p>
                    </div>
                    <div>
                        <p>elem2</p>
                    </div>
                    <div>
                        <p>elem3</p>
                    </div>
                    <div>
                        <p>elem4</p>
                    </div>
                    <div>
                        <p>elem5</p>
                    </div>
                    <div>
                        <p>elem6</p>
                    </div>
                </div>
            </div>
        </div>
        <button>hover me</button>
    </body>
</rml>

@mikke89 mikke89 added bug Something isn't working layout Layout engine issues and enhancements labels Apr 26, 2023
@mikke89
Copy link
Owner

mikke89 commented Apr 26, 2023

Thanks for the detailed report, I can easily reproduce the issue with it. This one might be quite tricky, I'll take a closer look at it at some point.

mikke89 added a commit that referenced this issue Apr 26, 2023
Co-authored-by: Dakror <mail@dakror.de>
@ShawnCZek
Copy link
Contributor

I have run into the same issue. I just want to highlight that it indeed seems to happen only within a flexbox container which is an incredibly great find. Working around that and working with CSS alternatives (temporarily) fixed the scrollbar being reset to 0.

@thecozies
Copy link

I want to add to this that this occurs whether or not any parent is a flex box, not just the immediate parent. I have a layout heavily dependent on flex positioning and this is causing a lot of issues for me and can't find a workaround without a hefty redesign. Has any progress been made on this issue?

@thecozies
Copy link

I tried looking into this issue by adding an id to the child element of a scroll-able div. I found that in SetScrollableOverflowRectangle, scroll_offset.y is getting set to 0 due to GetScrollHeight and GetClientHeight returning the same value - more importantly GetClientHeight being larger than it should be. This isn't a real fix, but I was able to prevent scroll resets by checking if those weren't equal:

		if (GetScrollHeight() != GetClientHeight())
		{
			scroll_offset.y = Math::Min(scroll_offset.y, GetScrollHeight() - GetClientHeight());
		}

So it may be that GetClientHeight() is incorrect before the end of the doc update. I'm unsure of anything else at this point.
Hopefully this information can help get a lead on the issue!

@mikke89
Copy link
Owner

mikke89 commented Jul 9, 2024

Thanks for reporting in. I haven't actually been able to prioritize this one yet unfortunately. I think I have a vague understanding of it, but I'm not sure exactly how to approach it. It might need some architectural improvements.

My theory of what is happening, is that the layout engine always starts out with the assumption that scrollbars are not necessary. Then, once it starts doing layouting, it is updating sizes based on that assumption. In some cases like those reported here, it is effectively zeroing out the scroll offset since the sizes are too small to allow scrolling. Then, once it later knows it needs scrollbars, things are layed out again, this time ending up in the right sizes. However, since the scroll offset was already zeroed out in the previous step, it can no longer get back to the original offset.

Indeed, as you discovered, we can effectively work around this by removing the scroll offset clamping in the SetScrollableOverflowRectangle function. We should find something better as a permanent solution though.

@thecozies
Copy link

Yeah, that makes sense. One thought I had is that it could make sense to cache the scroll position until updates are complete, so that it can be reset on the final update. I don't fully understand the update loop so you'd probably have a better idea than myself what's best here.
Would you like me to PR removing the clamping as a stop-gap solution? Or would you rather wait for a permanent solution?

@mikke89
Copy link
Owner

mikke89 commented Jul 9, 2024

Would you like me to PR removing the clamping as a stop-gap solution? Or would you rather wait for a permanent solution?

I am pretty sure this will cause other issues, so we should look for another solution.

I did have one idea though, to simply wait with clamping until all the layouting is one, and then clamp at the end. I implemented it a bit quickly here, do you want to test this one: 5a2d1cf
At least it seems to work with the visual test we have for this issue.

I am not fully sure about all the implications, but it could be a possible fix, and it is quite simple too.

@thecozies
Copy link

Just tested it! That works perfectly, no issues on my end in any of the scroll areas that were having this issue before. I have an area that has dynamic elements which get swapped with a toggle, and the scroll position was retained even though every child completely changed, so this seems very robust!

@mikke89
Copy link
Owner

mikke89 commented Jul 12, 2024

Great to hear! I want to test it a bit more before I merge to master, but that seems promising.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working layout Layout engine issues and enhancements
Projects
None yet
Development

No branches or pull requests

4 participants