Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix @counter-style CSSOM update #11948

Conversation

vitorroriz
Copy link
Contributor

@vitorroriz vitorroriz commented Mar 24, 2023

3fa329d

Fix @counter-style CSSOM update
https://bugs.webkit.org/show_bug.cgi?id=254411
rdar://107184142

Reviewed by Antti Koivisto.

We were not updating the CounterStyles descriptors when
the related CSSOM descriptor was updated.

Now, we define setters for each descriptor of CSSCounterStyleDescriptors.
The setters at CSSCounterStyleRule still use setterInternal()
to set the associated property at StyleProperties. The function
setterInternal() was modified to return true if it did set a new value.

The function newValueInvalidOrEqual() had a small bug in which
it would return symbolsValidForSystem(...). The right thing to do is
return !symbolsValidForSystem(...), since it should true for a invalid
value.

Each setter at CSSCounterStyleRule now returns early if setterInternal()
didn't set a value. If it did set a value we proceed to set the associated
descriptor stored at the rule with the newly created setters.

Ideally, storing only the descriptors
would be enough and we could stop storing StyleProperties. This would
require definining a serializer for each descriptor and modifying the
the CSSCounterStyleRule setters to set the descriptor directly,
without modifying the property first. This improvement should come in
a separated PR and it was was reported at
https://bugs.webkit.org/show_bug.cgi?id=254524
which is tracked by rdar://107267784.

Minor change:
We are also renaming hasUnresolvedReferences to m_hasUnresolvedReferences,
for following webkit's style.

* LayoutTests/TestExpectations:
* Source/WebCore/css/CSSCounterStyleDescriptors.cpp:
(WebCore::translateRangeFromStyleProperties):
(WebCore::translateAdditiveSymbolsFromStyleProperties):
(WebCore::translatePadFromStyleProperties):
(WebCore::translateNegativeSymbolsFromStyleProperties):
(WebCore::translateSymbolsFromStyleProperties):
(WebCore::translateFallbackNameFromStyleProperties):
(WebCore::translatePrefixFromStyleProperties):
(WebCore::translateSuffixFromStyleProperties):
(WebCore::extractDataFromSystemDescriptor):
(WebCore::CSSCounterStyleDescriptors::setNegative):
(WebCore::CSSCounterStyleDescriptors::setPrefix):
(WebCore::CSSCounterStyleDescriptors::setSuffix):
(WebCore::CSSCounterStyleDescriptors::setRanges):
(WebCore::CSSCounterStyleDescriptors::setPad):
(WebCore::CSSCounterStyleDescriptors::setFallbackName):
(WebCore::CSSCounterStyleDescriptors::setSymbols):
(WebCore::CSSCounterStyleDescriptors::setAdditiveSymbols):
* Source/WebCore/css/CSSCounterStyleDescriptors.h:
(WebCore::CSSCounterStyleDescriptors::setName):
* Source/WebCore/css/CSSCounterStyleRegistry.cpp:
(WebCore::CSSCounterStyleRegistry::resolveReferencesIfNeeded):
(WebCore::CSSCounterStyleRegistry::addCounterStyle):
(WebCore::CSSCounterStyleRegistry::clearAuthorCounterStyles):
(WebCore::CSSCounterStyleRegistry::invalidate):
* Source/WebCore/css/CSSCounterStyleRegistry.h:
* Source/WebCore/css/CSSCounterStyleRule.cpp:
(WebCore::StyleRuleCounterStyle::newValueInvalidOrEqual const):
(WebCore::CSSCounterStyleRule::setName):
(WebCore::CSSCounterStyleRule::setterInternal):
(WebCore::CSSCounterStyleRule::setSystem):
(WebCore::CSSCounterStyleRule::setNegative):
(WebCore::CSSCounterStyleRule::setPrefix):
(WebCore::CSSCounterStyleRule::setSuffix):
(WebCore::CSSCounterStyleRule::setRange):
(WebCore::CSSCounterStyleRule::setPad):
(WebCore::CSSCounterStyleRule::setFallback):
(WebCore::CSSCounterStyleRule::setSymbols):
(WebCore::CSSCounterStyleRule::setAdditiveSymbols):
* Source/WebCore/css/CSSCounterStyleRule.h:
* Source/WebCore/style/RuleSetBuilder.cpp:
(WebCore::Style::RuleSetBuilder::addMutatingRulesToResolver):
* Source/WebCore/style/StyleScope.cpp:
(WebCore::Style::Scope::clearResolver):
(WebCore::Style::Scope::updateResolver):

Canonical link: https://commits.webkit.org/262203@main

3265578

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 ⏳ πŸ§ͺ api-mac βœ… πŸ›  gtk
⏳ πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk1 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ›  tv βœ… πŸ§ͺ mac-wk2 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv-sim ⏳ πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  watch βœ… πŸ§ͺ mac-wk2-stress
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch-sim

@vitorroriz vitorroriz self-assigned this Mar 24, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 24, 2023
@vitorroriz vitorroriz removed the merging-blocked Applied to prevent a change from being merged label Mar 25, 2023
@vitorroriz vitorroriz force-pushed the eng/counter-style-rules-updated-via-CSSOM-failed-to-update branch from 0b82236 to 46b463f Compare March 25, 2023 16:44
@vitorroriz vitorroriz added the CSS Cascading Style Sheets implementation label Mar 25, 2023
@vitorroriz vitorroriz force-pushed the eng/counter-style-rules-updated-via-CSSOM-failed-to-update branch from 46b463f to cc75140 Compare March 27, 2023 15:26
@vitorroriz vitorroriz marked this pull request as ready for review March 27, 2023 15:27
@vitorroriz vitorroriz force-pushed the eng/counter-style-rules-updated-via-CSSOM-failed-to-update branch from cc75140 to 759216a Compare March 27, 2023 16:05
@vitorroriz vitorroriz force-pushed the eng/counter-style-rules-updated-via-CSSOM-failed-to-update branch from 759216a to 3265578 Compare March 27, 2023 16:11
@nt1m nt1m requested a review from anttijk March 28, 2023 04:57
Comment on lines +67 to 69
// FIXME: we can get rid of m_properties and use only m_descriptors for storing the descriptors data (rdar://107267784).
Ref<StyleProperties> m_properties;
CSSCounterStyleDescriptors m_descriptors;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it would be good to have only one source of truth here instead of maintaining two.

@vitorroriz vitorroriz added the merge-queue Applied to send a pull request to merge-queue label Mar 28, 2023
https://bugs.webkit.org/show_bug.cgi?id=254411
rdar://107184142

Reviewed by Antti Koivisto.

We were not updating the CounterStyles descriptors when
the related CSSOM descriptor was updated.

Now, we define setters for each descriptor of CSSCounterStyleDescriptors.
The setters at CSSCounterStyleRule still use setterInternal()
to set the associated property at StyleProperties. The function
setterInternal() was modified to return true if it did set a new value.

The function newValueInvalidOrEqual() had a small bug in which
it would return symbolsValidForSystem(...). The right thing to do is
return !symbolsValidForSystem(...), since it should true for a invalid
value.

Each setter at CSSCounterStyleRule now returns early if setterInternal()
didn't set a value. If it did set a value we proceed to set the associated
descriptor stored at the rule with the newly created setters.

Ideally, storing only the descriptors
would be enough and we could stop storing StyleProperties. This would
require definining a serializer for each descriptor and modifying the
the CSSCounterStyleRule setters to set the descriptor directly,
without modifying the property first. This improvement should come in
a separated PR and it was was reported at
https://bugs.webkit.org/show_bug.cgi?id=254524
which is tracked by rdar://107267784.

Minor change:
We are also renaming hasUnresolvedReferences to m_hasUnresolvedReferences,
for following webkit's style.

* LayoutTests/TestExpectations:
* Source/WebCore/css/CSSCounterStyleDescriptors.cpp:
(WebCore::translateRangeFromStyleProperties):
(WebCore::translateAdditiveSymbolsFromStyleProperties):
(WebCore::translatePadFromStyleProperties):
(WebCore::translateNegativeSymbolsFromStyleProperties):
(WebCore::translateSymbolsFromStyleProperties):
(WebCore::translateFallbackNameFromStyleProperties):
(WebCore::translatePrefixFromStyleProperties):
(WebCore::translateSuffixFromStyleProperties):
(WebCore::extractDataFromSystemDescriptor):
(WebCore::CSSCounterStyleDescriptors::setNegative):
(WebCore::CSSCounterStyleDescriptors::setPrefix):
(WebCore::CSSCounterStyleDescriptors::setSuffix):
(WebCore::CSSCounterStyleDescriptors::setRanges):
(WebCore::CSSCounterStyleDescriptors::setPad):
(WebCore::CSSCounterStyleDescriptors::setFallbackName):
(WebCore::CSSCounterStyleDescriptors::setSymbols):
(WebCore::CSSCounterStyleDescriptors::setAdditiveSymbols):
* Source/WebCore/css/CSSCounterStyleDescriptors.h:
(WebCore::CSSCounterStyleDescriptors::setName):
* Source/WebCore/css/CSSCounterStyleRegistry.cpp:
(WebCore::CSSCounterStyleRegistry::resolveReferencesIfNeeded):
(WebCore::CSSCounterStyleRegistry::addCounterStyle):
(WebCore::CSSCounterStyleRegistry::clearAuthorCounterStyles):
(WebCore::CSSCounterStyleRegistry::invalidate):
* Source/WebCore/css/CSSCounterStyleRegistry.h:
* Source/WebCore/css/CSSCounterStyleRule.cpp:
(WebCore::StyleRuleCounterStyle::newValueInvalidOrEqual const):
(WebCore::CSSCounterStyleRule::setName):
(WebCore::CSSCounterStyleRule::setterInternal):
(WebCore::CSSCounterStyleRule::setSystem):
(WebCore::CSSCounterStyleRule::setNegative):
(WebCore::CSSCounterStyleRule::setPrefix):
(WebCore::CSSCounterStyleRule::setSuffix):
(WebCore::CSSCounterStyleRule::setRange):
(WebCore::CSSCounterStyleRule::setPad):
(WebCore::CSSCounterStyleRule::setFallback):
(WebCore::CSSCounterStyleRule::setSymbols):
(WebCore::CSSCounterStyleRule::setAdditiveSymbols):
* Source/WebCore/css/CSSCounterStyleRule.h:
* Source/WebCore/style/RuleSetBuilder.cpp:
(WebCore::Style::RuleSetBuilder::addMutatingRulesToResolver):
* Source/WebCore/style/StyleScope.cpp:
(WebCore::Style::Scope::clearResolver):
(WebCore::Style::Scope::updateResolver):

Canonical link: https://commits.webkit.org/262203@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/counter-style-rules-updated-via-CSSOM-failed-to-update branch from 3265578 to 3fa329d Compare March 28, 2023 09:59
@webkit-commit-queue
Copy link
Collaborator

Committed 262203@main (3fa329d): https://commits.webkit.org/262203@main

Reviewed commits have been landed. Closing PR #11948 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Mar 28, 2023
@webkit-commit-queue webkit-commit-queue merged commit 3fa329d into WebKit:main Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Cascading Style Sheets implementation
Projects
None yet
5 participants