Support reverting to/from css-logical properties #22614
Merged
Conversation
This file contains 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
wpt-pr-bot
approved these changes
Apr 1, 2020
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-2130850
branch
from
7f2ff80
to
8c01ba6
Compare
The properties defined by css-logical (and more generally surrogates) pose a hard problem for 'revert' in the StyleCascade, because declarations such as 'margin-bottom:revert' may actually need to revert to a different property in the previous origin (e.g. margin-block-end). An example is the h1 element, which is styled by html.css using css-logical properties. When the author then reverts using a corresponding physical property, we can't simply look up the cascaded value for that same physical property in the UA origin: we must be aware that the css-logical property won the cascade _at that level_. So when we we're applying an author declaration 'margin-bottom:revert', we have to find the *target property* for the revert, which is either margin-bottom (if that property won the cascade at the target origin), or the corresponding css-logical property. However, to find the target property, we need to know during the application of margin-bottom that a corresponding logical property (surrogate) needs to be taken into account. AutoSurrogateScope was created for this purpose, which sets (and resets) a "current surrogate" field on CascadeResolver. This current surrogate is used when resolving reverts, in order to find the correct target property. Also, we don't control the order in which physical/logical properties are applied. This means we could be applying the physical property first, which is unaware of any logical properties that should have been taken into account for revert. In other words, if we apply 'margin-bottom:revert' without the context of a "current surrogate" set, we'll fall back to unconditionally using margin-bottom as the target property, which may be incorrect if there also exists a logical property we'll get to later. This is why the force-reapply code exists: when both the physical and logical property exists in the cascade, the physical property must be applied (or re-applied) during AutoSurrogateScope, otherwise revert will not behave correctly. Bug: 579788 Change-Id: I864a4c9afdd94f130c42635d28d45c843d5fb617 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2130850 Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org> Reviewed-by: Rune Lillesveen <futhark@chromium.org> Cr-Commit-Position: refs/heads/master@{#756419}
chromium-wpt-export-bot
force-pushed
the
chromium-export-cl-2130850
branch
from
8c01ba6
to
5c1cd90
Compare
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.
The properties defined by css-logical (and more generally surrogates)
pose a hard problem for 'revert' in the StyleCascade, because
declarations such as 'margin-bottom:revert' may actually need to revert
to a different property in the previous origin (e.g. margin-block-end).
An example is the h1 element, which is styled by html.css using
css-logical properties. When the author then reverts using a
corresponding physical property, we can't simply look up the cascaded
value for that same physical property in the UA origin: we must be
aware that the css-logical property won the cascade at that level.
So when we we're applying an author declaration 'margin-bottom:revert',
we have to find the target property for the revert, which is either
margin-bottom (if that property won the cascade at the target origin),
or the corresponding css-logical property. However, to find the target
property, we need to know during the application of margin-bottom that
a corresponding logical property (surrogate) needs to be taken into
account. AutoSurrogateScope was created for this purpose, which sets
(and resets) a "current surrogate" field on CascadeResolver. This
current surrogate is used when resolving reverts, in order to find
the correct target property.
Also, we don't control the order in which physical/logical properties
are applied. This means we could be applying the physical property
first, which is unaware of any logical properties that should have been
taken into account for revert. In other words, if we apply
'margin-bottom:revert' without the context of a "current surrogate" set,
we'll fall back to unconditionally using margin-bottom as the target
property, which may be incorrect if there also exists a logical
property we'll get to later. This is why the force-reapply code exists:
when both the physical and logical property exists in the cascade,
the physical property must be applied (or re-applied) during
AutoSurrogateScope, otherwise revert will not behave correctly.
Bug: 579788
Change-Id: I864a4c9afdd94f130c42635d28d45c843d5fb617
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2130850
Commit-Queue: Anders Hartvoll Ruud <andruud@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#756419}