Bug 229666 – [CSS Cascade Layers] Compute order correctly for late added sublayers

• Component: CSS
• Status: RESOLVED FIXED

[Antti Koivisto]

We currently get order wrong in cases like

@layer a.b {
  ...
}
@layer c {
  ...
}
@layer a.d {
  ...
}

where c should have higher priority than a.d.

[Antti Koivisto]

Created attachment 436763 [details]
patch

[Antti Koivisto]

Created attachment 436764 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 436764 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436764&action=review

> Source/WebCore/ChangeLog:3
> +        [CSS Cascade Layers] Computer order correctly for late added sublayers

Compute order

> Source/WebCore/style/RuleSet.cpp:86
> +void RuleSet::addRule(const StyleRule& rule, unsigned selectorIndex, unsigned selectorListIndex, unsigned cascadeLayerIdentifier, MediaQueryCollector* mediaQueryCollector)

unsigned -> CascadeLayerIdentifier ?

> Source/WebCore/style/RuleSet.cpp:93
> +        std::fill(m_cascadeLayerIdentifierForRulePosition.begin() + oldSize, m_cascadeLayerIdentifierForRulePosition.end(), 0);

Shame they don't init to 0 automatically

> Source/WebCore/style/RuleSet.cpp:405
> +        CascadeLayerIdentifier identifier = 0;

Oh there is a typedef! Perhaps it should be a type which can initialize itself to 0.

> Source/WebCore/style/RuleSet.cpp:470
> +    auto compareCascadeLayers = [&](CascadeLayerIdentifier a, CascadeLayerIdentifier b) {
> +        while (a && b) {
> +            // Identifiers are in parse order which almost corresponds to the layer priority order.
> +            // The only exception is when a sublayer gets added to a layer after adding other non-sublayers.
> +            // To resolve this we need look for a shared ancestor layer.
> +            auto aParent = ruleSet->cascadeLayerForIdentifier(a).parentIdentifier;
> +            auto bParent = ruleSet->cascadeLayerForIdentifier(b).parentIdentifier;
> +            if (aParent == bParent || aParent == b || bParent == a)
> +                break;
> +            if (aParent > bParent)
> +                a = aParent;
> +            else
> +                b = bParent;
> +        }
> +        return a < b;
> +    };
> +
> +    auto updateCascadeLayerOrder = [&] {
> +        Vector<CascadeLayerIdentifier> orderVector;
> +        for (CascadeLayerIdentifier identifier = 1; identifier <= ruleSet->m_cascadeLayers.size(); ++identifier)
> +            orderVector.append(identifier);
> +        std::sort(orderVector.begin(), orderVector.end(), compareCascadeLayers);
> +        for (unsigned i = 0; i < orderVector.size(); ++i)
> +            ruleSet->cascadeLayerForIdentifier(orderVector[i]).order = i + 1;
> +    };
> +
> +    if (mode == Mode::Normal && !ruleSet->m_cascadeLayers.isEmpty())
> +        updateCascadeLayerOrder();

Maybe put this in its own function

> Source/WebCore/style/RuleSet.h:193
> +    CascadeLayer& cascadeLayerForIdentifier(CascadeLayerIdentifier identifier) { return m_cascadeLayers[identifier - 1]; }
> +    const CascadeLayer& cascadeLayerForIdentifier(CascadeLayerIdentifier identifier) const { return m_cascadeLayers[identifier - 1]; }

Maybe assert that identifier != 0

[Antti Koivisto]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 436764 [details]
> Shame they don't init to 0 automatically

Yeah, was going to add a variant of grow that takes an initialization value.

[Antti Koivisto]

> > Source/WebCore/style/RuleSet.h:193
> > +    CascadeLayer& cascadeLayerForIdentifier(CascadeLayerIdentifier identifier) { return m_cascadeLayers[identifier - 1]; }
> > +    const CascadeLayer& cascadeLayerForIdentifier(CascadeLayerIdentifier identifier) const { return m_cascadeLayers[identifier - 1]; }
> 
> Maybe assert that identifier != 0

Didn't seem necessary since vector asserts against overflow.

[Antti Koivisto]

Created attachment 436788 [details]
patch

[Antti Koivisto]

Created attachment 436794 [details]
patch

[EWS]

Committed r281798 (241135@main): <https://commits.webkit.org/241135@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436794 [details].

[Radar WebKit Bug Importer]

<rdar://problem/82574233>