Should the LayoutManager fire "layout:after" and bubble up if layout was skipped?

[jiayihu]

fabric.js/src/LayoutManager/LayoutManager.ts

Lines 291 to 329 in 9ec5a3d

	protected onAfterLayout(
	context: StrictLayoutContext,
	layoutResult?: LayoutResult
	) {
	const {
	target,
	strategy,
	bubbles,
	prevStrategy: _,
	...bubblingContext
	} = context;
	const { canvas } = target;
	// fire layout event (event will fire only for layouts after initialization layout)
	target.fire('layout:after', {
	context,
	result: layoutResult,
	});
	canvas &&
	canvas.fire('object:layout:after', {
	context,
	result: layoutResult,
	target,
	});
	// bubble
	const parent = target.parent;
	if (bubbles && parent?.layoutManager) {
	// add target to context#path
	(bubblingContext.path || (bubblingContext.path = [])).push(target);
	// all parents should invalidate their layout
	parent.layoutManager.performLayout({
	...bubblingContext,
	target: parent,
	});
	}
	target.set('dirty', true);
	}

As we can see, layout:after is fired and parent.layoutManager.performLayout is called even if layoutResult is undefined. Notably, because of bubbling the ActiveSelection is uselessly recalculating the layout of a selection if any layout event (e.g. added) happened in the selected tree, even though it resulted in no layout change.

I wonder if fabric should be noop if there is no layoutResult or this can be left to be decided by the app by extending LayoutManager if not desired.

[ShaMan123]

Good point!
I would argue that the events should fire, the dev can check if the result exists to determine what happened.
Regarding bubbling I am not sure... maybe it should bubble but the layout manager should skip it with a similar check.
Thoughts?

[jiayihu]

maybe it should bubble but the layout manager should skip it with a similar check

Why bubbling at all then

[asturur]

I don't think the case of object:added is a dangerous one. How often does it happen?

Which are the cases in which this happen often?

[jiayihu]

It was just an example, you can have many events that trigger a noop layout, such as modified typically.

In our specific case, when you select shapes we create a text inside each shape. It doesn't affect the shape layout so we skip it in shouldPerformLayout. But unfortunately itis still bubbled up and every shape.add(text) results in a new ActiveSelection performLayout, not even a single debounced.
So if you select 50 shapes, it's gonna do 50 performLayouts despite the skipped shouldPerformLayout

[asturur]

so the bubbling is safeguarded by bubbles boolean, that is modified by the stopPropagation function that is available on the context. Is there a way to use that to your advantage?
This seems the perfect case in which either you can use that to stop bubbling or we should hardcode the bubbling stop on non performed layouts but remove the stopPropagation function that is not helping

[jiayihu]

I'm indeed calling stopPropagation(), just wondered if that should be done by default in fabric for instance if there is no layout change

[ShaMan123]

I think I am becoming convinced that @jiayihu is correct not to bubble a layout that did not change the bbox (but might have changed visuals)

[asturur]

the set dirty of the group is going to invalidate all the caches anyway, so visual changes are not an issue.
I m also for removing stopPropagation at that point if we don't have a need for it