Bug 229666 - [CSS Cascade Layers] Compute order correctly for late added sublayers
Summary: [CSS Cascade Layers] Compute order correctly for late added sublayers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 220779
  Show dependency treegraph
 
Reported: 2021-08-30 03:29 PDT by Antti Koivisto
Modified: 2021-08-31 04:49 PDT (History)
3 users (show)

See Also:


Attachments
patch (12.73 KB, patch)
2021-08-30 04:12 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (12.73 KB, patch)
2021-08-30 05:38 PDT, Antti Koivisto
simon.fraser: review+
Details | Formatted Diff | Diff
patch (13.16 KB, patch)
2021-08-30 10:10 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (13.26 KB, patch)
2021-08-30 10:56 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2021-08-30 03:29:43 PDT
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.
Comment 1 Antti Koivisto 2021-08-30 04:12:42 PDT
Created attachment 436763 [details]
patch
Comment 2 Antti Koivisto 2021-08-30 05:38:49 PDT
Created attachment 436764 [details]
patch
Comment 3 Simon Fraser (smfr) 2021-08-30 08:46:27 PDT
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
Comment 4 Antti Koivisto 2021-08-30 09:22:09 PDT
(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.
Comment 5 Antti Koivisto 2021-08-30 10:06:11 PDT
> > 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.
Comment 6 Antti Koivisto 2021-08-30 10:10:18 PDT
Created attachment 436788 [details]
patch
Comment 7 Antti Koivisto 2021-08-30 10:56:07 PDT
Created attachment 436794 [details]
patch
Comment 8 EWS 2021-08-31 04:48:20 PDT
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].
Comment 9 Radar WebKit Bug Importer 2021-08-31 04:49:25 PDT
<rdar://problem/82574233>