Repository navigation
fix(core): resolve css inherit on non-inherited properties to the parent value - #11521
Draft
edusperoni wants to merge 1 commit into
Draft
edusperoni wants to merge 1 commit into
edusperoni wants to merge 1 commit into
Conversation
…ent value A stylesheet `inherit` on a property that is not inherited by default (margin, border, background-color, opacity, width, ...) went through the same reset branch as `initial` and `unset`, so it always produced the default value instead of the parent's. The css setters of CssProperty and CssAnimationProperty now resolve `inherit` against the parent style. Since non-inherited values are never pushed down the tree, the child subscribes to the parent's change event for that one property, re-resolves on every parent change, and re-subscribes when it is moved to another parent. The subscription only exists while a stylesheet resolves the property to `inherit`, so the common path adds no listeners, allocations or child walks. Shorthands now expand a css-wide keyword to the same keyword on every longhand. Their converters only parse values, so `margin: inherit` and `border-color: inherit` failed to apply, and `font: inherit` and `transform: inherit` turned into fixed values. A keyword set from code keeps releasing the local value; that is documented on isResetValue.
|
View your CI Pipeline Execution ↗ for commit 236c81f
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
commit: |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Checklist
What is the current behavior?
Every css-wide keyword goes through one reset branch (
isResetValue).InheritedCssProperty(color,font-size, ...) resolves the keywords per spec, but a plainCssPropertyand aCssAnimationPropertymap every keyword to the default. For a non-inherited property,initial,unsetandrevert→ initial value is correct, butinheritmust take the parent's value:Shorthands are worse: their converters only parse values, so
margin: inherit,border-color: inherit,padding: inherit,background: inherit, ... fail to expand and are not applied at all, andfont: inherit/transform: inheritturn into fixed values (font-style: normal,translateX: 0, ...).What is the new behavior?
A stylesheet
inheriton a non-inherited property resolves to the parent view's current value, or the default when there is no parent or the parent has no value.CssProperty: the css setter resolvesinheritagainst the parent style and marks the valueValueSource.Inherited(also while the parent is at its default, so it keeps following it)._isCssValueStillAppliedtreats a recordedinheritas applied only while that mark is there, so a released local value makes the cascade write it again, while an untouched one is still skipped.CssAnimationProperty(background-color,opacity,width,height, transforms): the css slot holds the parent's computed value. It keeps following the parent under a local or keyframe value, so it is current when they are released.inheritsubscribes to the parent style's<property>Changeevent for that one property and writesinheritagain on each change. The subscription is created only when a stylesheet resolves a property toinherit, and dropped as soon as the stylesheet (or, for aCssProperty, a local value) stops doing so. The common path adds no listeners, allocations or child walks: the parent's setter already checkshasListenersbefore notifying, and the setters only gain a source comparison.clearInheritedProperties(removal) andpropagateInheritableCssProperties(add) re-resolve the followed properties, which moves the subscription to the new parent, or resolves to the default with none.defaultValueKeycapture/restore,valueChanged,equalityComparer, change events). A child following a parent at its default takes the reset path, so its own native default is restored._expandCssShorthandexpands a css-wide keyword into the same keyword on every longhand (margin: inherit→margin-top: inherit, ...), so each longhand applies it with its own semantics. This covers stylesheet rules, inline styles andvar()substitutions that resolve to a keyword.view.style.backgroundColor = 'inherit'(or'initial', ...) still just releases the local value. Unlike an inline CSS declaration it is not a sticky value that outranks the stylesheet; that is intentional and now documented onisResetValueand theCssPropertylocal setter.Tests:
packages/core/ui/core/properties/css-inherit-non-inherited.spec.ts(26 tests, 15 of them fail onmain): parent value, following later parent changes, the parent's css applied after the child, chainedinherit, no parent / parent at default,initial/unset/revertstill → default, re-parenting (incl. dropping the old parent's listener), the stylesheet changing away frominherit, local value overinheritand release, the unchanged-inheritskip on the next pass, native writes, no listener on the parent when no child usesinherit,background-color/opacity, andmargin/border-color/font: inheritplusmargin: initial.apps/automated/src/ui/styling/css-keywords-tests.ts: addedtest_css_inherit_on_non_inherited_propertyandtest_css_inherit_on_shorthand.test_value_after_inherit(code-set'inherit'releases the local value) still holds, as it pins the intended local-keyword behavior; its staleTODOnow says so. I reviewed the other automated style tests that use keywords orunsetValue(value-source-tests.ts,style-tests.ts); they exercise inherited properties or local resets and are not affected. The automated app was not run on a device or simulator for this PR.Related: #11520 (local reset restores the cascaded value) and #11413 (inherited children reset cascade). This branch merges cleanly with each of them and with both together, and the core suite passes on the three-way merge. One interaction to settle once both land: #11520's
cascadedCssValuetreats a cascadedinheritas "no css value", so a local reset over a stylesheetinheriton aCssPropertyfalls back to the default until the next cascade pass, which then re-resolves it (aCssAnimationPropertyis already correct). Routing a cascadedinheritthroughstyle[cssName] = 'inherit'in that path would close it.