- Revision
- 279044
- Author
- [email protected]
- Date
- 2021-06-18 12:44:30 -0700 (Fri, 18 Jun 2021)
Log Message
[css-logical] Fix cssom "set a CSS declaration" for logical properties
https://bugs.webkit.org/show_bug.cgi?id=226461
Reviewed by Antti Koivisto.
LayoutTests/imported/w3c:
This test is now passing.
* web-platform-tests/css/cssom/cssstyledeclaration-setter-logical-expected.txt:
Source/WebCore:
Test: imported/w3c/web-platform-tests/css/cssom/cssstyledeclaration-setter-logical.html
Before this patch, setting a value to a property already in the list of
declarations, would just update its value, without reordering.
The problem was that the order is important when there is a mix of
logical and physical properties:
el.style.paddingTop = "1px";
el.style.paddingBlockStart = "2px";
el.style.cssText; // "padding-top: 1px; padding-block-start: 2px"
el.style.paddingTop = "3px";
el.style.cssText; // "padding-top: 3px; padding-block-start: 2px"
getComputedStyle(el).paddingTop; // "2px" -- no effect!
Therefore, this patch implements this part of the spec:
> If there are CSS declarations in declarations whose property name is
> in the same logical property group as property, but has a different
> mapping logic, target declaration must be at an index after all of
> those CSS declarations.
This change is based on this Chromium CL:
https://chromium-review.googlesource.com/c/chromium/src/+/2575081/
* css/CSSProperty.h:
* css/StyleProperties.cpp:
(WebCore::MutableStyleProperties::canUpdateInPlace const):
(WebCore::MutableStyleProperties::setProperty):
* css/StyleProperties.h:
* css/makeprop.pl:
Modified Paths
Diff
Modified: trunk/LayoutTests/imported/w3c/ChangeLog (279043 => 279044)
--- trunk/LayoutTests/imported/w3c/ChangeLog 2021-06-18 19:15:40 UTC (rev 279043)
+++ trunk/LayoutTests/imported/w3c/ChangeLog 2021-06-18 19:44:30 UTC (rev 279044)
@@ -1,3 +1,14 @@
+2021-06-18 Oriol Brufau <[email protected]>
+
+ [css-logical] Fix cssom "set a CSS declaration" for logical properties
+ https://bugs.webkit.org/show_bug.cgi?id=226461
+
+ Reviewed by Antti Koivisto.
+
+ This test is now passing.
+
+ * web-platform-tests/css/cssom/cssstyledeclaration-setter-logical-expected.txt:
+
2021-06-17 Chris Dumez <[email protected]>
Add support for IDBCursor.request
Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/css/cssom/cssstyledeclaration-setter-logical-expected.txt (279043 => 279044)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/css/cssom/cssstyledeclaration-setter-logical-expected.txt 2021-06-18 19:15:40 UTC (rev 279043)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/css/cssom/cssstyledeclaration-setter-logical-expected.txt 2021-06-18 19:44:30 UTC (rev 279044)
@@ -1,3 +1,3 @@
-FAIL newly set declaration should be after all declarations in the same logical property group but have different logical kind assert_less_than: padding-top should be after padding-block-start after setting padding-top expected a number less than 0 but got 4
+PASS newly set declaration should be after all declarations in the same logical property group but have different logical kind
Modified: trunk/Source/WebCore/ChangeLog (279043 => 279044)
--- trunk/Source/WebCore/ChangeLog 2021-06-18 19:15:40 UTC (rev 279043)
+++ trunk/Source/WebCore/ChangeLog 2021-06-18 19:44:30 UTC (rev 279044)
@@ -1,3 +1,41 @@
+2021-06-18 Oriol Brufau <[email protected]>
+
+ [css-logical] Fix cssom "set a CSS declaration" for logical properties
+ https://bugs.webkit.org/show_bug.cgi?id=226461
+
+ Reviewed by Antti Koivisto.
+
+ Test: imported/w3c/web-platform-tests/css/cssom/cssstyledeclaration-setter-logical.html
+
+ Before this patch, setting a value to a property already in the list of
+ declarations, would just update its value, without reordering.
+
+ The problem was that the order is important when there is a mix of
+ logical and physical properties:
+
+ el.style.paddingTop = "1px";
+ el.style.paddingBlockStart = "2px";
+ el.style.cssText; // "padding-top: 1px; padding-block-start: 2px"
+ el.style.paddingTop = "3px";
+ el.style.cssText; // "padding-top: 3px; padding-block-start: 2px"
+ getComputedStyle(el).paddingTop; // "2px" -- no effect!
+
+ Therefore, this patch implements this part of the spec:
+ > If there are CSS declarations in declarations whose property name is
+ > in the same logical property group as property, but has a different
+ > mapping logic, target declaration must be at an index after all of
+ > those CSS declarations.
+
+ This change is based on this Chromium CL:
+ https://chromium-review.googlesource.com/c/chromium/src/+/2575081/
+
+ * css/CSSProperty.h:
+ * css/StyleProperties.cpp:
+ (WebCore::MutableStyleProperties::canUpdateInPlace const):
+ (WebCore::MutableStyleProperties::setProperty):
+ * css/StyleProperties.h:
+ * css/makeprop.pl:
+
2021-06-18 Peng Liu <[email protected]>
[iOS] Fullscreen video playback gets stuck after interacting with the playback controls
Modified: trunk/Source/WebCore/css/CSSProperty.h (279043 => 279044)
--- trunk/Source/WebCore/css/CSSProperty.h 2021-06-18 19:15:40 UTC (rev 279043)
+++ trunk/Source/WebCore/css/CSSProperty.h 2021-06-18 19:44:30 UTC (rev 279044)
@@ -80,6 +80,8 @@
static bool isInheritedProperty(CSSPropertyID);
static Vector<String> aliasesForProperty(CSSPropertyID);
static bool isDirectionAwareProperty(CSSPropertyID);
+ static bool isInLogicalPropertyGroup(CSSPropertyID);
+ static bool areInSameLogicalPropertyGroupWithDifferentMappingLogic(CSSPropertyID, CSSPropertyID);
static bool isDescriptorOnly(CSSPropertyID);
static bool isColorProperty(CSSPropertyID);
Modified: trunk/Source/WebCore/css/StyleProperties.cpp (279043 => 279044)
--- trunk/Source/WebCore/css/StyleProperties.cpp 2021-06-18 19:15:40 UTC (rev 279043)
+++ trunk/Source/WebCore/css/StyleProperties.cpp 2021-06-18 19:44:30 UTC (rev 279044)
@@ -918,6 +918,23 @@
m_propertyVector.append(CSSProperty(longhand, value.copyRef(), important));
}
+bool MutableStyleProperties::canUpdateInPlace(const CSSProperty& property, CSSProperty* toReplace) const
+{
+ // If the property is in a logical property group, we can't just update the value in-place,
+ // because afterwards there might be another property of the same group but different mapping logic.
+ // In that case the latter might override the former, so setProperty would have no effect.
+ CSSPropertyID id = property.id();
+ if (CSSProperty::isInLogicalPropertyGroup(id)) {
+ ASSERT(toReplace >= m_propertyVector.begin());
+ ASSERT(toReplace < m_propertyVector.end());
+ for (CSSProperty* it = toReplace + 1; it != m_propertyVector.end(); ++it) {
+ if (CSSProperty::areInSameLogicalPropertyGroupWithDifferentMappingLogic(id, it->id()))
+ return false;
+ }
+ }
+ return true;
+}
+
bool MutableStyleProperties::setProperty(const CSSProperty& property, CSSProperty* slot)
{
if (!removeShorthandProperty(property.id())) {
@@ -931,11 +948,15 @@
}
if (toReplace) {
- if (*toReplace == property)
- return false;
+ if (canUpdateInPlace(property, toReplace)) {
+ if (*toReplace == property)
+ return false;
- *toReplace = property;
- return true;
+ *toReplace = property;
+ return true;
+ }
+ m_propertyVector.remove(toReplace - m_propertyVector.begin());
+ toReplace = nullptr;
}
}
Modified: trunk/Source/WebCore/css/StyleProperties.h (279043 => 279044)
--- trunk/Source/WebCore/css/StyleProperties.h 2021-06-18 19:15:40 UTC (rev 279043)
+++ trunk/Source/WebCore/css/StyleProperties.h 2021-06-18 19:44:30 UTC (rev 279044)
@@ -267,6 +267,7 @@
CSSProperty* findCSSPropertyWithID(CSSPropertyID);
CSSProperty* findCustomCSSPropertyWithName(const String&);
std::unique_ptr<PropertySetCSSStyleDeclaration> m_cssomWrapper;
+ bool canUpdateInPlace(const CSSProperty&, CSSProperty* toReplace) const;
friend class StyleProperties;
};
Modified: trunk/Source/WebCore/css/makeprop.pl (279043 => 279044)
--- trunk/Source/WebCore/css/makeprop.pl 2021-06-18 19:15:40 UTC (rev 279043)
+++ trunk/Source/WebCore/css/makeprop.pl 2021-06-18 19:44:30 UTC (rev 279044)
@@ -658,6 +658,58 @@
}
}
+bool CSSProperty::isInLogicalPropertyGroup(CSSPropertyID id)
+{
+ switch (id) {
+EOF
+
+for my $logicalPropertyGroup (values %logicalPropertyGroups) {
+ for my $kind ("logical", "physical") {
+ for my $name (values %{ $logicalPropertyGroup->{$kind} }) {
+ print GPERF " case CSSPropertyID::CSSProperty" . $nameToId{$name} . ":\n";
+ }
+ }
+}
+
+print GPERF << "EOF";
+ return true;
+ default:
+ return false;
+ }
+}
+
+bool CSSProperty::areInSameLogicalPropertyGroupWithDifferentMappingLogic(CSSPropertyID id1, CSSPropertyID id2)
+{
+ switch (id1) {
+EOF
+
+for my $logicalPropertyGroup (values %logicalPropertyGroups) {
+ my $logical = $logicalPropertyGroup->{"logical"};
+ my $physical = $logicalPropertyGroup->{"physical"};
+ for my $first ($logical, $physical) {
+ my $second = $first eq $logical ? $physical : $logical;
+ while (my ($resolver, $name) = each %{ $first }) {
+ print GPERF " case CSSPropertyID::CSSProperty" . $nameToId{$name} . ":\n";
+ }
+ print GPERF " switch (id2) {\n";
+ while (my ($resolver, $name) = each %{ $second }) {
+ print GPERF " case CSSPropertyID::CSSProperty" . $nameToId{$name} . ":\n";
+ }
+ print GPERF << "EOF";
+ return true;
+ default:
+ return false;
+ }
+EOF
+ }
+}
+
+print GPERF << "EOF";
+ default:
+ return false;
+ }
+}
+
CSSPropertyID CSSProperty::resolveDirectionAwareProperty(CSSPropertyID propertyID, TextDirection direction, WritingMode writingMode)
{
const TextFlow& textflow = makeTextFlow(writingMode, direction);
