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

Absolutely positioned elements do not trigger clipping on parent elements #235

Closed
MatthiasJFM opened this issue Sep 6, 2021 · 10 comments
Closed
Labels
layout Layout engine issues and enhancements

Comments

@MatthiasJFM
Copy link

I noticed that absolutely positioned elements that exit the bounds of their parent element do not trigger clipping when overflow is enabled. In some cases I have used a workaround of having another empty element in that same parent to trigger the overflow, but it's not a nice solution.

Commenting out the
if (clipping_element->GetClientWidth() < clipping_element->GetScrollWidth() - 0.5f || clipping_element->GetClientHeight() < clipping_element->GetScrollHeight() - 0.5f)
In ElementUtilities.cpp allows absolutely positioned elements to be clipped too, but it's not an efficient solution. Only including a clipping region when relevant sounds like a good idea, so the real solution is probably to have absolutely positioned elements properly trigger an overflow when they are out of bounds (which is also the normal HTML/CSS behaviour I believe).

Excluding a repro example, should be easy enough to do with an absolutely positioned div overflowing an overflow: hidden parent.

@mikke89 mikke89 added the layout Layout engine issues and enhancements label Sep 6, 2021
@mikke89
Copy link
Owner

mikke89 commented Sep 6, 2021

Hi and thanks for the suggestion.

This might be a bit tricky in RmlUi, since absolute offseting is done separately and after layouting. Thus, say we have scrollbars appearing due to the absolute positioned box. Then we would need to re-layout, but we only know this after we have performed the layout step. At least this is my understanding right now, I will investigate a bit more to see if it is possible somehow without rewriting the whole layout engine, or in case my understanding is wrong.

Just to be clear, in CSS you would also have to add position: relative to the parent div before overflow: hidden takes effect on the absolutely positioned box. The RmlUi issue is indeed deeper though.

@MatthiasJFM
Copy link
Author

Hi,

I have position: relative set on the parent, and the child is positioning correctly but simply not triggering the overflow and not getting clipped.
I see how the layout ordering is tricky... if it helps, I checked in CSS again and indeed when the parent is set to overflow: auto then an overflowing absolutely positioned element will also trigger the scrollbar.

The layout step proceeds up the DOM tree right? If I understand the code correctly the absolute elements are being linked/assigned to the ancestor that they are being positioned relative to; perhaps you can layout absolute elements as soon as that element is positioned and check for overflow then? Like in LayoutEngine::FormatElementBlock with

// Close the block box, and check the return code; we may have overflowed either this element or our parent. switch (new_block_context_box->Close())

I think this is how overflow is handled for block level elements? Do absolutely positioned elements need to be different?

@mikke89
Copy link
Owner

mikke89 commented Sep 7, 2021

The library is generally written with the assumption that offseting can be done completely separate from layouting. I'm not sure why this was done back in the day, but one very nice benefit is that we can scroll and move things around without invoking any layouting steps at all. You can also see based on StyleSheetSpecification.cpp that we can change top/right/bottom/left properties without forcing a layout step. Solving the issue would essential need to break this assumption. And I'm afraid too much code is written based on that assumption at this point, changing it would probably be a can of new bugs.

However, I do have another suggestion, and I wonder if this could be a suitable way to solve the issue. What if we add an always value to the clip property, which always applies clipping for the given element by essentially bypassing the condition in your original post.

This would still be a workaround in some sense, but at least it is a lot prettier than a large invisible element, what do you think?

@MatthiasJFM
Copy link
Author

I think having clip: always as a workaround trade-off to keep the performance benefits of not forcing a layout step is a good idea, and yes it would be a lot prettier. Plus it's a lot more efficient of a solution compared to commenting out that if as I mentioned in the opening post. It would deviate from CSS so please do add to the documentation :)

@mikke89
Copy link
Owner

mikke89 commented Sep 9, 2021

I went ahead and implemented clip: always. Let me know if it works as intended :)

I had to simultaneously change the clip property to be non-inherited, as it would be very strange to have descendants inherit the always value. In fact, the clip property was already documented as non-inherited. From the uses I've seen so far, this change made no difference in the end, and it's probably a rare property so I think this change should be okay backward compatibility wise.

Updated the documentation as well, our clip property is completely different from the CSS clip property, so this new value fits very nicely with how we use it already.

@MatthiasJFM
Copy link
Author

Works as intended, thank you!

@MatthiasJFM
Copy link
Author

MatthiasJFM commented Nov 2, 2021

Reopening this to point something out: clip always always clips. Even if there is a child element with clip: n;
I think the fix for this is to have
if (clip_always && (num_ignored_clips == 0 && clip_enabled)) instead of
if (clip_always || (num_ignored_clips == 0 && clip_enabled))
on line 197 in ElementUtilities.cpp (ElementUtilities::GetClippingRegion).

I tested the above and that works, let me know if you want me to draft up a quick rml snippet for this case.

Edit: Too quick! That is not a fix because it messes with overflow clipping!

@MatthiasJFM MatthiasJFM reopened this Nov 2, 2021
@MatthiasJFM
Copy link
Author

V2:
if ((clip_always || clip_enabled) && num_ignored_clips == 0 )
Makes more sense too, the clip: always serves to clip when a child exiting bounds is not recognised so we bypass the clip_enabled and the subsequent out-of-bounds check. However if we have clips to ignore - ignore them.

@mikke89
Copy link
Owner

mikke89 commented Nov 2, 2021

Yeah, I wasn't sure when implementing this how or if it should interact with other values. I'm completely open to suggestions and even better pull requests ;) Your latter solution sounds good to me.

@MatthiasJFM
Copy link
Author

Pull request #251 👌

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

No branches or pull requests

2 participants