LibWeb/CSS: Fix crash resolving calc with only percentages

[Jaycadox]

Fixes #3006 , (relates to) #648

From some analysis, it seems as if this issue only occurs with a only percentages in a calc.

• calc(10%) crashes.
• calc(10% + 10%) crashes.
• calc(10% + 0px) doesn't crash.
• calc(10vw) doesn't crash.

The resolve function returns a Length in the general case, but returns a Percentage if only percentages are provided.

As a sanity check, I looked at the code before it was changed (commit before change was here), and it only handled the case of percentages and lengths, as I do in this commit.

The old code being:

Optional<Length> CalculatedStyleValue::resolve_length_percentage(Length::ResolutionContext const& resolution_context, Length const& percentage_basis) const
{
    auto result = m_calculation->resolve(resolution_context, percentage_basis);

    return result.value().visit(
        [&](Length const& length) -> Optional<Length> {
            return length;
        },
        [&](Percentage const& percentage) -> Optional<Length> {
            return percentage_basis.percentage_of(percentage);
        },
        [&](auto const&) -> Optional<Length> {
            return {};
        });
}

Not sure if this is perfect, very open to constructive criticism.

[gmta]

Hi @Jaycadox, thanks for looking into this! Could you also add a test to expose the original issue?

[AtkinsSJ]

This isn't correct. If a percentage gets returned (eg, 35%) it'll return that number as a length (35px).

A more correct thing to do would be to return the percentage applied to the percentage_basis in that case.

The question is why this isn't getting converted to a length earlier on. NumericCalculationNode::resolve() should be doing that.

[Jaycadox]

From my analysis, I believe it would actually return the correct resolved pixel value (as percentage_basis was being called on it in NumericCalculationNode::resolve), the issue was purely that it was being reported as the wrong type, and I do think that my original patch, did fix the specific issue, but agree that it didn't fix the underlying problem.

[Jaycadox]

I had a look again at the issue, it seems that NumericCalculationNode::resolve, when given a percentage, would convert it to a pixel value (via percentage_of) but not update the CSSNumericType, so it would stay as Percentage. When putting the expression as a sum, SumCalculationNode would call add_the_types, and I think this would see the expression "Percentage" + Length, and set the output type to Length, thus fixing the bug.

In my new patch, I visit all the variants of the percentage basis and set the CSSNumericType explicitly, which fixes the bug, though I'm not sure if it makes sense to allow all of these types to be a percentage basis. Still open for feedback.

As the original code (which I put in my original message) only handles the case of Length and Percentage, and returns an empty Option if those weren't provided, I would think that it would make sense to only allow those two cases, though it would require some more code modification.

[Jaycadox]

I don't know what CSSNumericType should occur in the case of Number, I assume there isn't a set one and it can change. But this code should probably be corrected if there is one.

[AtkinsSJ]

This is actually a duplicate of what's done in NumericCalculationNode::create(), maybe that code can be pulled out and reused.

[Jaycadox]

Thanks for the suggestion 👍

[Jaycadox]

My bad, I believe I have now fixed the linting errors.

[AtkinsSJ]

Thanks, this is looking good. (And I finally understand what I messed up originally!) Just a couple of comments:

1. The commit message should be more descriptive of what the commit does. It's good to mention that it fixes a bug somewhere in the message, but the title itself should be what the code change is. Here, that'd be something like "Recalculate calc() numeric type when resolving percentages" (though you can probably word that better).
2. There are a few VERIFY()s sprinkled about, which seem like they might be from your debugging and testing? It's fine to leave them in but I just want to check they're supposed to be permanent. :^)

So really just the commit message needs adjusting, then this should be good to merge. (And then you won't have to keep waiting for someone to manually run CI. 😅 )

[AtkinsSJ]

Oh also, once a comment is resolved, press the [Resolve conversation] button on it. That makes it clear to reviewers that all the feedback has been dealt with.

[ladybird-bot]

Hello!

One or more of the commit messages in this PR do not match the Ladybird code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

[Jaycadox]

Thanks for the feedback, I believe that I have implemented your suggestions.

In this case, I left the VERIFY's on purpose, as I believe they express the intent, that is that the numeric type member should have to match the actual numeric type of the value.

[AtkinsSJ]

Excellent! Thanks for that.

[LucasChollet]

This file doesn't end with a new line. I'm surprised the CI didn't pick it up.

[AtkinsSJ]

We don't worry about that for tests. Back in the day I think we manually removed the trailing newlines for layout tests just so they wouldn't add the extra text node.

[LucasChollet]

Oh ok, I'm always fighting the pre-commit hook when I import WPT tests that don't have one.

[AtkinsSJ]

That's odd, maybe your commit hook is outdated? I don't think I've had that issue but maybe I've been importing tests that don't lack the newline.