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

LibWeb/CSS: Fix crash resolving calc with only percentages #3007

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jaycadox
Copy link

@Jaycadox Jaycadox commented Dec 22, 2024

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.

@Jaycadox Jaycadox requested a review from AtkinsSJ as a code owner December 22, 2024 13:42
@gmta
Copy link
Collaborator

gmta commented Dec 22, 2024

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

Comment on lines +1893 to 1894
if (result.type().has_value() && (result.type()->matches_length() || result.type()->matches_percentage()))
return Length::make_px(CSSPixels { result.value() });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash when resolving transform: translateX(calc(10%))
3 participants