Infinite recursion in IsLayoutDirty()

[mikke89]

Infinite recursion in IsLayoutDirty()

[viciious]

Might be relevant:

RMLUI_ASSERT(!parent || !_parent)
.../libRocket/Source/Core/Element.cpp:1913

[viciious]

Same thing in Element::GetStyleSheet():

const SharedPtr<StyleSheet>& Element::GetStyleSheet() const
{
	if (ElementDocument * document = GetOwnerDocument())
		return document->GetStyleSheet();

[mikke89]

Hmm, that's strange, thanks for reporting.

Looks like the owner_document is set to an Element that is not an ElementDocument. I'm out traveling now, but will have a closer look at it once I have some more time.

[viciious]

Yeah, I think culprit here is that I'm attaching a document to an "iframe" element, which a regular ekement and that worked in libRocket but not in RmlUi.

[mikke89]

Ah, yeah, I'm not sure how that would work out.

So this document under the iframe is an ElementDocument, could the document be replaced by a normal Element? Can I ask what you are using this for? :)

[viciious]

I'm using this to create embedded iframes - basically, a document within another document. Each document has its own set of scripts, listeners and styles attached so that makes it impossible to replace them with normal Elements.

By nesting documents you are also able to pass events between them.

[mikke89]

Sounds good! I'll have a closer look at it.

[mikke89]

I don't see how an Element can be set as an owner_document. Are you setting this manually somehow?

How do you create and attach the document?

[viciious]

I don't see how an Element can be set as an owner_document. Are you setting this manually somehow?

How do you create and attach the document?

Here's the code (commented out for now):
https://github.com/Qfusion/qfusion/blob/librml/source/ui/widgets/ui_iframe.cpp#L115

[mikke89]

I cannot reproduce this issue. I do spot a potential issue with the commented out code though. You are wrapping a non-owning raw pointer in a new smart pointer, which will lead to a double delete at some point (memory corruption), and other potential issues wrt. ownership.

Can you try replacing the commented out code with the following:

assert(rocket_document->GetParentNode() != nullptr);
AppendChild( rocket_document->GetParentNode()->RemoveChild(rocket_document) );

Assuming the document is still attached to the root (as after loading the document).

There may be other issues lurking though, but I cannot see the owner document ever getting changed in an ElementDocument. In my own testing I'm getting a different assertion failure, not sure if it is serious but I will investigate further.

[viciious]

Doh, you're right - that was indeed an oversight of mine. Changing the code to what you suggested fixed the crash, however I can't get the child document to get positioned properly: its position is always seems to be absolute no matter what, even after tweaking the line in ElementDocument() constructor from:
SetProperty(PropertyId::Position, Property(Style::Position::Absolute));
to
SetProperty(PropertyId::Position, Property(Style::Position::Relative));

The focus is also messed up. Manually calling DirtyPosition() didn't do anything.

[viciious]

Finally decided to try the Debugger plugin and I must say that I'm pretty damn impressed. Here's what it has to say about the inlined document and its parent iframe "profile_container":

[viciious]

Just for reference, here's how it is supposed to actually look (the inlined document should be positioned at the top left corner of the ancestor's box):

[mikke89]

Can you try this change:

40e04b9

There is one other issue I've recognized. The layout needs to be updated manually for the iframe document. Did you have to do this before?

[mikke89]

Forgot to mention, in addition you still have to set the position property manually, or set the parent to position: relative.

Okay, I think I understand the need for manual layouting. The dirty layout is set on the nearest document, instead, it needs to be set on the outer document. I think it's easier to just do it manually rather than work around it inside the core library.

[viciious]

Hm, it has always worked for me without manual positioning or dirtying of the parent document. The possible reason being that for the iframe element, i'm doing the following:

		SetProperty( "display", "inline-block" );
		SetProperty( "overflow", "auto" );

[viciious]

But yes, that commit has resolved the issue for me, thanks!

[mikke89]

Good to hear, no problem!

I think the best solution for iframe is to create a custom IframeElement, which is just a simple replaced element. Then, have the ElementDocument attached to the Context as normal, and draw it in the position of the iframe. I'd rather spend my time on other priorities now though, but something to consider if we want to support iframe natively.

[viciious]

Good to hear, no problem!

I think the best solution for iframe is to create a custom IframeElement, which is just a simple replaced element. Then, have the ElementDocument attached to the Context as normal, and draw it in the position of the iframe. I'd rather spend my time on other priorities now though, but something to consider if we want to support iframe natively.

If I'm getting this right, it isn't quite the same thing, as you'll end up with missing scrollbars in case the inline document overflows its parent iframe.

[mikke89]

It would work like the html iframe element, we would need width and height defined on the iframe, which we would also use on the iframe body. And probably overflow: auto by default, which should give the same behavior?

We wouldn't be able to have the iframe scale dynamically based on its content no, if this is the behavior you need. That is, width: auto and height: auto wouldn't make sense.

[viciious]

No-no, that's not what I mean. The way it currently works for me is exactly the way the the html iframe works. And ye, it has fixed width and height set in css, and defaults to overflow: auto and display: inline-block in the code, which in turn causes the attached document to have position set to relative.

https://github.com/Qfusion/qfusion/blob/librml/source/ui/widgets/ui_iframe.cpp#L37

https://github.com/Qfusion/qfusion/blob/librml/source/ui/widgets/ui_iframe.cpp#L113

The iframe also subscribes to show and hide events of the parent and repeats the events for the framed document.

[mikke89]

Oh I see, then the behavior should be the same with how I described. The difference is that the document would not be a child of the iframe, but the context. Then the iframe (or context itself) would be responsible for setting the position and clipping of the document. This way we would avoid any potential issues with nested documents, and also ensure that inherited properties are not inherited "through" the iframe.

But of course, since it works now, I guess no need to spend time on this :)

[viciious]

Yes, the way this works right now is alright by me :)

[mikke89]

Then it's all good :)

[viciious]

Yup, just don't forget to merge the iframe branch into develop ;)

[mikke89]

Yup, I've merged the relevant parts now. Do you feel this is ready to be closed?

[viciious]

Sure! Thanks again!