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.
<rdar://problem/90106420>
Created attachment 454672 [details] Patch
Created attachment 454702 [details] Patch
PTAL.
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
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.
Created attachment 455268 [details] Patch
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.
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.
Created attachment 455274 [details] Patch
(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; }
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.
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].