Text opacity bug
Fix for #3606 (closed) and #3914
Tagging @doctormo for reviewing and merging this MR.
Tale of an interesting bug
When you read the issue #3606 (closed) for the first time, the first thing that comes to your mind is, the problem must be with the opacity spin button, but after investing time to it and playing out here and there, you get to know that the spin button is absolutely correct as it is working in other cases, so the issue might be with the text. But that's not correct too, because text's opacity can be changed using selection tool, and this issue arises when you click the text tool. Ok, so there must be a logic written out there that when text tool is active you commit this issue. But my friend, opacity spin button is independent of the active tool. Ah, that frustration, this mighty bug, your ego has been hurt. One minute, what if there is some problem setting the opacity style attribute while setting the css style. Let's figure it out, and yes, to my surprise, here the bug lies.
In sp_desktop_apply_css_recursive(...), you see this logic been written at the end,
for (auto& child: o->children) {
if (sp_repr_css_property(css, "opacity", nullptr) != nullptr) {
// Unset properties which are accumulating and thus should not be set recursively.
// For example, setting opacity 0.5 on a group recursively would result in the visible opacity of 0.25 for an item in the group.
SPCSSAttr *css_recurse = sp_repr_css_attr_new();
sp_repr_css_merge(css_recurse, css);
sp_repr_css_set_property(css_recurse, "opacity", nullptr);
sp_desktop_apply_css_recursive(&child, css_recurse, skip_lines);
sp_repr_css_attr_unref(css_recurse);
} else {
sp_desktop_apply_css_recursive(&child, css, skip_lines);
}
}
and this is where the issue lies. Explaination of the above old logic : This for loops picks up the selected object and iterates through its children, if the children has an already opacity being set then it makes that opacity 1 and continues. By doing this opacity attribute is being prevented to get accumulated and is thus not being set recursively. But we wrote this logic for say a group (see example in comment). We didn't knew that text is not a standalone object, it has its own children.
Text may contain nested tspan elements, meaning it has a slight hierarchy. The function sees a child object inside the text and removes the opacity property before applying the style. This prevents the opacity from updating correctly in the UI.
You don't believe me, huh, see an example,
<text id="text1">
<tspan id="tspan1">
"word"
So, this is why opacity spin button works absolutely fine when the selected object is say a rectangle. But does not work fine when the selected object is a text.