[css-cascade] revert-layer should roll back to preshints

• Component: CSS
• Status: RESOLVED FIXED

[Oriol Brufau]

Load this: https://software.hixie.ch/utilities/js/live-dom-viewer/saved/10100

  <style>
  iframe {
    width: revert-layer;
    height: revert-layer;
  }
  </style>
  <iframe width="44" height="33"></iframe>

Expected: 'revert-layer' rolls back the cascaded value to the presentational hints origin, the the iframe is 44x33.
Actual: the iframe is 300x150.

See https://drafts.csswg.org/css-cascade-5/#preshint, https://github.com/w3c/csswg-drafts/issues/7087
Presentational hints should only be considered part of the author origin for 'revert', but not for 'revert-layer'.

Chromium and Firefox do it correctly.

[Radar WebKit Bug Importer]

<rdar://problem/90106420>

[Oriol Brufau]

Created attachment 454672 [details]
Patch

[Oriol Brufau]

Created attachment 454702 [details]
Patch

[Oriol Brufau]

PTAL.

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Darin Adler]

Comment on attachment 454702 [details]
Patch

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

> Source/WebCore/style/ElementRuleCollector.cpp:129
> +inline void ElementRuleCollector::addElementStyleProperties(const StyleProperties* propertySet, CascadeLayerPriority cascadeLayerPriority, bool isCacheable, FromStyleAttribute fromStyleAttribute)

Since the type is so specific and strong typing prevents us from making obvious mistakes, I think we could take the risk of just calling this argument priority. Single-word names often make things easier to read.

> Source/WebCore/style/RuleSetBuilder.cpp:294
> +        // Priority 0 is reserved for presentational hints.
> +        auto priority = std::min<unsigned>(i + 1, RuleSet::cascadeLayerPriorityForUnlayered - 1);

I find this comment confusing; it says some, but not all, of why the code below does what it does and is correct. I think the issue is that priorities matter only relative to each other, so we use "+1" to avoid clobbering the priority of 0. But that isn’t said, and the code below has "i + 1" and since I can see the diff I know the comment is about the "+ 1" in that but also has "RuleSet::cascadeLayerPriorityForUnlayered - 1" and it’s quite unclear why that value is correct.

[Oriol Brufau]

Created attachment 455268 [details]
Patch

[Oriol Brufau]

Thanks!

(In reply to Darin Adler from comment #6)
> Since the type is so specific and strong typing prevents us from making
> obvious mistakes, I think we could take the risk of just calling this
> argument priority. Single-word names often make things easier to read.

Done.

> > Source/WebCore/style/RuleSetBuilder.cpp:294
> > +        // Priority 0 is reserved for presentational hints.
> > +        auto priority = std::min<unsigned>(i + 1, RuleSet::cascadeLayerPriorityForUnlayered - 1);
> 
> I find this comment confusing

Changed it to:

// Priorities matter only relative to each other, so we use "+1" to avoid clobbering the priority of 0, which is reserved for presentational hints.

[Antti Koivisto]

Comment on attachment 455268 [details]
Patch

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

> Source/WebCore/style/ElementRuleCollector.cpp:553
> +        addElementStyleProperties(styledElement.presentationalHintStyle(), 0);

We should use named constant here (similar to cascadeLayerPriorityForUnlayered) instead of magic 0.

[Oriol Brufau]

Created attachment 455274 [details]
Patch

[Oriol Brufau]

(In reply to Antti Koivisto from comment #9)
> We should use named constant here (similar to
> cascadeLayerPriorityForUnlayered) instead of magic 0.

OK, added RuleSet::cascadeLayerPriorityForPresentationalHints.

Then I have also updated the comment in RuleSetBuilder.cpp:

    // Priorities matter only relative to each other, so assign them enforcing these constraints:
    // - Layers must get a priority greater than RuleSet::cascadeLayerPriorityForPresentationalHints.
    // - Layers must get a priority smaller than RuleSet::cascadeLayerPriorityForUnlayered.
    // - A layer must get at least the same priority as the previous one.
    // - A layer should get more priority than the previous one, but this may be impossible if there are too many layers.
    //   In that case, the last layers will get the maximum priority for layers, RuleSet::cascadeLayerPriorityForUnlayered - 1.
    for (unsigned i = 0; i < layerCount; ++i) {
        auto priority = std::min<unsigned>(i + RuleSet::cascadeLayerPriorityForPresentationalHints + 1, RuleSet::cascadeLayerPriorityForUnlayered - 1);
        m_ruleSet->cascadeLayerForIdentifier(layersInPriorityOrder[i]).priority = priority;
    }

[Oriol Brufau]

Comment on attachment 454702 [details]
Patch

I think wpt-pr-bot is not approving the PR because r+ got cleared when uploading a new patch.

[EWS]

Committed r291594 (248688@main): <https://commits.webkit.org/248688@main>

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